Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> Hi,
>
> sorry for the late answer. I can only address the netdev_add /
> opts-visitor stuff now.
>
> On 02/14/13 17:36, Luiz Capitulino wrote:
>> On Thu, 14 Feb 2013 14:31:50 +0100
>> Markus Armbruster <address@hidden> wrote:
>>> Luiz Capitulino <address@hidden> writes:
>>>> On Thu, 14 Feb 2013 10:45:22 +0100
>>>> Markus Armbruster <address@hidden> wrote:
>
>>>>> netdev_add() uses QAPI wizardry to create the appropriate object from
>>>>> the QemuOpts.  The parsing is done by visitors.  Things get foggy for me
>>>>> from there on, but it looks like one of them, opts_type_size(), can
>>>>> parse size suffixes, which is inappropriate for QMP.  A quick test
>>>>> confirms that this is indeed possible:
>>>>>
>>>>>     {"execute": "netdev_add","arguments": {"type":"tap","id":"net.1",
>>>>>     "sndbuf": "8k" }}
>>>>>
>>>>> Sets NetdevTapOptions member sndbuf to 8192.
>>>>
>>>> Well, I've just tested this with commit 783e9b4, which is before
>>>> netdev_add conversion to the qapi, and the command above just works.
>>>>
>>>> Didn't check if sndbuf is actually set to 8192, but this shows that
>>>> we've always accepted such a command.
>>>
>>> Plausible.  Nevertheless, we really shouldn't.
>> 
>> Agreed. I would be willing to break compatibility to fix the suffix
>> part of the problem, but there's another issue: sndbuf shouldn't be
>> a string in QMP, and that's unfixable.
>
> The main purpose / requirement / guideline of the netdev_add &
> opts-visitor conversion was that the exact same command lines had to
> keep working. The primary source of input was the command line, ie.
> QemuOpts. The qapi-schema was absolutely bastardized for the task, with
> the single goal that the QemuOpts stuff *already gotten from the user*
> would be auto-parsed into C structs -- structs that were generated from
> the json. So, no QMP callers had been in anyone's mind as a priority;
> the qapi/visitor etc. scaffolding was retrofitted to QemuOpts.
>
> Please read the message on the main commit of the series:
>
>   http://git.qemu.org/?p=qemu.git;a=commitdiff;h=eb7ee2cb
>
> plus here's the blurb:
>
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-07/msg02102.html

I understand, and it makes sense.  The trouble is it has since bled into
QMP.  I'd like us to clean that up.

>>>>> Netdev could be done without this key=value business in the schema.  We
>>>>> have just a few backends, and they're well-known, so we could just
>>>>> collect them all in a union, like Gerd did for chardev backends.
>
> The -netdev option centers on [type=]discriminator, and it had to work
> as transparently as possible. I can't recall all the details any longer
> (luckily!), but I remember sweating bullets wrapping the visitor API
> around QemuOpts.
>
>>>> I honestly don't know if this is a good idea, but should be possible
>>>> to change it if you're willing to.
>>>
>>> chardev-add: the schema defines an object type for each backend
>>> (ChardevFile, ChardevSocket, ...), and collects them together in
>>> discriminated union ChardevBackend.  chardev_add takes that union.
>>> Thus, the schema accurately describes chardev-add's arguments, and the
>>> generated marshaller takes care of parsing chardev-add arguments into
>>> the appropriate objects.
>> 
>> Yes, it's a nice solution. I don't know why we didn't have that idea
>> when discussing netdev_add. Maybe we were biased by the old
>> implementation.
>> 
>>> netdev_add: the schema defines an object type for each backend
>>> (NetdevNoneOptions, NetdevUserOptions, ...).  netdev_add doesn't use
>>> them, but takes arbitrary parameters instead.  The connection is made in
>>> code, which stuffs the parameters in a QemuOpts (losing all JSON type
>>> information), then takes them out again to stuff them into the
>>> appropriate object type.  A job the marshaller should be able to do for
>>> us.
>>>
>>> For me, the way chardev-add works makes a whole lot more sense, and I
>>> think we should clean up netdev_add to work the same way.
>>> Unfortunately, I can't commit to this cleanup job right now.
>> 
>> Agreed, and I think we won't break compatibility by doing this
>> improvement.
>
> The most important things to compare are qemu_chr_new_from_opts() and
> net_client_init(), if my reading is correct. Each is the corresponding
> iteration callback for the list of chardev/netdev list of QemuOpts.
>
> (a) qemu_chr_new_from_opts() dives in, fishes out the discriminator
> manually -- "backend" is encoded as a C string literal, and there's a
> similar access to "mux" --, and looks up the appropriate open function
> based on backend (with linear search & strcmp()).
>
> No matter which open function we choose, each one is chock-full of
> qemu_opt_get_XXXX() calls with property names hard-coded as C string
> literals. *Killing this* was the exact purpose of opts-visitor. Cf.:
>
> (b) net_client_init() parses the QemuOpts object into a C struct, based
> on the schema, validating basic syntax simultaneously. Then
> net_client_init1(), rather than a linear, string-based search, uses the
> enum value ("kind") as the controlling expression of a switch statement,
> and as immediate offset into the array of function pointers.
>
> None of those init functions makes one qemu_opt_get_XXXX() call with a
> hard-coded property name; they all use *field names* and access the
> pre-parsed structs.

More readable, and the compiler can help more with typos.

> Honestly I don't know wheter opts-visitor was a good idea to begin with.
> I was asked to do it, so I tried my best.
>
> What is clear however is that opts-visitor is an utter failure -- people
> are not using it; probably because it's awkward or not flexible enough.
> If it had lived up to its promise, then we'd be discussing a rebase of
> chardev (and maybe even the recent multiqueue patches) *onto* opts-visitor.

I suspect it's not used more widely because:

* There's just one example (I think) for opts-visitors, and
  understanding how it works is anything but trivial.  Examples for
  doing it the old-fashioned way are all over the place, impossible to
  miss, and easy to copy.

* opts-visitors is overkill for simple cases.  Most cases are simple, or
  at least start simple.

> (I'm mentioning multiqueue specifically because of the get_fds()
> function introduced in commit 264986e2. That commit extends the schema
> with a field called "fds" and introduces manually parses it into a list
> with get_fds().

Clearly inappropriate for QMP.  Should have been caught in review.
Needs to be deprecated and replaced by an appropriate interface.

We suck.

> Let it suffice to mention that I was working very hard to implement
> repeating options in opts-visitor. The parsed output is a standard qapi
> list, ie. on the schema level. See again commit eb7ee2cb. In this case
> you'd just say "fd=X,fd=Y,fd=Z" rather than the new, custom-format
> QemuOpt "fds=X:Y:Z".
>
> I think this nicely underlines the failure of opts-visitor:
> - if get_fds() could have been equivalently implemented with a repeating
> option in the schema, then opts-visitor failed to get recognition (=
> useless work),
> - if opts-visitor had turned out inflexible to accomodate this use case,
> then it would have failed functionally (= useless work).)
>
> Failure is *not* sweet.

I suspect the real failure is in patch review.

We can't expect everyone to know every feature, such as repeating
options.  But we need to catch wheel reinventions in review.