Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading
On Thu, Apr 07, 2011 at 12:19:01AM -, r...@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
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
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
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
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
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
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 -initrdmodule1 settings=use_foo,use_bar won't work! -- Ralf
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading
On Thu, Apr 7, 2011 at 1:56 PM, Ralf Ramsauer ralf.ramsa...@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 -initrdmodule1 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
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.ramsa...@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 -initrdmodule1 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 a...@os.inf.tu-dresden.de Lackorzynski http://os.inf.tu-dresden.de/~adam/
Re: [Qemu-devel] [PATCH] hw: improve multiboot module loading
On 07.04.2011, at 02:19, r...@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
On Thu, 7 Apr 2011 10:19:01 am r...@humppa.name wrote: Signed-off-by: Looks like something may be wrong with your git config Brad
