qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH for-1.4 v2 4/6] qemu-option: Disable two helpful messages that got broken recently
Date: Thu, 14 Feb 2013 10:45:22 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux)

[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 attempts to set an option value, using the conventional Error **
parameter for error reporting.  Its buddy qemu_opt_set() does the same,
except it reports errors to the user and returns only success/failure.

Two, qemu_opts_from_qdict() should be taken out and shot (I may say
that, because I created it).  I created it because I had to go from
QDicts I get from QMP to internal APIs that want QemuOpts, specifically
qdev_device_add().

qdev_device_add() takes QemuOpts because that's the only tool we had at
the time for passing dictionaries around.  Made enough sense then:
command line gives us QemuOpts already, so why not just pass them on.

Still made sense for the human monitor command: give it an argument just
like -device's option argument, parse it the same way, done.

It became awkward with QMP.  Perhaps I should've switched
qdev_device_add() to QDict then, so the QMP command handler can use it
directly.  Instead, I simply converted from QDict to QemuOpts.

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

QemuOpts is a bad fit for general interfaces, because it's really
designed for accumulating command line options for later use.  Features
useful there get in the way, e.g. how QemuOptsList serves both as schema
and as the single list of QemuOpts.  Features not needed there are
missing, e.g. signed and floating-point numbers.

End of detours, back to your questions.

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.

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.

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.

Devices are hairier.  For a union approach, we'd have to include a
schema for every device model.  Dunno.

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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]