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

[Laszlo Ersek]

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


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


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'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().

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.

Laszlo