Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc

[Michael Roth]

On Wed, Jun 06, 2012 at 09:38:24AM +0200, Kevin Wolf wrote:
> Am 05.06.2012 18:03, schrieb Michael Roth:
> > On Tue, Jun 05, 2012 at 11:29:24AM +0200, Kevin Wolf wrote:
> >> Am 05.06.2012 03:00, schrieb Michael Roth:
> >>> Signed-off-by: Michael Roth <address@hidden>
> >>> ---
> >>>  qidl-generated/mc146818rtc.json |    1 +
> >>>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>>  create mode 100644 qidl-generated/mc146818rtc.json
> >>
> >> I haven't looked at the Makefiles, but does this commit mean that the
> >> files aren't generated automatically but you have to run the generator
> >> manually after changing any device struct?
> > 
> > Nope, the files are automatically generated when changes are made to
> > QIDL sources and you do a build.
> > 
> > The reason they're still checked-in is so that changes to a device's
> > serialization schema can be "signed-off" by the author that made the
> > change. This applies to qidl-generated vmstate descriptions as well.
> 
> Doesn't really make sense to me. You already have a sign-off for the
> changed header/source file.
> 
> > It also makes an automated `make check-qidl` and, in the case of
> > qidl-generated vmstate descriptions, `make check-vmstate` possible, so
> > that a submitter/maintainer can detect and bring attention to changes to
> > serialized device state that need to be addressed/signed-off when
> > testing/reviewing patches.
> 
> Why can't 'make check-qidl' generate the new version itself like a
> simple 'make' would do?
> 

Because unless the generated code is checked in at some point, make
check-qidl doesn't have anything to check the newly-generated code against
to flag uncommitted/"undocumented" changes to the ABI.

> > We could get part of the way there by just keeping tabs on changes to qidl
> > sources, but ultimately how we do the serialization is a matter of how the
> > generated visitors look, in which case the generated QAPI schemas are the 
> > more
> > reliable representation. Annotations are hints, schemas are ABI, so
> > tracking the latter is more important.
> 
> So your statement is that the generator is likely buggy and therefore
> its output should be reviewed as well as the source changes?

The QIDL annotations are a tool/guideline for developers to ease the burdon
of having to think too much about the serialized data representation, but
they can still be misused or neglected, and those changes can still slip
by and break migration just as easily vmstate can be broken today.

Tracking changes to generated QAPI schemas is a way to draw attention to
such changes.

It's easy for some odd field to get a s/int/int64_t/ as
part of a substantial patch series and break compatability.

By committing the generated code, it's easy to detect when this happens,
easily note implications for migration compatability, then go back and review
those device state changes for correctness.

And keeping tabs on QAPI schemas changes for QMP/guest agent ABIs has
proven pretty successful, whereas keeping tabs on device state
changes hasn't.

> 
> > Similar rationale for vmstate: the relationship between annotations and
> > the generated vmstate descriptions isn't strong enough that we can
> > easily infer changes based on qidl annotations, and in many cases those
> > inferred changes will be overwritten by special handling in the vmstate
> > generator.
> 
> I don't understand. Is this file generated or manually edited? If the
> former, why does having it in the repository add anything new when you
> can (and with appropriate Makefile magic will) always run the generator
> after pulling changes to source files? If the latter, why does the
> subject say it's generated?

It's generated. The rationale is similar to the case for checking in
generated QAPI schemas: we track changes to the generated code to easily
infer the implications for migration on a patch-by-patch basis. It's
easilly to tell when changes have been made to a VMStateDescription in
qidl-generated/, much easier than spotting every extra field or type
change etc. to the various device state structures, especially given
that those are generally part of longer changesets whereas the files in
qidl-generated/ serve one well-known purpose.

> 
> Kevin
>