Re: [Qemu-devel] [PATCH 07/11] qmp: Wean off qerror_report()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/13/2015 08:20 AM, Markus Armbruster wrote:
>> The traditional QMP command handler interface
>> 
>>     int qmp_FOO(Monitor *mon, const QDict *params, QObject **ret_data);
>> 
>> doesn't provide for returning an Error object.  Instead, the handler
>> is expected to stash it in the monitor with qerror_report().
>> 
>> When we rebased QMP on top of QAPI, we didn't change this interface.
>> Instead, commit 776574d introduced "middle mode" as a temporary aid
>> for converting existing QMP commands to QAPI one by one.  More than
>> three years later, we're still using it.
>> 
>> Middle mode has two effects:
>
> Are these effects documented anywhere, as in docs/qapi-code-gen.txt?
> Should they be, or are we better off trying to get rid of middle mode?

The latter.

>> * Instead of the native input marshallers
>> 
>>       static void qmp_marshal_input_FOO(QDict *, QObject **, Error **)
>> 
>>   it generates input marshallers conforming to the traditional QMP
>>   command handler interface.
>> 
>> * It suppresses generation of code to register them with
>>   qmp_register_command()
>> 
>>   This permits giving them internal linkage.
>> 
>> As long as we need qmp-commands.hx, we can't use the registry behind
>> qmp_register_command(), so the latter has to stay for now.
>
> Is there a qapi marking we can add for this effect, similar to
> 'gen':false?  For that matter...
>
>> 
>> The former has to go to get rid of qerror_report().  Changing all QMP
>> commands to fit the QAPI mold in one go was impractical back when we
>> started, but by now there are just a few stragglers left:
>> do_qmp_capabilities(), qmp_qom_set(), qmp_qom_get(), qmp_object_add(),
>> qmp_netdev_add(), do_device_add().
>
> ...many of these are already using 'gen':false!

Using qmp_register_command() for some commands and qmp-commands.hx for
others doesn't sound appealing.

The best way forward is getting rid of qmp-commands.hx.  Not in this
development cycle, though.

>> Switch middle mode to generate native input marshallers, and adapt the
>> stragglers.  Simplifies both the monitor code and the stragglers.
>> 
>> Rename do_qmp_capabilities() to qmp_capabilities(), and
>> do_device_add() to qmp_device_add, because that's how QMP command
>> handlers are named today.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hmp.c                     |  5 ++++-
>>  include/monitor/monitor.h |  7 +++---
>>  include/monitor/qdev.h    |  3 ++-
>>  include/net/net.h         |  2 +-
>>  monitor.c                 | 24 ++++++---------------
>>  net/net.c                 | 16 ++++++--------
>>  qdev-monitor.c            | 15 ++++++-------
>>  qmp-commands.hx           |  4 ++--
>>  qmp.c | 55 +++++++++++------------------------------------
>>  scripts/qapi-commands.py  | 41 ++++++-----------------------------
>>  util/qemu-error.c         |  4 ++--
>>  11 files changed, 50 insertions(+), 126 deletions(-)
>
> Relatively nice diffstat.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!