Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading

[Alexander Graf]

On 07.04.2011, at 07:56, Ralf Ramsauer wrote:

> Here the version with the correct coding style.

Phew - please keep the commit message intact. What we do to apply patches is 
that we simply directly take your patch into git using "git am". With a patch 
description like this, if anyone looks at "git blame" or "git log" later, will 
wonder what the rationale was behind the patch, as the description is basically 
missing.

Also, you need to put a line like this there:

   Signed-off-by: Ralf Humppa <address@hidden>

I would actually simply resend a patch with proper patch description and fixed 
braces (see next comment), but I can't, because the first patch you sent was 
already missing your name after the Signed-off-by, basically making the patch a 
legal grey zone. So please, resend it again with proper patch description, 
proper Signed-off-by line and the brace thing fixed.

Alternatively, I could rewrite the patch myself. But that would obviously rob 
you off your name in the commit log (due to the missing Signed-off-by), which I 
bet you wouldn't want :). The fame is all yours after all.

> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index 0d2bfb4..2380d5e 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -267,6 +267,11 @@ int load_multiboot(void *fw_cfg,
>             /* if a space comes after the module filename, treat everything
>                after that as parameters */
>             target_phys_addr_t c = mb_add_cmdline(&mbs, initrd_filename);
> +            /* Kill spaces at the beginning of the filename */

Please move your code before the comment above. This way, it's

 a) confusing to read
 b) the $0 argument passed to the guest is wrong, as the virtual command line 
that shows up in the guest multiboot descriptor tables doesn't get the spaces 
removed.

> +            while (*initrd_filename == ' ')
> +            {

Please do the brace in the same line as the while. This way it's not following 
CODING_STYLE.

Also, I just realized that you did miss another small part that needs change. A 
few lines above, there is code to interpret the modules right before that as 
well:

    if (initrd_filename) {
        const char *r = initrd_filename;
        mbs.mb_buf_size += strlen(r) + 1;
        mbs.mb_mods_avail = 1;
        while ((r = strchr(r, ','))) {
           mbs.mb_mods_avail++;
           r++;
        }
        mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
    }

This code would need some change as well, as now the mb_buf_size field is 
incorrect. It doesn't really hurt, but is bad style to not use the exact amount 
of memory we need to.



> +              initrd_filename++;
> +            }
>             if ((next_space = strchr(initrd_filename, ' ')))
>                 *next_space = '\0';
>             mb_debug("multiboot loading module: %s\n", initrd_filename);


I've also CC'ed Adam. He's been the most active person contributing to 
multiboot recently.


Alex