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

[Markus Armbruster]

[Some quoted material restored]

Luiz Capitulino <address@hidden> writes:

> On Thu, 14 Feb 2013 10:45:22 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> [Note cc: +Laszlo, +Anthony, -qemu-trivial]
>> 
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Fri, 08 Feb 2013 20:34:20 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> > The real problem here is that the k, M, G suffixes, for example, are not
>> >> > good to be reported by QMP. So maybe we should refactor the code in a 
>> >> > way
>> >> > that we separate what's done in QMP from what is done in
>> >> > HMP/command-line.
>> >> 
>> >> Isn't it separated already?  parse_option_size() is used when parsing
>> >> key=value,...  Such strings should not exist in QMP.  If they do, it's a
>> >> design bug.
>> >
>> > No and no. Such strings don't exist in QMP as far as can tell (see bugs
>> > below though), but parse_option_size() is theoretically "present" in a
>> > possible QMP call stack:
>> >
>> > qemu_opts_from_dict_1()
>> >   qemu_opt_set_err()
>> >     opt_set()
>> >       qemu_opt_paser()
>> >         parse_option_size()
>> >
>> > I can't tell if this will ever happen because qemu_opts_from_dict_1()
>> > restricts the call to qemu_opt_set_err() to certain values, but the
>> > fact that it's not clear is an indication that a better separation is
>> > necessary.
>> 
>> Permit me two detours.
>> 
>> One, qemu_opt_set_err() is a confusing name.  I doesn't set an error.
>
> It takes an Error ** object and it was introduced to avoid having
> to convert qemu_opt_set() to take an Error ** object, as this would
> multiply the work required to get netdev_add converted to the qapi.
>
> Now, I pass the bikeshed :)

I get why it's there, I just find its name confusing.

> [...]
>> Now, to build hmp_device_add() on top of qmp_device_add(), we'd have to
>> convert first from QemuOpts to QDict and then back to QemuOpts.  Blech.
>> Instead, we made do_device_add() go straight to qdev_device_add().  Same
>> for hmp_netdev_add(): it goes straight to netdev_add().
>
> Yes, the idea was to have netdev_add() and add frontends to hmp
> and qmp. More on this below.
>
> [...]
>> I guess weird things can happen with qemu_opts_from_qdict(), at least in
>> theory.
>> 
>> For each (key, value) in the QDict, qemu_opts_from_qdict() converts
>> value to string according to its qtype_code.  The string then gets
>> parsed according to key's QemuOptType.  Yes, that means you can pass a
>> QString to QEMU_OPT_SIZE option, and get the suffixes interpreted.
>> 
>> However, we've only used qemu_opts_from_qdict() with QemuOptsLists that
>> have empty desc[]!  Specifically: qemu_netdev_opts and qemu_device_opts.
>> Without desc, qemu_opt_parse() does nothing, and QemuOpts holds just
>> string values.  Actual parsing left to the consumer.
>> 
>> Two consumers: qdev_device_add() and netdev_add().
>> 
>> 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.

>> To sum up, we go from QDict delivered by the JSON parser via QemuOpts to
>> QAPI object, with a few cock-ups along the way.  Ugh.
>> 
>> By the way, the JSON schema reads
>> 
>>     { 'command': 'netdev_add',
>>       'data': {'type': 'str', 'id': 'str', '*props': '**'},
>>       'gen': 'no' }
>> 
>> I'll be hanged if I know what '**' means.
>
> For QMP on the wire it means that the command accepts a bunch of
> arguments not specified in the schema.

Thanks!  Is the schema language documented anywhere?

> Yes, it's a dirty trick. Long story short: we decide to maintain
> QemuOpts usage in both HMP and QMP to speed up netdev_add conversion.

I don't think '**' is as dirty as you make it sound.  It's simply a way
to relax the rigidity of the schema.  I got no problem with that.

>> qdev_device_add() uses a few well-known options itself, and they're all
>> strings.  The others it passes on to qdev_prop_parse().  This permits
>> some weirdness like passing PCI device's addr property as number in QMP,
>> even though it's nominally a string of the form SLOT[.FN].
>> 
>> No JSON schema, because device_add hasn't been qapified (Laszlo's
>> working on it now).
>> 
>> > Now, I think I've found at least two bugs. The first one is the
>> > netdev_add doc in the schema, which states that we do accept key=value
>> > strings. The problem is here is that that's about the C API, on the
>> > wire we act as before (ie. accepting each key as a separate argument).
>> > The qapi-schame.json is more or less format-independent, so I'm not
>> > exactly sure what's the best way to describe commands using QemuOpts
>> > the way QMP uses it.
>> 
>> 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.
>
> 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.

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.

>> Devices are hairier.  For a union approach, we'd have to include a
>> schema for every device model.  Dunno.
>
> IMHO, right now going fast is more important than doing things
> with perfection (unless you can do perfection in no time, of course),
> because the qapi conversions already took a lot more than expected
> and it's delaying very good stuff (like the new qmp server, and dropping
> the *.hx files and old QMP code).
>
> So, I wouldn't bother doing old crap commands perfect. Specially when
> replacements are on the way.

I'm not asking for perfection.  You wondered whether we can hit certain
errors with qemu_opts_from_qdict(), and I showed you how we can, and the
unintended misfeatures that make it possible.

As far as I can tell, we can hit them only with netdev_add, not with
device_add.  Happy to explain in more detail.

>> > The second bug is that I entirely ignored how set_option_paramater()
>> > handles errors when doing parse_option_size() conversion to Error **
>> > and also when converting bdrv_img_create(). The end result is that
>> > we can report an error twice, once with error_set() and later with
>> > qerror_report() (or vice-versa). Shouldn't hurt on QMP as it knows
>> > how to deal with this, on HMP and command-line we get complementary
>> > error messages if we're lucky.
>> 
>> Example?  Fixable?
>
> Of course it's fixable, everything is fixable :)
>
> qmp_drive_mirror()
>   bdrv_img()
>     set_option_paramater()
>     
>> > I'm very surprised with my mistakes on the second bug (although some
>> > of the mess with fprintf() was already there), but I honestly think we
>> > should defer this to 1.5 (and I can do it myself next week).
>> 
>> Stick a fork in 1.4, it's done.
>
> No, I honestly think that at this point in time we should be fixing bugs
> with proven end-user impact and serious regressions.

Note even that, it's *done*.  Finished.  Fertig.  Finito.  Game over;
insert coin to play again ;-P

> I consider bikeshedding and fixing small glitches present in more than
> one release to be an abuse for a hard-freeze.

Misunderstanding?