[syslinux] [PATCH] chain.c32: add menu support to grub loader
Shao Miller
Shao.Miller at yrdsb.edu.on.ca
Wed Jun 30 12:43:04 PDT 2010
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
More information about the Syslinux
mailing list