Re: Multiboot(8) command-line format/parsing

[Grégoire Sutre <gregoire.sutre%labri.fr@localhost>]

Hi Julio,

> Can you please file a PR, include your patch in it, and assign it to
> me?  This will prevent this getting lost in the mailing list.

Thanks for your interest, and for NetBSD's multiboot code :-)

The PR is filed, it's port-i386/42599, but I don't know how to assign it.

> > thus ignored.  The other parsing functions do not make this assumption, for
> > instance, the command line "root=wd0a -z" works fine.
>
> Well, that shouldn't be allowed in my opinion, just as you cannot pass
> options in any order when using getopt() (despite what gnu getopt
> does).

Agreed.

> > absolutefilename [-1234abcdmqsvxz] [extraopt=value ...]
>
> Yup, that's correct.

I would find it helpful if an error message could be printed when the 
command-line has not the required format.  And maybe completely discard 
the command-line in that case?

Since the parsing of the command-line is done in different independent 
functions (setup_*), I guess that a syntax check of the command-line 
must be performed before calling these functions.

> > 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 looks really confusing; please not do that.  We should stick to
> the convention of executable, options and later arguments.

Ok.  This means that setup_bootpath should always use the first 
argument, right?  The code of setup_bootpath could then be simplified a 
bit by simply picking the first word.

> The non-grouping of options is a good thing, though, for consistency
> with getopt().

I also like it.  In my first attempts, it took me some time to realize 
that options did not work because they were not grouped...

> > 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.
>
> We should use this name difference to see if the command line contains
> the kernel name at the very beginning or not, and act accordingly.

Then I guess it's a typo in the code (but I may be wrong):

        if (strncmp(Multiboot_Loader_Name, "GNU GRUB ",
            sizeof(Multiboot_Loader_Name)) > 0) {

should be

        if (strncmp(Multiboot_Loader_Name, "GNU GRUB ", 9) == 0) {


But, still, if the specification (multiboot(8)) of the multiboot 
command-line for NetBSD says that it must start with the file name of 
the kernel, then boot-loaders are expected to comply.  So we could 
remove the test.  For instance NetBSD's boot(8) complies with this, but 
it does not seem to set Multiboot_Loader_Name, hence it won't pass the test.

Grégoire