On Wed, Dec 14, 2016 at 04:38:07PM -0500, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Eduardo Habkost" <ehabk...@redhat.com> > > To: "Paolo Bonzini" <pbonz...@redhat.com> > > Cc: "Vlad Lungu" <vlad.lu...@windriver.com>, qemu-devel@nongnu.org, > > r...@twiddle.net > > Sent: Wednesday, December 14, 2016 6:51:44 PM > > Subject: Re: [PATCH] multiboot: copy the cmdline verbatim > > > > On Wed, Dec 14, 2016 at 06:20:46PM +0100, Paolo Bonzini wrote: > > > On 14/12/2016 18:00, Eduardo Habkost wrote: > > > > On Wed, Dec 14, 2016 at 05:55:07PM +0100, Paolo Bonzini wrote: > > > >> > > > >> > > > >> On 14/12/2016 17:19, Vlad Lungu wrote: > > > >>> get_opt_value() truncates the value at the first comma. > > > >>> Use memcpy() instead. > > > >> > > > >> Looks good since get_opt_value is already used by the caller. Have you > > > >> tested this with multiple initrd modules too? > > > > > > > > get_opt_value() is used by the caller, but with NULL buf > > > > argument. This means the caller doesn't handle ",," escaping. > > > > (See my reply to the previous submission of this patch) > > > > > > Hmm, wait. When NULL is passed, ",," escaping is handled correctly in > > > that next_initrd points to the next lone comma. The lone comma is > > > replaced with a '\0' by the caller. > > > > It is handled correctly when splitting the string, but not when opening the > > file. > > > > > > > > So you need to use get_opt_value again in mb_add_cmdline to do the > > > unescaping, because mb_add_cmdline only receives double commas. > > > > > > This was actually my first reaction to the patch, and it was correct. > > > Then I overthought it. :) > > > > > > So the patch is wrong. > > > > Except that the caller is already broken when using ",," for a > > different reason: it calls get_image_size(initrd_filename) and > > load_image(initrd_filename, ...) directly. So comma-escaping > > never worked anyway: > > > > $ qemu-system-x86_64 -kernel ~/Downloads/gnumach -initrd > > /tmp/one,,file,/tmp/another,,file > > Failed to open file '/tmp/one,,file' > > > > The right fix for comma-escaping would require calling > > get_opt_value() with non-NULL buf outside mb_add_cmdline(), > > because the mb_add_cmdline(&mbs, kcmdline) call do NOT need > > get_opt_value() to be called. > > For filenames containing commas you're right, but... > > > In other words: this fixes the mb_add_cmdline(kcmdline) case, and > > doesn't break comma escaping on the initrd case (because it was > > already broken). I don't see a problem with this patch. > > ... there is one case of comma escaping that wasn't broken: > > $ qemu-system-x86_64 -kernel foo -initrd '/tmp/one > arg,,with,,commas,/tmp/another arg,,with,,commas' >
Oh, I didn't notice the whitespace-based split for initrd arguments. This is messier than I thought. Maybe the simplest solution is to inline mb_add_cmdline() at both callers, and change the kcmdline one to use memcpy(). > And presumably this is what Vlad was trying to do. I believe he was trying to fix -append, not -initrd. The patch fixes -append, but breaks comma-escaping on -initrd. -- Eduardo
