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

[Thomas Huth]

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...

> 
>> +	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" ?

>> +		}
>> +	}
>> +}
>> +
> 
> 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.

>> +		free(bootdevs[nr_devs].path);
>> +		bootdevs[nr_devs].path = NULL;
>> +	}
>> +}
>> +
>> +static void bootmenu_show_devs(void)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < nr_devs; i++) {
>> +		printf("%c) %6s : %s\n", i < 9 ? '1' + i : 'a' + i - 9,
>> +		       bootdevs[i].alias, bootdevs[i].path);
>> +	}
>> +}
>> +
>>  void bootmenu(void)
>>  {
>> +	bootmenu_populate_devs();
>> +	if (!nr_devs) {
>> +		puts("No available boot devices!");
>> +		return;
>> +	}
>> +
>> +	bootmenu_show_devs();
>> +
>> +	bootmenu_free_devs();
> 
> The separation of patches look strange to me - the code above does not make
> sense alone (no user input yet) and it is not called anyway until 4/4 but
> if afterwards a bug is found in let's say bootmenu_populate_devs() - git
> bisest will point to the last patch in the series. Reviewing is not easier
> either - I need to find all changes to this function in later patch(es).
> 
> I suggest merging them all into a single patch, it is not going to be huge
> anyway.

I've split the patches up for easier review, but if you prefer, sure, I
can also merge them into one patch instead.

 Thomas