From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52261) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9Nfx-0005T5-40 for qemu-devel@nongnu.org; Thu, 29 Sep 2011 16:57:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R9Nfv-0003U0-2g for qemu-devel@nongnu.org; Thu, 29 Sep 2011 16:57:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28102) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9Nfu-0003RH-HW for qemu-devel@nongnu.org; Thu, 29 Sep 2011 16:57:47 -0400 Date: Thu, 29 Sep 2011 17:57:39 -0300 From: Luiz Capitulino Message-ID: <20110929175739.56f641c5@doriath> In-Reply-To: <0M2bxl-1Qqk6729i9-00rxvY@mrelay.perfora.net> References: <1317221085-5825-1-git-send-email-lcapitulino@redhat.com> <4E846AC9.7010801@us.ibm.com> <20110929105230.568c6dd4@doriath> <0M2bxl-1Qqk6729i9-00rxvY@mrelay.perfora.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 00/21]: First round of QAPI conversions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: kwolf@redhat.com, Anthony Liguori , qemu-devel@nongnu.org, armbru@redhat.com On Thu, 29 Sep 2011 15:15:17 -0500 Michael Roth wrote: > On Thu, 29 Sep 2011 10:52:30 -0300, Luiz Capitulino wrote: > > On Thu, 29 Sep 2011 07:55:37 -0500 > > Anthony Liguori wrote: > > > > > On 09/28/2011 09:44 AM, Luiz Capitulino wrote: > > > > This series is a bundle of three things: > > > > > > > > 1. Patches 01 to 04 add the middle mode feature to the current QMP server. > > > > That mode allows for the current server to support QAPI commands. The > > > > Original author is Anthony, you can find his original post here: > > > > > > > > http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg00374.html > > > > > > > > 2. Patches 05 to 10 are fixes from Anthony and Michael to the QAPI > > > > handling of the list type. > > > > > > > > 3. Patches 11 to 21 are simple monitor commands conversions to the QAPI. > > > > This is just a rebase of a previous conversion work by Anthony. > > > > > > Great series! > > > > > > Other than the one minor comment re: strdup and commit messages, I think it's > > > ready to go. > > > > Actually, I've found a few small problems with the enumeration in > > patch 14: > > > > 1. I'm not using VmRunStatus internally in QEMU, I'm using RunState (which > > has to be dropped, right? - Is VmRunStatus a good name btw?) > > Ideally, yes, especially since you're directly assigning enum values from > RunState to VmRunStatus. VmRunStatus seems reasonable to me, though if you call > it RunState you can just drop the previous declaration of RunState to convert > everythin over to using the schema definition. Yes, that's probably a good idea. I've called it VmRunStatus because RunState is a too generic name for a public API. > > > > > 2. RunState has a RSTATE_NO_STATE enum, which is used for initialization. To > > have that in VmRunStatus I had to add a 'no-status' value in the schema, > > is that ok? > > > Seems fine to me... the visitor routine assumes enumeration for schema-defined > enums starts at 0, so if that corresponds to RSTATE_NO_STATE you're fine. I've talked with Anthony about this and we decided to drop RSTATE_NO_STATE because it doesn't make sense to be part of the protocol. > > > > > 3. The code generator is replacing "-" by "_" (eg. 'no-status becomes > > 'no_status') but I have already fixed this and will include the patch > > in v2 > > Sounds good. > > > > > Also, it would be nice if Michael could review how I'm doing lists in > > patches 16 and 17. > > > > Sure, reviewed those and they look good. Also did some quick tests on all the converted commands and didn't notice any other issues. Thanks a lot! Will send v2 soon. > > > Thanks! > > > > > > > > Regards, > > > > > > Anthony Liguori > > > > > > > > > > > Makefile | 12 ++ > > > > Makefile.objs | 3 + > > > > Makefile.target | 6 +- > > > > error.c | 4 + > > > > hmp-commands.hx | 11 +- > > > > hmp.c | 116 ++++++++++++++++++ > > > > hmp.h | 31 +++++ > > > > monitor.c | 273 +++++-------------------------------------- > > > > qapi-schema.json | 273 +++++++++++++++++++++++++++++++++++++++++++ > > > > qapi/qapi-dealloc-visitor.c | 34 +++++- > > > > qapi/qapi-types-core.h | 3 + > > > > qapi/qmp-input-visitor.c | 4 +- > > > > qapi/qmp-output-visitor.c | 20 +++- > > > > qemu-char.c | 35 ++---- > > > > qerror.c | 33 +++++ > > > > qerror.h | 2 + > > > > qmp-commands.hx | 57 +++++++-- > > > > qmp.c | 92 +++++++++++++++ > > > > scripts/qapi-commands.py | 98 ++++++++++++--- > > > > scripts/qapi-types.py | 5 + > > > > scripts/qapi-visit.py | 4 +- > > > > scripts/qapi.py | 4 +- > > > > test-qmp-commands.c | 29 +++++ > > > > test-visitor.c | 48 +++++++-- > > > > vl.c | 12 ++ > > > > 25 files changed, 877 insertions(+), 332 deletions(-) > > > > > > > > > > > > > >