Message ID | 1338858018-17189-12-git-send-email-mdroth@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Am 05.06.2012 03:00, schrieb Michael Roth: > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > 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? Kevin
On 06/05/2012 04:00 AM, Michael Roth wrote: > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qidl-generated/mc146818rtc.json | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > create mode 100644 qidl-generated/mc146818rtc.json > > diff --git a/qidl-generated/mc146818rtc.json b/qidl-generated/mc146818rtc.json > new file mode 100644 > index 0000000..3be6fb6 > --- /dev/null > +++ b/qidl-generated/mc146818rtc.json > @@ -0,0 +1 @@ > +{'type': 'RTCState', 'data': {'cmos_data': {'type': ['uint8'], '<annotated>': 'true', 'array_size': '128'}, 'cmos_index': 'uint8', 'current_tm': 'tm', 'base_year': 'int32', 'periodic_timer': 'QEMUTimer', 'next_periodic_time': 'int64', 'next_second_time': 'int64', 'irq_coalesced': 'uint32', 'period': 'uint32', 'second_timer': 'QEMUTimer', 'second_timer2': 'QEMUTimer'}} Why are we committing generated files?
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 <mdroth@linux.vnet.ibm.com> > > --- > > 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. 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. 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. 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. It is ugly though... and I'm certainly open to alternative approaches. > > Kevin >
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 <mdroth@linux.vnet.ibm.com> >>> --- >>> 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? > 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? > 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? Kevin
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 <mdroth@linux.vnet.ibm.com> > >>> --- > >>> 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 >
diff --git a/qidl-generated/mc146818rtc.json b/qidl-generated/mc146818rtc.json new file mode 100644 index 0000000..3be6fb6 --- /dev/null +++ b/qidl-generated/mc146818rtc.json @@ -0,0 +1 @@ +{'type': 'RTCState', 'data': {'cmos_data': {'type': ['uint8'], '<annotated>': 'true', 'array_size': '128'}, 'cmos_index': 'uint8', 'current_tm': 'tm', 'base_year': 'int32', 'periodic_timer': 'QEMUTimer', 'next_periodic_time': 'int64', 'next_second_time': 'int64', 'irq_coalesced': 'uint32', 'period': 'uint32', 'second_timer': 'QEMUTimer', 'second_timer2': 'QEMUTimer'}}
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- qidl-generated/mc146818rtc.json | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) create mode 100644 qidl-generated/mc146818rtc.json