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

Paul Bolle pebolle at tiscali.nl
Wed Jun 30 13:25:06 PDT 2010


On Wed, 2010-06-30 at 15:43 -0400, Shao Miller wrote:
> A nice offering and please kindly accept the following code review.

Thanks.

> 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

It might be better to return to the prompt. I hadn't thought about that
option. What's the best way to do that?

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

As I said in my commit message, the columns do run past column 80.
That's a no-no?

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

My inline patches should be acceptable to the git tools as is. If not,
I'll have to change the way I post patches.

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

Fine with me.

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

Seems a good idea. Thanks.

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

That might be a good idea too. 
  
> Also, was 'drive & 0x7' a typo for 'drive & 0x7F'?

No. "Legacy" GRUB doesn't use drive numbers 8 and up.

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

Either is fine with me. (Chances are compilers will generate identical
code. Or can't they?)

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

I still hope to do that.

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

I guess a few #define's might make the code more readable.

> Quite a co-incidence of timing where Gert H. submitted a similar patch 
> nearly simultaneously!

Quit a co-incidence indeed!

> It would be really great if chain.c32 got the 
> best of both. :)

I don't care whose code gets merged. Really, I don't. I do like to see
good code gets merged.


Paul




More information about the Syslinux mailing list