From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50757) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4qNX-0002Bf-87 for qemu-devel@nongnu.org; Tue, 16 Jun 2015 08:54:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4qNV-00086s-1H for qemu-devel@nongnu.org; Tue, 16 Jun 2015 08:54:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4qNU-00086g-Qr for qemu-devel@nongnu.org; Tue, 16 Jun 2015 08:54:08 -0400 From: Markus Armbruster References: <1434205258-1932-1-git-send-email-armbru@redhat.com> <1434205258-1932-8-git-send-email-armbru@redhat.com> <557F3C04.4060508@redhat.com> Date: Tue, 16 Jun 2015 14:54:05 +0200 In-Reply-To: <557F3C04.4060508@redhat.com> (Eric Blake's message of "Mon, 15 Jun 2015 14:56:36 -0600") Message-ID: <87mvzzg6fm.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 07/11] qmp: Wean off qerror_report() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mdroth@linux.vnet.ibm.com Eric Blake 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 >> --- >> 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 Thanks!