[syslinux] [PATCH] chain.c32: add menu support to grub loader

[Shao Miller]

A nice offering and please kindly accept the following code review.

Paul Bolle wrote:
> Allow the grub loader to (optionally) support using a GRUB menu file.
> For example
>     chain fs grub=stage2,grub.conf

Advantage: One cannot specify a GRUB config-file without specifying the 
'grub=' option.  Disadvantage: A lengthy path is silently truncated and 
used.  Even if a warning were issued, GRUB might clear the screen anyway. :S

> -Options: file=<loader>      Load and execute file, instead of boot sector\n\
> -         isolinux=<loader>  Load another version of ISOLINUX\n\
> -         ntldr=<loader>     Load Windows NTLDR, SETUPLDR.BIN or BOOTMGR\n\
> ...
> +Options: file=<loader>           Load and execute file, instead of boot sector\n\
> +         isolinux=<loader>       Load another version of ISOLINUX\n\
> +         ntldr=<loader>          Load Windows NTLDR, SETUPLDR.BIN or BOOTMGR\n\
> ...

Disadvantage: All option descriptions are drawn closer to the right, 
leaving the possibility of running past column 80.  I won't immediately 
tell if it would run past column 80 or not, since I do not trust patches 
inlined into e-mails to be mangle-free.  Good for easy viewing, though!

> +	    opt.grubmenu = strchr(opt.loadfile, ',');
> +	    if (opt.grubmenu) {
> +		*opt.grubmenu = '\0';
> +		opt.grubmenu += 1;
> +	    }

'opt.grubmenu++;' could be used as a convenience. :)

>  	if (opt.grub) {
> +	    char *path;

You could also work with the field in the stage2 binary directly.  You 
know there're 89 bytes there reserved for a config-file.  That would 
avoid the zalloc() and 'path &&' below.

>  	    regs.ip = 0x200;	/* jump 0x200 bytes into the loadfile */
>  
> +	    /* 89 bytes reserved for menu at offset 0x217 */
> +	    path = zalloc(89);

This zalloc()ation is never freed.  In current COMBOOT32, H. Peter says 
heaps are initialized by each invoked COMBOOT32 module and thus 
allocations do not persist past a module's exit.  I mention it as a 
habit worth keeping, however.

> +
> +	    if (path && opt.grubmenu) {
> +		char partition[5];	/* maximum is ",100" and up! */
> +		int len;
> +
> +		if (whichpart > 1)
> +		    sprintf(partition, ",%d", whichpart - 1);
> +		else
> +		    partition[0] = '\0';
> +		sprintf(path, "(%cd%d%s)/",
> +		        drive & 0x80 ? 'h': 'f',
> +		        (drive & 0x7),
> +			partition);

An alternative to these two sprintfs() would be two sprintfs(), one 
containing a comma, a '%d', and 'whichpart - 1', the other without.  
Also, was 'drive & 0x7' a typo for 'drive & 0x7F'?

> +		len = strlen(path);
> +		strncpy (path + len, opt.grubmenu, 89 - len);
> +		memcpy(data[ndata].data + 0x217, path, 89);
> +		memset(path + 88, '\0', 1);	/* to be safe*/

'memset(path + 88, '\0', 1);' could also be 'path[88] = 0;' or even 
'path[88] = '\0';'.

I notice that this patch does not include additional documentation for 
GRUB's stage2 field @ 0x208, which you had mentioned might be beneficial 
to include.  Why, such could even be somewhat "self-documented" if one 
made a 'struct xxx { ... } __attribute__((packed));' to represent what's 
at 0x200 and beyond in the stage2 binary.  Otherwise, '#define's or 
'enum { ... };'s can be nicer to read than more cryptic '0x217' and even 
'89'.  Personally, I enjoy using an array of 'char xxx[89];' somewhere 
and then using 'sizeof(xxx)' or 'sizeof(xxx) - 1' wherever I need to 
talk about its size.

Quite a co-incidence of timing where Gert H. submitted a similar patch 
nearly simultaneously!  It would be really great if chain.c32 got the 
best of both. :)

I don't speak on behalf of Syslinux; just my review.

- Shao Miller