Re: [PATCH 1/2] keyval: accept escaped commas in implied option

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This is used with the weirdly-named device "SUNFD,two", so accepting it

"SUNW,fdtwo"

> is also a preparatory step towards keyval-ifying -device and the
> device_add monitor command.

Not quite.  Device "SUNW,fdtwo" has user_creatable = false (it's a
sysbus device), and therefore isn't available with -device or
device_add.

There are more device names containing comma, but only one is available
with -device or device_add: xlnx,zynqmp-pmu-soc.  I doubt it makes sense
there.

Parameter values with comma need special treatment before and after the
patch.  Before the patch, you have to use the unsugared form and double
the commas.  After the patch, that still works, but you can now *also*
use the sugared form and double the commas.  Not much of an improvement,
I'm afraid.

Of course, we aren't really after improvement, we're after switching to
keyval_parse() with fewer compatiblity issues.  But this issue only
exists where values with comma are valid.

None of the QemuOpts I looked at take such values, except for -device
xlnx,zynqmp-pmu-soc, which I believe to be useless.  If an actual use
exists, I missed it.

Permit me to digress: we should kill the anti-social device names
regardless of this patch.

I want device names to conform to QAPI naming rules for enum values.  If
we ever do QAPI-schema-based device configuration, "driver" changes from
str to enum.  Even if we're ready to reject QAPI-schema-based device
configuration as impractical forever, there's still a consistency
argument: names of things should conform to a common set of rules.

This applies to all implied parameters with an emumeration-like value.
I'm not aware of other offenders, though.

>                              But in general it is an unexpected wart
> of the keyval syntax and 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 ''

The suboptimal error message is due to the way I coded the parser, not
due to the grammar.

> 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.

Yes, my parse errors are less than friendly.  Let's review some of them.
I'm testing with

    $ qemu-storage-daemon --nbd $ARG

because that one doesn't have an implied key, which permits slightly
simpler $ARG.

* Empty key

  --nbd ,

    master:       Invalid parameter ''
    your patch:   Expected parameter before ','

    Improvement.

  --nbd key=val,=,fob=

    master:       Invalid parameter ''
    your patch:   Expected parameter before '='

    Improvement, but which '='?  Possibly better:

                  Expected parameter before '=,fob='

* Empty key fragment

  --nbd key..=

    master:       Invalid parameter 'key..'
    your patch:   same

    Slightly better, I think:

                  Name or number expected after 'key.'

  --nbd .key=

    master:       Invalid parameter '..key'
    your patch:   same

    Better, I think:

                  Name expected before '..key'

  Odd: if I omit the '=', your patch's message often changes to

                  Expected '=' after parameter ...

  This means the parser reports a non-first syntax error.  Parsing
  smell, I'm afraid :)

* Invalid key fragment

  --nbd _=

    master:       Invalid parameter '_'
    your patch:   same

  --nbd key.1a.b=

    master:       Invalid parameter 'key.1a.b'
    your patch:   same

    Slightly better, I think:

                  'key.1a' is not a valid parameter name

  --ndb anti,,social,,key=

    master:       Expected '=' after parameter 'anti'
    your patch:   Invalid parameter 'anti,social,key'

    The new error message shows the *unescaped* string.  Okay.

> 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 believe there are two, maybe three reasons for this series:

1. Support ',' in values with an implicit keys.

2. Improve error reporting.

3. Maybe nicer code.

1. is a feature without a known use.  2. can be had with much less churn
(I'm ready to back that up with a patch).  Since I haven't looked at
PATCH 2, I'm reserving judgement on 3.