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

[Michael Roth]

On Thu, 29 Sep 2011 10:52:30 -0300, Luiz Capitulino <address@hidden> wrote:
> On Thu, 29 Sep 2011 07:55:37 -0500
> Anthony Liguori <address@hidden> 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.

> 
>  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.

> 
>  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!
> 
> > 
> > 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(-)
> > >
> > >
> > 
> 

-- 
Sincerely,
Mike Roth
IBM Linux Technology Center