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

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

[ralf]

Multiboot modules couldn't be loaded when there are spaces between the
filename and ','. Those spaces can simply be killed.

Signed-off-by:
---
diff --git a/hw/multiboot.c b/hw/multiboot.c
index 0d2bfb4..27eb159 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -267,6 +267,9 @@ 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 */
+            while( *initrd_filename == ' ' )
+              initrd_filename++;
             if ((next_space = strchr(initrd_filename, ' ')))
                 *next_space = '\0';
             mb_debug("multiboot loading module: %s\n", initrd_filename);

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

[Alexander Graf]

On 07.04.2011, at 02:19, ralf@humppa.name wrote:

> Multiboot modules couldn't be loaded when there are spaces between the
> filename and ','. Those spaces can simply be killed.
> 
> Signed-off-by:
> ---
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index 0d2bfb4..27eb159 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -267,6 +267,9 @@ 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 */
> +            while( *initrd_filename == ' ' )
> +              initrd_filename++;

Please make sure to follow the coding style. There shouldn't be whitespace between the (), but between while and ( and you need some extra braces :):

while (*initrd_filename == ' ') {
    initrd_filename++;
}

Otherwise, the patch looks good! Please resend a corrected version, so that it can be easily applied.


Alex

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

[Brad Hards]

On Thu, 7 Apr 2011 10:19:01 am ralf@humppa.name wrote:
> Signed-off-by:
Looks like something may be wrong with your git config

Brad

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

[Ralf Ramsauer]

Here the version with the correct coding style.

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 */
+            while (*initrd_filename == ' ')
+            {
+              initrd_filename++;
+            }
             if ((next_space = strchr(initrd_filename, ' ')))
                 *next_space = '\0';
             mb_debug("multiboot loading module: %s\n", initrd_filename);

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 <ralf@humppa.name>

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

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

[Stefan Hajnoczi]

On Thu, Apr 07, 2011 at 12:19:01AM -0000, ralf@humppa.name wrote:
> @@ -267,6 +267,9 @@ 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 */
> +            while( *initrd_filename == ' ' )
> +              initrd_filename++;

If we want to do this, shouldn't it be done before adding to the
multiboot command-line?  Otherwise the multiboot image also has to strip
leading spaces.

Stefan

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

[Ralf Ramsauer]

Leading spaces mustn't be stripped! The commandline could look like:
qemu -kernel kern -initrd "1.mod arg=foo arg2=bar, 2.mod arg=asdf"
The problem is, that the string
"1.mod arg=foo arg2=bar, 2.mod arg=asdf"
is divided at the ','. So we have two string
"1.mod arg=foo arg2=bar" and
" 2.mod arg=asdf"
 ^ This space causes the error "Failed to get image size"
So just the spaces at the beginning of the file have to be stripped.
Spaces at the end could be important for the commandline. E.g.
-initrd "1.mod arg=argwithmanyspaces    , 2.mod"

--
Ralf

> If we want to do this, shouldn't it be done before adding to the
> multiboot command-line?  Otherwise the multiboot image also has to strip
> leading spaces.
>
> Stefan
>

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

[Ralf Ramsauer]

On 07.04.2011 um 10:38, Alexander Graf wrote:
> 
> 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.

You're right, I didn't look at this section. But now i had a deeper look into the code, and I noticed the the mb_buf size seems to be incorrect anyway.

Look at this line:

mbs.mb_buf_size += strlen(r) + 1;

If I start qemu with the option -initrd "mod1 arg=bla, mod2 arg=foo, mod3 arg=bar", then r contains
"mod1 arg=bla, mod2 arg=foo, mod3 arg=bar", so the commas and spaces after the comma are counted for the size of the multiboot command line.
Yes, this doesn't really hurt, but it's nevertheless not the exact amount of memory. I'll try to fix it.

--
Ralf

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

[Alexander Graf]

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

> On 07.04.2011 um 10:38, Alexander Graf wrote:
>> 
>> 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.
> 
> You're right, I didn't look at this section. But now i had a deeper look into the code, and I noticed the the mb_buf size seems to be incorrect anyway.
> 
> Look at this line:
> 
> mbs.mb_buf_size += strlen(r) + 1;
> 
> If I start qemu with the option -initrd "mod1 arg=bla, mod2 arg=foo, mod3 arg=bar", then r contains
> "mod1 arg=bla, mod2 arg=foo, mod3 arg=bar", so the commas and spaces after the comma are counted for the size of the multiboot command line.
> Yes, this doesn't really hurt, but it's nevertheless not the exact amount of memory. I'll try to fix it.

Good point. Thanks!


Alex

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

[Ralf Ramsauer]

Hello again,

there's another problem with parsing the initrd string:

If you want to start qemu with ... -initrd "mod1 arg=foo,bar , mod2 arg=bar,foo" it won't work
The string is separated at every comma, arguments containing a comma are misinterpreted.

So we'd think about another way delivering the arguments to load_multiboot.

-initrd is used passing multiboot modules, but multiboot modules aren't really initrd's

Multiboot modules are only loaded when qemu identifies the kernel as a Multiboot kernel.
It would be much easier to pass the modules to qemu using a commandline argument like -multiboot.
e.g.
qemu -kernel mymultibootkern -module "1.mod arg=bla" -module "2.mod arg=foo"

Well i didn't look at the global command line parsing of qemu, and i don't know how difficult it would be
to realize this. Anyone ideas?

--
Ralf

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

[Stefan Hajnoczi]

Out of curiousity, why are you trying to kill spaces at all?

Why not just use a correct command-line to invoke QEMU?

Stefan

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

[Ralf Ramsauer]

On 07.04.2011, at 14:48, Stefan Hajnoczi wrote:

> Out of curiousity, why are you trying to kill spaces at all?
> 
> Why not just use a correct command-line to invoke QEMU?
> 
> Stefan

Well it took me 2 days to find out why -initrd "module1, module2" didn't work. If there's a space after the comma you'll always
get the error message "Failed to get  image size". And if you want to pass a comma in a multiboot argument you've no way to do this.
So -initrd"module1 settings=use_foo,use_bar" won't work!

--
Ralf

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

[Stefan Hajnoczi]

On Thu, Apr 7, 2011 at 1:56 PM, Ralf Ramsauer
<ralf.ramsauer@googlemail.com> wrote:
> On 07.04.2011, at 14:48, Stefan Hajnoczi wrote:
>
>> Out of curiousity, why are you trying to kill spaces at all?
>>
>> Why not just use a correct command-line to invoke QEMU?
>>
>> Stefan
>
> Well it took me 2 days to find out why -initrd "module1, module2" didn't work. If there's a space after the comma you'll always
> get the error message "Failed to get  image size".

How about improving the error message?

> And if you want to pass a comma in a multiboot argument you've no way to do this.
> So -initrd"module1 settings=use_foo,use_bar" won't work!

>From what I can tell your patch does not change this.

Stefan

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

[Adam Lackorzynski]

On Thu Apr 07, 2011 at 14:52:34 +0100, Stefan Hajnoczi wrote:
> On Thu, Apr 7, 2011 at 1:56 PM, Ralf Ramsauer
> <ralf.ramsauer@googlemail.com> wrote:
> > On 07.04.2011, at 14:48, Stefan Hajnoczi wrote:
> >
> >> Out of curiousity, why are you trying to kill spaces at all?
> >>
> >> Why not just use a correct command-line to invoke QEMU?
> >>
> >> Stefan
> >
> > Well it took me 2 days to find out why -initrd "module1, module2" didn't work. If there's a space after the comma you'll always
> > get the error message "Failed to get  image size".
> 
> How about improving the error message?

I'll send a patch shortly fixing the message.
 
> > And if you want to pass a comma in a multiboot argument you've no way to do this.
> > So -initrd"module1 settings=use_foo,use_bar" won't work!
> 
> >From what I can tell your patch does not change this.

It should be possible to put commas on the mb command line.
Do we want to escape commas?



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/