[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading |
Date: |
Thu, 7 Apr 2011 10:38:48 +0200 |
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
- [Qemu-devel] [PATCH] hw: improve multiboot module loading, ralf, 2011/04/06
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Alexander Graf, 2011/04/06
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Ralf Ramsauer, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading,
Alexander Graf <=
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Ralf Ramsauer, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Alexander Graf, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Ralf Ramsauer, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Stefan Hajnoczi, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Ralf Ramsauer, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Stefan Hajnoczi, 2011/04/07
- Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Adam Lackorzynski, 2011/04/07
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Brad Hards, 2011/04/06
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading, Stefan Hajnoczi, 2011/04/07