Patchwork hw: improve multiboot module loading

login
register
mail settings
Submitter ralf@humppa.name
Date April 7, 2011, 12:19 a.m.
Message ID <312edea8efadfde52c4c6d267c924cf2.squirrel@humppa.name>
Download mbox | patch
Permalink /patch/90116/
State New
Headers show

Comments

ralf@humppa.name - April 7, 2011, 12:19 a.m.
Multiboot modules couldn't be loaded when there are spaces between the
filename and ','. Those spaces can simply be killed.

Signed-off-by:
---
Alexander Graf - April 7, 2011, 12:31 a.m.
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
Brad Hards - April 7, 2011, 2 a.m.
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
Stefan Hajnoczi - April 7, 2011, 8:43 a.m.
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
ralf@humppa.name - April 7, 2011, 10:18 a.m.
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
>

Patch

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);