[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 00/21]: First round of QAPI conversions
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH v1 00/21]: First round of QAPI conversions |
Date: |
Thu, 29 Sep 2011 17:57:39 -0300 |
On Thu, 29 Sep 2011 15:15:17 -0500
Michael Roth <address@hidden> wrote:
> 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.
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(-)
> > > >
> > > >
> > >
> >
>
- Re: [Qemu-devel] [PATCH 14/21] qapi: Convert query-status, (continued)
- [Qemu-devel] [PATCH 18/21] qapi: Convert quit, Luiz Capitulino, 2011/09/28
- [Qemu-devel] [PATCH 15/21] qapi: Convert query-uuid, Luiz Capitulino, 2011/09/28
- [Qemu-devel] [PATCH 19/21] qapi: Convert stop, Luiz Capitulino, 2011/09/28
- [Qemu-devel] [PATCH 10/21] qapi: modify visitor code generation for list iteration, Luiz Capitulino, 2011/09/28
- [Qemu-devel] [PATCH 20/21] qapi: Convert system_reset, Luiz Capitulino, 2011/09/28
- Re: [Qemu-devel] [PATCH v1 00/21]: First round of QAPI conversions, Anthony Liguori, 2011/09/29