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

[Michael Roth]

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.

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

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