Re: [Qemu-devel] [PATCH v1 00/23] QAPI Infrastructure Round 1

[Luiz Capitulino]

On Wed, 18 May 2011 11:33:20 -0500
Michael Roth <address@hidden> wrote:

> On 05/18/2011 10:10 AM, Luiz Capitulino wrote:
> > On Wed, 18 May 2011 09:43:37 -0500
> > Michael Roth<address@hidden>  wrote:
> >
> >> On 05/18/2011 08:46 AM, Luiz Capitulino wrote:
> >>> On Tue, 17 May 2011 19:51:47 -0500
> >>> Michael Roth<address@hidden>   wrote:
> >>>
> >>>> These apply on top of master, and can also be obtained from:
> >>>> git://repo.or.cz/qemu/mdroth.git qapi_round1_v1
> >>>
> >>> Nice to see this moving forward.
> >>>
> >>>> These patches are a backport of some of the QAPI-related work from 
> >>>> Anthony's
> >>>> glib tree. The main goal is to get the basic code generation 
> >>>> infrastructure in
> >>>> place so that it can be used by the guest agent to implement a QMP-like 
> >>>> guest
> >>>> interface, and so that future work regarding the QMP conversion to QAPI 
> >>>> can be
> >>>> decoupled from the infrastructure bits.
> >>>>
> >>>> Round1 incorporates the following components from Anthony's tree:
> >>>>
> >>>>    - Pulls in GLib libraries (core GLib, GThreads, and GIO)
> >>>>
> >>>>    - Adds code to do exception-like error propagation
> >>>>
> >>>>    - New error reporting functions
> >>>>
> >>>>    - Schema-based code generation for QAPI types and synchronous QMP 
> >>>> commands
> >>>>      using visiter patterns to cut reduce the amount of code generated 
> >>>> by the
> >>>>      previous scripts. This is just infrastructure, QMP will remain 
> >>>> untouched
> >>>>      until the actual conversion efforts are underway. Only a set of 
> >>>> unit tests
> >>>>      and, in the guest, virtagent, will utilize this infrastructure 
> >>>> initially.
> >>>
> >>> This series introduces quite a lot of infrastructure w/o adding a single 
> >>> real
> >>> user. This has some disadvantages, the most important one being that we 
> >>> can't
> >>> test it for real (unit-tests are important, but don't replace real usage).
> >>> Another disadvantage is that, reviewers don't actually see how this is 
> >>> going to
> >>> be used and can't comment on API level improvements/bugs.
> >>
> >> The guest agent user will mirror the QMP user pretty closely, but I
> >> could see why it'd be nice to have an actual QMP user as part of the
> >> series. I think we decided on IRC that an incremental QMP conversion
> >> wouldn't be the best route and should instead be done as part of a
> >> single concerted effort. So one approach I would propose is to have
> >> example conversions tacked on to the end of this series.
> >
> > Yes, that would be good.
> >
> >> So for this series we'd have 1 or 2 example conversions for synchronous
> >> QMP functions. Future infrastructure patches could provide examples for
> >> async QMP/proxied QMP/QMP event/qcfg/etc users as the relevant
> >> infrastructure bits are added.
> >
> > I think the examples have to use all the added infrastructure. For example,
> > if you're not adding async commands, then we'd have to drop the async 
> > support
> > from the series.
> 
> Yup I agree. I actually tried to cull some of the async/proxy stuff from 
> this series but there were some hooks and code gen bits I left in. I'll 
> clean it up a bit better for the next submission.
> 
> >
> > I'm tempted to say that we should try to reduce the code generator (and all
> > the infrastructure around it) to generated only the bits that are going to 
> > be
> > used by the examples. But I'm not sure if the work involved is worth it.
> >
> 
> It wouldn't be too bad for this submission, far as I can tell. 
> Generation of async QMP commands and event types are the main thing 
> ones. The main complication is losing work from the glib tree if we're 
> not careful, since the initial commits were pretty much the whole 
> shebang. But it shouldn't be too difficult to piece things back together 
> as we go.

Nice.

> >> So long as the example conversions capture the general use cases, we'd
> >> still be able to decouple conversion efforts from infrastructure (with
> >> any corner cases fixed as a part of those efforts), while allowing the
> >> infrastructure code to be reviewed in the proper context.
> >
> > Yes.
> >
> >>> I prefer an incremental approach. We could try to split this series in 
> >>> smaller
> >>> parts and change current QMP to use that parts. This will make review 
> >>> easier
> >>> and will make it possible to do incremental testing too.
> >>>
> >>
> >> I could split the code conversion stuff out into a separate series. So
> >> we'd have:
> >
> > Looks good to me.
> >
> >> Round 1: error-related changes
> >
> > I'm already taking care of this one. I hope to have patches soon. The 
> > problem
> > here is that I'm very serious about testing and am going to test each
> > converted handler. Unfortunately, most of the testing is done by hand today 
> > :(
> > but kvm-autotest has some support for testing error paths and libvirt has a
> > more general suite too.
> >
> 
> Ok, cool. A pretty good swath of the error stuff is needed to avoid 
> breakage for Round 2/3, but if you can point me to a repo I can base 
> this series on that and send you patches for error-related dependencies.

That's my repo:

 git://repo.or.cz/qemu/qmp-unstable.git qapi-qerror-v1

But be aware that I rebase it often.

> 
> >> Round 2: json-related changes
> >
> > I think I saw patches flying on the list, did you submit then? Do they
> > depend on the error stuff?
> >
> 
> That was probably a pull request I sent a week or so ago to Anthony for 
> the glib tree. I think they got lost in the noise of all this reworking. 
> I'd also been carrying them in my virtagent series for a while. Round 2 
> would have those as well as the ones in the glib tree. I'll make sure to 
> give those a good bit of testing.
> 
> Some of them do make use of error propagation, but that's it as far as I 
> can tell.
> 
> >> Round 3: code conversion infra + examples
> >>
> >> If we take the approach mentioned above, anyway.
> >>
> >> Otherwise I don't see how we could decouple any QMP conversion efforts
> >> from infrastructure (which I think was considered desirable). In terms
> >> of the code generation it's basically all or nothing, with the exception
> >> of the unit tests we've added. Did you have something else in mind?
> >
> > Your plan looks good to me. I mean, maybe it' me who's is still catching up
> > with all that stuff and want it to go slower so that I can fully absorb it
> > and try to make sure it won't break anything before it's merged.
> >
> > On the other hand, we might want to discuss errors separately for example,
> > as it's not specified to QMP.
> >
> 
> Yah, hopefully the proposed Rounds are granular enough that everyone can 
> absorb what's going on. Stefan H. also suggested adding some 
> documentation for schema/code generation usage, which might help in that 
> department as well. I'll try to get that included.
>