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

[Paolo Bonzini]

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

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.

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.