Re: [PATCH 04/25] keyval: accept escaped commas in implied option

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This is used with the weirdly-named device "SUNFD,fdtwo":

"SUNW,fdtwo"

Suggest "with weirdly-named devices such as "SUNW,fdtwo:", because we've
got more weirdos.

>   $ qemu-system-sparc -device SUNW,,fdtwo,help
>   SUNW,fdtwo options:
>     drive=<str>            - Node name or ID of a block device to use as a backend
>     fallback=<FdcDriveType> - FDC drive type, 144/288/120/none/auto (default: "144")
>     ...
>
> Therefore, accepting it is a preparatory step towards keyval-ifying
> -device and the device_add monitor command.

It's a preparatory step, but is it a necessary one?  More on that below.

>                                              In general, however, this
> unexpected wart of the keyval syntax leads to suboptimal errors compared
> to QemuOpts:
>
>   $ ./qemu-system-x86_64 -object foo,,bar,id=obj
>   qemu-system-x86_64: -object foo,,bar,id=obj: invalid object type: foo,bar
>   $ storage-daemon/qemu-storage-daemon --object foo,,bar,id=obj
>   qemu-storage-daemon: Invalid parameter ''

This is a second, independent argument supporting your patch.

As I remarked in reply to a prior post as "[PATCH 1/2] keyval: accept
escaped commas in implied option", the suboptimal errors could be
improved in a less invasive way.  Your way has a distinct advantage,
though: a working patch.

A third argument you've put forward elsewhere, but modestly left out
here: nicer code.  I'll get back to it after looking at the followup
cleanup in the next patch.

Either one argument could justify the patch, I think.

I'm this explicit to avoid the impression that the critique of the first
argument that comes next is me trying to find a reason to shoot down
your patch.

I don't think -device *needs* to accept anti-social device names.

We have a few devices with anti-social names, but none of them works
with -device, except in a help request.

We don't have to keep requests for human-readable help backwards
compible.

Anti-social device names are a usability issue with or without this
patch, with or without keyvalified -device.  The patch ensures the
sugared form of the help request continues to work after keyvalification
(the unsugared from is unaffected).  You could argue that loss of the
sugared form is a usability regression.  Maybe.  But usability is *poor*
in any case.  If we really cared for it, we'd get rid of the anti-social
names.

My point is: we're sitting in a hole, and the commit message starts with
"we need to dig a bit deeper to keep us comfortable".

My first preference: get rid of the anti-social names, drop the first
argument from the commit message, and let the patch rest on the other
two.

Second preference: rephrase the commit message along the lines of "This
is a step towards keyval-ifying -device without fixing the anti-social
device names first, and without breaking backward compatibility for help
requests".

> To implement this, the flow of the parser is changed to first unescape
> everything up to the next comma or equal sign.  This is done in a
> new function keyval_fetch_string for both the key and value part.
> Keys therefore are now parsed in unescaped form, but this makes no
> difference in practice because a comma is an invalid character for a
> QAPI name.  Thus keys with a comma in them are rejected anyway, as
> demonstrated by the new testcase.
>
> As a side effect of the new code, parse errors are slightly improved as
> well: "Invalid parameter ''" becomes "Expected parameter before '='"
> when keyval is fed a string starting with an equal sign.
>
> The slightly baroque interface of keyval_fetch_string lets me keep the
> key parsing loop mostly untouched.  It is simplified in the next patch,
> however.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I'll now look at the next patch, then get back to this one.