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 14:31:50 +0100
Markus Armbruster <address@hidden> wrote:

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

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.

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

Unfortunately not.

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

Problem is, I don't know how to make it better if we want to. I think
we could use it for the old commands that depend on QemuOpts so that
we don't make conversions too hard, but we shouldn't use it for new
commands.

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

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.

But you don't have to do it right now, having this for 1.5 would be nice.

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

What would be nice is to have a list of bugs to fix if you're not
planning to fix them yourself. The problem I mention below is more
likely to be triggered with drive-mirror, I can fix that.

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

Regarding 1.4, yes. I thought you meant we should fix it for 1.4 and
be done with it. Sorry for that.