port-i386/42599: Multiboot(8) command-line format/parsing

[gregoire.sutre%labri.fr@localhost]

>Number:         42599
>Category:       port-i386
>Synopsis:       Multiboot(8) command-line format/parsing problem with GRUB 2
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    port-i386-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Jan 09 18:55:01 +0000 2010
>Originator:     Grégoire Sutre
>Release:        NetBSD 5.0_STABLE (20091024)
>Organization:
>Environment:
System: NetBSD niagara 5.0_STABLE NetBSD 5.0_STABLE (NIAGARA) #0: Sat Oct 24 
11:54:39 CEST 2009 
instsoft@niagara:/usr/build/usr/src/sys/arch/i386/compile/NIAGARA i386
Architecture: i386
Machine: i386
>Description:
I've been experimenting with multibooting NetBSD/i386 (5.0_STABLE) from 
GRUB 2, and I had some problems passing the right arguments on the 
multiboot command line.  I thought this list might be a good place to 
report them, especially since the problems are not so specific to GRUB 2.

This started with the fact that, in GRUB 2, the command-line passed to 
the kernel does not contain the kernel itself, i.e. with the command

multiboot (hd0,1,a)/netbsd -z root=wd0a

the command-line passed to the kernel is "-z root=wd0a".  The option 
root=wd0a is detected ok by the kernel, but -z is ignored.  Looking at 
sys/arch/i386/i386/multiboot.c  (the code is  very clear by the way :-)) 
revealed the reason: when parsing boothowto(9) options, the first 
argument of the multiboot command-line is assumed to be the kernel file 
name, and is thus ignored.  The other parsing functions do not make this 
assumption, for instance, the command line "root=wd0a -z" works fine.

If the multiboot kernel code expects the command-line to be in a 
specific format, then I would find it helpful to have the detailed 
format in the multiboot(8) man page.  As far as I understand the code, 
the command-line passed to the kernel (by the boot-loader) is expected 
to be of the form:

absolutefilename [-1234abcdmqsvxz] [extraopt=value ...]

An alternative solution would be to accept a more general format for the 
multiboot command-line, with no pre-supposed ordering of the arguments. 
  For instance `-zs root=wd0a /netbsd console=pc' would be accepted.  I 
believe that all functions in multiboot.c already authorize this, except 
setup_howto.  A small patch that allows this for setup_howto is 
attached.  The patch actually also accepts boothowto options that are 
not grouped together, e.g. `-z root=wd0a -s' but this can be removed.

The patch also removes an if test in the function setup_bootpath that 
checks whether Multiboot_Loader_Name is lexicographically greater than 
"GNU GRUB ".  I fail to see why this is required, and, moreover, this is 
specific to GRUB Legacy, since GRUB 2 identifies itself as "GRUB ". 
Maybe the intention here is to avoid setting an empty bootpath, but this 
could be checked by actually testing the guessed the bootpath before 
setting it.

Thanks for your time,

Grégoire Sutre

p.s. Regarding NetBSD's boot-loader (boot(8)), with `multiboot netbsd', 
the command-line passed to the kernel starts with `netbsd' instead of 
`/netbsd', which leads to btinfo_bootpath not being set in function 
setup_bootpath.  But I did not see the actual effect of that (except 
that the sysctl parameter `machdep.booted_kernel' is empty).
>How-To-Repeat:
Boot from GRUB 2 with a command of the form:
multiboot (hd0,1,a)/netbsd -z root=wd0a
and observe that `-z' is not taken into account.
>Fix:
Possible patch:
Index: multiboot.c
===================================================================
RCS file: /cvsroot/src/sys/arch/i386/i386/multiboot.c,v
retrieving revision 1.17
diff -C10 -a -u -p -r1.17 multiboot.c
--- multiboot.c 11 Oct 2008 11:06:19 -0000      1.17
+++ multiboot.c 9 Jan 2010 17:01:00 -0000
@@ -483,41 +483,38 @@ setup_bootdisk(struct multiboot_info *mi
  * structure does not provide this detail directly, so we try to derive
  * it from the command line setting.
  */
 static void
 setup_bootpath(struct multiboot_info *mi)
 {
        struct btinfo_bootpath bi;
        char *cl, *cl2, old;
        int len;
 
-       if (strncmp(Multiboot_Loader_Name, "GNU GRUB ",
-           sizeof(Multiboot_Loader_Name)) > 0) {
                cl = mi->mi_cmdline;
                while (*cl != '\0' && *cl != '/')
                        cl++;
                cl2 = cl;
                len = 0;
                while (*cl2 != '\0' && *cl2 != ' ') {
                        len++;
                        cl2++;
                }
 
                old = *cl2;
                *cl2 = '\0';
                memcpy(bi.bootpath, cl, MIN(sizeof(bi.bootpath), len));
                *cl2 = old;
                bi.bootpath[MIN(sizeof(bi.bootpath), len)] = '\0';
 
                bootinfo_add((struct btinfo_common *)&bi, BTINFO_BOOTPATH,
                    sizeof(struct btinfo_bootpath));
-       }
 }
 
 /* --------------------------------------------------------------------- */
 
 /*
  * Sets up the console bootinfo structure if the user gave a 'console'
  * argument on the boot command line.  The Multiboot information
  * structure gives no hint about this, so the only way to know where the
  * console is is to let the user specify it.
  *
@@ -583,39 +580,36 @@ setup_console(struct multiboot_info *mi)
 static void
 setup_howto(struct multiboot_info *mi)
 {
        char *cl;
 
        if (!(mi->mi_flags & MULTIBOOT_INFO_HAS_CMDLINE))
                return;
 
        cl = mi->mi_cmdline;
 
-       /* Skip kernel file name. */
-       while (*cl != '\0' && *cl != ' ')
-               cl++;
-       while (*cl != '\0' && *cl == ' ')
-               cl++;
-
        /* Check if there are flags and set 'howto' accordingly. */
-       if (*cl == '-') {
-               int howto = 0;
-
-               cl++;
-               while (*cl != '\0' && *cl != ' ') {
-                       BOOT_FLAG(*cl, howto);
+       while (*cl != '\0') {
+               /* Invariant: (cl == mi->mi_cmdline) or (*cl == ' '). */
+               while (*cl == ' ')
                        cl++;
-               }
-               if (*cl == ' ')
+               if (*cl == '-') {
                        cl++;
-
-               boothowto = howto;
+                       while (*cl != '\0' && *cl != ' ') {
+                               BOOT_FLAG(*cl, boothowto);
+                               cl++;
+                       }
+               }
+               else {
+                       while (*cl != '\0' && *cl != ' ')
+                               cl++;
+               }
        }
 }
 
 /* --------------------------------------------------------------------- */
 
 /*
  * Sets up the memmap bootinfo structure to describe available memory as
  * given by the BIOS.
  */
 static void