Re: [RFC] Don't pass filename in multiboot command line

[Robert Millan]

On Sat, Aug 01, 2009 at 03:23:04PM +0200, Vladimir 'phcoder' Serbinenko wrote:
> According to multiboot specification "It should be possible to write
> compliant boot loaders that load the OS image from a variety of
> sources, including floppy disk, hard disk, and across a network. "
> It implicitly says that kernel shouldn't care about filename used by
> bootloader. About commandline it's only said:
> "If bit 2 of the `flags' longword is set, the `cmdline' field is
> valid, and contains the physical address of the command line to be
> passed to the kernel. The command line is a normal C-style
> zero-terminated string"
> Currently grub however passes filename as first argument of command line.
> Felix Zielcke did a series of tests and it revealed that OSes
> subdivide into two categories:
> 1) Ones that don't care whether this argument is omitted altogether or
> not (e.g. the don't ignore second argument even if filename is
> omitted)
> 2) Solaris (for kernel name) and AROS (for module name) both assume
> that filename is the same under grub and OS. This assumption isn't
> necessarily true. These OSes are broken one way or another. If we omit
> the first argument user can workaround the problem by supplying
> kernel/module OS  image name as first argument of multiboot/module
> command line
> E.g.
> multiboot /RPOOL/opensolaris/@/platform/i86pc/kernel /platform/i86pc/kernel

I agree with this.  But please wait a few days to give everyone a chance
to read it.

Regarding the patch:

>    /* Figure out cmdline length.  */
> -  for (i = 0, cmdline_length = 0; i < argc; i++)
> +  for (i = 1, cmdline_length = 0; i < argc; i++)
>      cmdline_length += grub_strlen (argv[i]) + 1;
>  
> +  if (cmdline_length == 0)
> +    cmdline_length = 1;
> +
> 
>    boot_loader_name_length = sizeof(PACKAGE_STRING);
>  
>  #define cmdline_addr(x)              ((void *) ((x) + code_size))
> @@ -351,14 +354,16 @@ grub_multiboot (int argc, char *argv[])
>    if (! cmdline)
>      goto fail;
>  
> -  for (i = 0; i < argc; i++)
> +  for (i = 1; i < argc; i++)
>      {
>        p = grub_stpcpy (p, argv[i]);
>        *(p++) = ' ';
>      }
>  
>    /* Remove the space after the last word.  */
> -  *(--p) = '\0';
> +  if (p != cmdline)
> +    p--;
> +  *p = 0;
>  
>    mbi->flags |= MULTIBOOT_INFO_CMDLINE;
>    mbi->cmdline = (grub_uint32_t) cmdline_addr (grub_multiboot_payload_dest);
> @@ -422,9 +427,12 @@ grub_module  (int argc, char *argv[])
>        goto fail;
>      }
>  
> -  for (i = 0; i < argc; i++)
> +  for (i = 1; i < argc; i++)
>      len += grub_strlen (argv[i]) + 1;
>  
> +  if (len == 0)
> +    len = 1;
> +
>    cmdline = p = grub_malloc (len);
>    if (! cmdline)
>      goto fail;
> @@ -436,7 +444,9 @@ grub_module  (int argc, char *argv[])
>      }
>  
>    /* Remove the space after the last word.  */
> -  *(--p) = '\0';
> +  if (p != cmdline)
> +    p--;
> +  *p = '\0';

There's a much simpler way to address this.  Just add something like:

  cmdline_argv = argv + 1;
  cmdline_argc = argc - 1;

at the beginning, and then use cmdline_argv and cmdline_argc instead of
correcting the off-by-one every time.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."