[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