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

[Luiz Capitulino]

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 :)

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

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

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

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.

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

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

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

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