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

[Alexey Kardashevskiy]

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


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

> +		}
> +	}
> +}
> +

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?



> +static void bootmenu_free_devs(void)
> +{
> +	while (nr_devs > 0) {
> +		nr_devs -= 1;

--nr_devs?
Or even for ( ; nr_devs > 0; --nr_devs) ?

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


>  }
> 


-- 
Alexey