On Thu, 29 Sep 2011 15:15:17 -0500 Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> On Thu, 29 Sep 2011 10:52:30 -0300, Luiz Capitulino <lcapitul...@redhat.com> > wrote: > > On Thu, 29 Sep 2011 07:55:37 -0500 > > Anthony Liguori <aligu...@us.ibm.com> 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(-) > > > > > > > > > > > > > >
