[SLOF] [PATCH 2/4] bootmenu: Gather devices and print the menu

[Alexey Kardashevskiy]

On 07/06/17 01:06, Thomas Huth wrote:
> On 06.06.2017 11:20, Alexey Kardashevskiy wrote:
>> On 02/06/17 01:25, Thomas Huth wrote:
>>> Examine the aliases to get a list of possible boot devices
>>> and print a list with all these devices.
>>>
>>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>>> ---
>>>  lib/libbootmenu/bootmenu.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 86 insertions(+)
>>>
>>> diff --git a/lib/libbootmenu/bootmenu.c b/lib/libbootmenu/bootmenu.c
>>> index d8d00cb..649e518 100644
>>> --- a/lib/libbootmenu/bootmenu.c
>>> +++ b/lib/libbootmenu/bootmenu.c
>>> @@ -14,8 +14,94 @@
>>>  
>>>  #include <string.h>
>>>  #include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <paflof.h>
>>>  #include "bootmenu.h"
>>>  
>>> +#define MAX_DEVS 36        /* Enough for 10 digits + 26 letters */
>>> +#define MAX_ALIAS_LEN 8    /* Maximum length of alias names */
>>> +
>>> +struct bootdev {
>>> +	char alias[MAX_ALIAS_LEN];
>>> +	char *path;
>>> +};
>>> +
>>> +static int nr_devs;
>>> +static struct bootdev bootdevs[MAX_DEVS];
>>> +
>>> +/**
>>> + * Look up an alias name.
>>> + * @return The NUL-terminated device tree path (should be released with free()
>>> + *         when it's not required anymore), or NULL if it can't be found.
>>> + */
>>> +static char *find_alias(char *alias)
>>> +{
>>> +	char *path;
>>> +	long len;
>>> +
>>> +	forth_push((unsigned long)alias);
>>> +	forth_push(strlen(alias));
>>> +	forth_eval("find-alias");
>>> +
>>> +	len = forth_pop();
>>> +	if (!len)
>>> +		return NULL;
>>> +
>>> +	path = malloc(len + 1);
>>
>> Sometime we do check return from malloc() in SLOF, sometime we do not...
> 
> Yeah ... it's likely better to check for NULL here...


Yes, this was my only real concern here, if not this one, I would not
bother mentioning others :)

> 
>>
>>> +	memcpy(path, (void *)forth_pop(), len);
>>> +	path[len] = '\0';
>>> +
>>> +	return path;
>>> +}
>>> +
>>> +static void bootmenu_populate_devs(void)
>>> +{
>>> +	char *aliases[] = { "cdrom", "disk", "net", NULL };
>>> +	int ai, idx;
>>> +
>>> +	for (ai = 0; aliases[ai] != NULL; ai++) {
>>> +		for (idx = 0; idx <= 9; idx++) {
>>> +			char *cur_alias = bootdevs[nr_devs].alias;
>>> +			if (idx == 0)
>>> +				strcpy(cur_alias, aliases[ai]);
>>> +			else
>>> +				sprintf(cur_alias, "%s%i", aliases[ai], idx);
>>> +			bootdevs[nr_devs].path = find_alias(cur_alias);
>>> +			if (!bootdevs[nr_devs].path)
>>> +				break;
>>> +			nr_devs += 1;
>>
>> It is usually nr_devs++ or ++nr_devs, why "+= 1"?
> 
> Just my personal taste. Why not "+= 1" ?

++ is lot more common and seeing +=1 alerts me that it probably should have
been something else, like +=10 and "0" was lost in the process. It is more
or less like your hatred of unnecessary braces.


> 
>>> +		}
>>> +	}
>>> +}
>>> +
>>
>> static void bootmenu_populate_devs(void)
>> {
>> 	bootmenu_populate_devs_alias("cdrom");
>> 	bootmenu_populate_devs_alias("disk");
>> 	bootmenu_populate_devs_alias("net");
>> }
>>
>> and add bootmenu_populate_devs_alias() with just a single loop?
> 
> Good idea, that avoids one level of indentation.
> 
>>> +static void bootmenu_free_devs(void)
>>> +{
>>> +	while (nr_devs > 0) {
>>> +		nr_devs -= 1;
>>
>> --nr_devs?
>> Or even for ( ; nr_devs > 0; --nr_devs) ?
> 
> No, your for-loop example is wrong. That will decrement nr_devs at the
> end of the loop instead of the beginning of the loop.

ok.




-- 
Alexey