----- 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'
And presumably this is what Vlad was trying to do.
Paolo
