Re: [Qemu-devel] [PATCH v1 00/21]: First round of QAPI conversions

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 29 Sep 2011 15:15:17 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Thu, 29 Sep 2011 10:52:30 -0300, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > On Thu, 29 Sep 2011 07:55:37 -0500
> > Anthony Liguori <aliguori@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(-)
> > > >
> > > >
> > > 
> > 
>