From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGRvS-00082h-Pa for qemu-devel@nongnu.org; Fri, 24 Jun 2016 10:17:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bGRvQ-0004cl-Me for qemu-devel@nongnu.org; Fri, 24 Jun 2016 10:17:41 -0400 Received: from mail-vk0-x231.google.com ([2607:f8b0:400c:c05::231]:36400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bGRvQ-0004ca-Hz for qemu-devel@nongnu.org; Fri, 24 Jun 2016 10:17:40 -0400 Received: by mail-vk0-x231.google.com with SMTP id u64so152251499vkf.3 for ; Fri, 24 Jun 2016 07:17:40 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <576CB7AC.90602@redhat.com> References: <20160623000809.4522-1-marcandre.lureau@redhat.com> <20160623000809.4522-6-marcandre.lureau@redhat.com> <576CB7AC.90602@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Fri, 24 Jun 2016 16:17:39 +0200 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 05/12] monitor: register the qapi generated commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: QEMU , Markus Armbruster On Fri, Jun 24, 2016 at 6:31 AM, Eric Blake wrote: >> + qcmd =3D qmp_find_command(cmd_name); > > Is it worth creating a single type that contains both mon_cmd_t and > QmpCommand information, so that we don't need two similarly-named > functions to look up two related pieces of information? Not necessarily > in this patch. Well, this is only temporary. Theuse 'qmp_dispatch()' get rid of all that. > >> + if (!qcmd || !cmd) { >> error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND, >> "The command %s has not been found", cmd_name); >> goto err_out; >> @@ -3931,7 +3933,7 @@ static void handle_qmp_command(JSONMessageParser *= parser, GQueue *tokens) >> goto err_out; >> } >> >> - cmd->mhandler.cmd_new(args, &data, &local_err); >> + qcmd->fn(args, &data, &local_err); >> >> err_out: >> monitor_protocol_emitter(mon, data, local_err); >> @@ -4000,9 +4002,13 @@ void monitor_resume(Monitor *mon) >> >> static QObject *get_qmp_greeting(void) >> { >> + QmpCommand *cmd; >> QObject *ver =3D NULL; >> >> - qmp_marshal_query_version(NULL, &ver, NULL); >> + cmd =3D qmp_find_command("query-version"); >> + assert(cmd && cmd->fn); >> + cmd->fn(NULL, &ver, NULL); >> + >> return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': []= }}",ver); > > Worth fixing the missing space after ',' while touching near here? fine by me ;) > >> } >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index ee88e48..95c1e7d 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -63,7 +63,6 @@ EQMP >> { >> .name =3D "quit", >> .args_type =3D "", >> - .mhandler.cmd_new =3D qmp_marshal_quit, > > At one point, I posted an RFC patch for removing .args_type on most QMP > command listings in this file. Maybe you still do that later in your > series, but as my work definitely conflicts with yours, and your series > is older, I don't mind getting through your series first. > Overall, I like the general idea of automating things rather than having > to duplicate information in qmp-commands.hx. Yeah, I basically get rid of qmp-commands.hx in my series (and the doc follow up in https://github.com/elmarco/qemu/commits/qapi-doc) --=20 Marc-Andr=C3=A9 Lureau