[U-Boot] [PATCH v2 2/4] New command bootmenu: ANSI terminal Boot Menu support

[Pali Rohár]

Hi, I will fix all mentioned style problems.

On Tuesday 13 November 2012 09:27:41 Wolfgang Denk wrote:
> > +				/* key up was pressed */
> > +				if (key == 'A')
> > +					key = 1;
> > +				/* key down was pressed */
> > +				else if (key == 'B')
> > +					key = 2;
> > +				/* other key was pressed */
> > +				else
> > +					key = 0;
> > +
> > +			}
> 
> use switch() ?
> 
> Can we please avoid using hard-coded magic constants like 'A',
> 'B' etc. here?
> 

And what to use? ANSI sequence for key up is \e[1A (down is B).

> 
> > +	while ((option = bootmenu_getoption(i))) {
> > +
> > +		sep = strchr(option, '=');
> > +		if (!sep)
> > +			break;
> 
> Is there any specific reason for inventing yet another data
> format here?  Can we not for example re-use what we already
> have in the hwconfig command, and use common code for parsing
> the format?
> 
> Using a '=' as separator here is not a good idea, IMO.
> 

So which char is better for separator? Bootmenu line has config 
"name=command" and I think '=' is the best char for separating 
key and value pair.

> > +	/* Add U-Boot console entry at the end */
> > +	if (i < 100) {
> 
> Magic constant 100 ?
> 

<i>-th menu entry is stored in env bootmenu_<i> (you can look to 
README for API). So I need to create string "bootmenu_<i>" and 
call getenv. I think it is very bad idea to use malloc for very 
allocating string of size: strlen("bootmenu_") + log10(i) + 2. 
I'm using array of size strlen("bootmenu_") + 3 allocated at 
stack which is enught for number less then 100. And I think 100 
menu entries in uboot is really a lot of. If you do not think I 
can increase number to 1000 or 10000 (which increase size of 
string +2)...

> > +cleanup:
> > +	iter = menu->first;
> > +	while (iter) {
> > +		entry = iter->next;
> > +		free(iter->title);
> > +		free(iter->command);
> > +		free(iter);
> > +		iter = entry;
> > +	}
> > +	free(menu);
> 
> Make this a separate function?
> 

Same as function bootmenu_destroy, so I changed code to call 
bootmenu_destroy.

> 
> > +	/* If delay is 0 do not create menu, just run first entry
> > */
> > +	if (delay == 0) {
> > +		option = bootmenu_getoption(0);
> > +		if (!option)
> > +			return;
> > +		sep = strchr(option, '=');
> > +		if (!sep)
> > +			return;
> 
> That would be an error condition, would it not?  Should this
> not raise some error message, then?
> 

Ok, I added error message.

> > +int menu_show(int bootdelay)
> > +{
> > +	bootmenu_show(bootdelay);
> > +	return -1; /* -1 - abort boot and run monitor code */
> > +}
> 
> If this function never returns anything else, why do we need
> the return value at all?
> 

You can see that this function is not static. Reason is that 
menu_show is uboot API function which must return int. -1 means 
abort booting and run monitor code.

> > +int do_bootmenu(cmd_tbl_t *cmdtp, int flag, int argc, char
> > *const argv[]) +{
> > +	char *delay_str = NULL;
> > +	int delay = 10;
> > +
> > +	if (argc >= 2)
> > +		delay_str = argv[1];
> > +
> > +	if (!delay_str)
> > +		delay_str = getenv("bootmenu_delay");
> > +
> > +	if (delay_str)
> > +		delay = (int)simple_strtol(delay_str, NULL, 10);
> > +
> > +	bootmenu_show(delay);
> > +	return 0;
> > +}
> 
> Umm... don't we handle any errors at all?
> 

I do not understand what you mean.

> > +First argument of bootmenu command override bootmenu_delay
> > env
> I cannot parse this.
> 

First argument of bootmenu command is delay and override env 
bootmenu_delay.

> > +If env bootmenu_delay or bootmenu arg is not specified,
> > delay is 10 seconds
> Why not using CONFIG_BOOTDELAY as default instead?
> 

Ok.

> > +If delay is 0, no entry will be shown on screen and first
> > will be called +If delay is less then 0, no autoboot delay
> > will be used
> What will happen then?  How is this different from delay==0 ?
> 

If delay is less then 0, bootmenu will be shown and autoboot
will be disabled. So there will not be any timer which 
automatically boot first entry in time $delay.

> > +Boot Menu always add menu entry "U-Boot console" at end of
> > all entries +
> > +Example using:
> > +
> > +  setenv bootmenu_0 Boot 1. kernel=bootm 0x82000000  # Set
> > first menu entry +  setenv bootmenu_1 Boot 2. kernel=bootm
> > 0x83000000  # Set second menu entry +  setenv bootmenu_2
> > Reset board=reset                # Set third menu entry + 
> > setenv bootmenu_3 U-Boot boot order=boot           # Set
> > fourth menu entry +  setenv bootmenu_4  # Empty string is
> > end of all bootmenu entries +  bootmenu 20        # Run
> > bootmenu with autoboot delay 20s
> You might give an example here what the resulting screen will
> look like.
> 

Ok, I can here text output. If you want to see screenshot from 
bootmenu rendered to framebuffer (in qemu) here is: 
http://atrey.karlin.mff.cuni.cz/~pali/u-boot-bootmenu.png

> 
> > +  #define CONFIG_BOOTDELAY 30
> > +  #define CONFIG_AUTOBOOT_KEYED
> 
> Do we really need CONFIG_AUTOBOOT_KEYED ?
> 

Yes, because menu_show() call in common/main.c is called only of 
CONFIG_AUTOBOOT_KEYED is defined. You can look at this condition.

-- 
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121114/43f0332a/attachment.pgp>