Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 27/11/2013 15:15, Markus Armbruster ha scritto:
>>>> >> This is unfortunately a counter-example to the rule that HMP commands
>>>> >> should always be implemented in terms of their QMP counterparts.  I do
>>>> >> not believe this is really a problem.  It can be fixed later; for now, I
>>>> >> think "perfect is the enemy of good" applies.
>> I don't get why there's a counter-example.
>
> Take as an example the current implementation of netdev_add.
>
> void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> {
>     ...
>     QemuOpts *opts = qemu_opts_from_qdict(qemu_find_opts("netdev"),
> qdict, &err);
>     netdev_add(opts, &err);
>     ...
> }
>
> int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
> {
>     opts_list = qemu_find_opts_err("netdev", &local_err);
>     opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
>     netdev_add(opts, &local_err);
>     ...
>     return 0;
>
> exit_err:
>     ...
> }
>
> These obey the rules you have below (there's just the weirdness that the QMP
> command handler is not autogenerated).
>
> But as you see above, this is really the same code for both handlers,
> they only differ in error handling minutiae.  This is possible with
> netdev_add (and you could do the same for device_add) because the QMP
> interface sucks and it is not well defined unless you make all values
> strings (qemu_opts_from_dict tries to do something meaningful for integers
> and floats, but given the existence of qdev property types such as hex32
> I wouldn't trust it too much).

netdev_add is a problematic example, because its a horrible, horrbible
QMP command.  I may say that, because I committed the crime :)

hmp_netdev_add() is the HMP command handler, and qmp_netdev_add() is the
QMP command handler.

Their error handling differs only because the former still implements
the legacy handler interface.

HMP's netdev_add parses arguments into a QemuOpts, exactly like -netdev.

I have to admit I can't tell offhand what the heck QMP's netdev_add
accepts, because I don't understand what "'*props:': '**'" means in
qapi-schema.json.  Even today, we permit code to serve as documentation
and specification for new features.  I wish we didn't.

Anyway, whatever it parses ends up in a QDict, as usual.

The part that sucks is the use of QemuOpts as netdev_add() parameter,

It made some sense when it was done, because then the command line was
the only user, its data type for option parameters was (and is)
QemuOpts, and QemuOpts was the least inconvenient way to do a function
that wants a a discriminated union of parameters, like netdev_add does.

It stopped making sense when we started using it from QMP, whose data
type for parameters is QDict.  I shoehorned netdev_add into QMP in its
early days.  Hinsight 20/20...

In my opinion, use of QemuOpts for anything but parsing parameter
strings has become a mistake.

Laszlo converted net.c to saner internal interfaces (commit
d195325..1a0c095).  Perhaps we can build on that.

> If we want a really different interface between HMP and QMP, one
> human-oriented and the other JSON-oriented, I'm not sure that you can
> share much code between the two implementation.

You should be able to share everything but the QMP marshalling, and the
HMP parsing and printing.

> Paolo
>
>> The rules are
>> 
>> 1. A QMP command handler (which needs to be of type int (*)(Monitor *,
>> const QDict *params, QObject **ret_data)) should be a thin wrapper
>> around a function with a type appropriate for C that contains all the
>> logic.  The wrapper merely translates.
>> 
>> 2. HMP commands are to be built on top of these functions, not their
>> handler wrappers.