Syslinux - Jun 2010 - [PATCH] chain.c32: add menu support to grub loader

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

will load GRUB's stage2 and pass the (absolute) path to the file
"grub.conf" in the Syslinux filesystem to GRUB's stage2. The path
is
passed to stage2 in GRUB's device/partition syntax (eg,
"(hd0,1)/foo").
Note that we don't check whether the menu file actually exists in the
Syslinux filesystem. GRUB will do the right thing if the file doesn't
exist and will just fall back to its prompt.

(This patch pushes the usage string over the 80 characters border, which
might not be acceptable.)

Signed-off-by: Paul Bolle <pebolle at tiscali.nl>
---
 com32/modules/chain.c |   58 ++++++++++++++++++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/com32/modules/chain.c b/com32/modules/chain.c
index 4f5baf1..a386502 100644
--- a/com32/modules/chain.c
+++ b/com32/modules/chain.c
@@ -76,7 +76,7 @@
  *	equivalent to seg=0x70 file=<loader> sethidden,
  *	used with DOS' io.sys.
  *
- * grub=<loader>
+ * grub=<loader>[,<menu>]
  *	same as seg=0x800 file=<loader> & jumping to seg 0x820,
  *	used with GRUB stage2 files.
  *
@@ -124,6 +124,7 @@ static struct options {
     bool isolinux;
     bool cmldr;
     bool grub;
+    char *grubmenu;
     bool grldr;
     bool swap;
     bool hide;
@@ -1269,20 +1270,20 @@ Usage:   chain.c32 [options]\n\
          chain.c32 label:<label> [<partition>] [options]\n\
          chain.c32 boot [<partition>] [options]\n\
          chain.c32 fs [options]\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\
-         cmldr=<loader>     Load Recovery Console of Windows
NT/2K/XP/2003\n\
-         freedos=<loader>   Load FreeDOS KERNEL.SYS\n\
-         msdos=<loader>     Load MS-DOS IO.SYS\n\
-         pcdos=<loader>     Load PC-DOS IBMBIO.COM\n\
-         grub=<loader>      Load GRUB stage2\n\
-         grldr=<loader>     Load GRUB4DOS grldr\n\
-         seg=<segment>      Jump to <seg>:0000, instead of
0000:7C00\n\
-         swap               Swap drive numbers, if bootdisk is not fd0/hd0\n\
-         hide               Hide primary partitions, except selected
partition\n\
-         sethidden          Set the FAT/NTFS hidden sectors field\n\
-         keeppxe            Keep the PXE and UNDI stacks in memory
(PXELINUX)\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\
+         cmldr=<loader>          Load Recovery Console of Windows
NT/2K/XP/2003\n\
+         freedos=<loader>        Load FreeDOS KERNEL.SYS\n\
+         msdos=<loader>          Load MS-DOS IO.SYS\n\
+         pcdos=<loader>          Load PC-DOS IBMBIO.COM\n\
+         grub=<loader>[,<menu>]  Load GRUB stage2, using (optional)
<menu> file\n\
+         grldr=<loader>          Load GRUB4DOS grldr\n\
+         seg=<segment>           Jump to <seg>:0000, instead of
0000:7C00\n\
+         swap                    Swap drive numbers, if bootdisk is not
fd0/hd0\n\
+         hide                    Hide primary partitions, except selected
partition\n\
+         sethidden               Set the FAT/NTFS hidden sectors field\n\
+         keeppxe                 Keep the PXE and UNDI stacks in memory
(PXELINUX)\n\
 See syslinux/com32/modules/chain.c for more information\n";
     error(usage);
 }
@@ -1349,6 +1350,11 @@ int main(int argc, char *argv[])
 	    opt.seg = 0x800;	/* stage2 wants this address */
 	    opt.loadfile = argv[i] + 5;
 	    opt.grub = true;
+	    opt.grubmenu = strchr(opt.loadfile, ',');
+	    if (opt.grubmenu) {
+		*opt.grubmenu = '\0';
+		opt.grubmenu += 1;
+	    }
 	} else if (!strncmp(argv[i], "grldr=", 6)) {
 	    opt.loadfile = argv[i] + 6;
 	    opt.grldr = true;
@@ -1583,8 +1589,30 @@ int main(int argc, char *argv[])
 	}
 
 	if (opt.grub) {
+	    char *path;
 	    regs.ip = 0x200;	/* jump 0x200 bytes into the loadfile */
 
+	    /* 89 bytes reserved for menu at offset 0x217 */
+	    path = zalloc(89);
+
+	    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);
+		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*/
+	    }
+
 	    /* GRUB's stage2 wants the partition number in the install_partition
 	     * variable, located at memory address 0x8208.
 	     * We only need to change the value of memory address 0x820a too:
-- 
1.7.1

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

Paul Bolle wrote:
> Allow the grub loader to (optionally) support using a GRUB menu file.
> For example
>     chain fs grub=stage2,grub.conf
>
> will load GRUB's stage2 and pass the (absolute) path to the file
> "grub.conf" in the Syslinux filesystem to GRUB's stage2. The
path is
> passed to stage2 in GRUB's device/partition syntax (eg,
"(hd0,1)/foo").
> Note that we don't check whether the menu file actually exists in the
> Syslinux filesystem. GRUB will do the right thing if the file doesn't
> exist and will just fall back to its prompt.
> +	    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);
> +		len = strlen(path);
> +		strncpy (path + len, opt.grubmenu, 89 - len);
Why do you add (drivenumber,partition) to the config file name?

When the user adds the drivenumber and partition to its config filename
itself:
  chain.c32 hd0,3 grub=/boot/grub/stage2,(hd1,10)/anotherconf.lst

you get a "bad" configfilename: (hd0,2)/(hd1,10)/anotherconf.lst

In my patch, grub will search on the correct drive and partition
automatically, when you just use /anotherconf.lst.
GRUB just searches its config file on the drive (passed via DL) and
partition (install_partition). You don't need to add the right drive and
partition to the config filename yourself.

- Gert Hulselmans