| Message ID | 1496330742-18181-3-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas Huth <thuth@redhat.com> writes: > 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@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); > + 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++) { Here we would have cdrom - cdrom9, disk - disk9, and net - net9. That is total 30 devices with cap on 10 devices of one type. I guess this is intentional. Regards Nikunj
On 05.06.2017 07:58, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: >> 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@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); >> + 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++) { > > Here we would have cdrom - cdrom9, disk - disk9, and net - net9. That is > total 30 devices with cap on 10 devices of one type. I guess this is > intentional. Yes, the Forth code currently even limits the number of aliases per class to 8, see MAX-ALIAS in node.fs. So we currently never have aliases with more than one digit at the end. So I think limiting the boot menu code here to one digit is OK here, too. But maybe we should increase MAX-ALIAS to 10 instead of 8 ? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 05.06.2017 07:58, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> +#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 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++) { >> >> Here we would have cdrom - cdrom9, disk - disk9, and net - net9. That is >> total 30 devices with cap on 10 devices of one type. I guess this is >> intentional. > > Yes, the Forth code currently even limits the number of aliases per > class to 8, see MAX-ALIAS in node.fs. So we currently never have aliases > with more than one digit at the end. > So I think limiting the boot menu code here to one digit is OK here, > too. But maybe we should increase MAX-ALIAS to 10 instead of 8 ? Yes, that will be in line with new boot menu. Regards Nikunj
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@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. > } >
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@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
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@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.
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); + 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; + } + } +} + +static void bootmenu_free_devs(void) +{ + while (nr_devs > 0) { + nr_devs -= 1; + 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(); }
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@redhat.com> --- lib/libbootmenu/bootmenu.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)