| Message ID | 7f0b8ad36b8e8d1828f0dfc1803c0da0.squirrel@humppa.name |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
Out of curiousity, why are you trying to kill spaces at all? Why not just use a correct command-line to invoke QEMU? Stefan
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
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
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
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);