| Message ID | 20220112102345.79395-1-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Add canary to VMSTATE_END_OF_LIST | expand |
On Wed, 12 Jan 2022 at 10:24, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions; > given that the current check is only for ->name being NULL, sometimes > we get unlucky and the code apparently works and no one spots the error. > > Explicitly add a flag, VMS_END that should be set, and assert it is > set during the traversal. Does 'make check' definitely do the traversal for all vmstate structs, or do we need to add a "sanity check them all on startup" bit of test code ? thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Wed, 12 Jan 2022 at 10:24, Dr. David Alan Gilbert (git) > <dgilbert@redhat.com> wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > We fairly regularly forget VMSTATE_END_OF_LIST markers off descriptions; > > given that the current check is only for ->name being NULL, sometimes > > we get unlucky and the code apparently works and no one spots the error. > > > > Explicitly add a flag, VMS_END that should be set, and assert it is > > set during the traversal. > > Does 'make check' definitely do the traversal for all vmstate > structs, or do we need to add a "sanity check them all on startup" > bit of test code ? Oh I doubt it does; some vmsd's are conditional on guest state, many are only on particular machine types. I think the closest we have to being able to walk the tree, is --dump-vmstate - although you need to call that for each machine type. (I forgot to add the canary check in the dump-vmstate code, I'll fix that). Dave > thanks > -- PMM >
On Wed, 12 Jan 2022 at 10:42, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > Does 'make check' definitely do the traversal for all vmstate > > structs, or do we need to add a "sanity check them all on startup" > > bit of test code ? > > Oh I doubt it does; some vmsd's are conditional on guest state, many are > only on particular machine types. > > I think the closest we have to being able to walk the tree, is > --dump-vmstate - although you need to call that for each machine type. > (I forgot to add the canary check in the dump-vmstate code, I'll fix > that). We could have vmstate_register_with_alias_id() iterate through and assert presence of the right terminator (probably only if qtest enabled, or some other suitable condition). Then the existing tests that do the basic "check we can instantiate every device and initialize every board model" would run that code and catch most missing terminator cases, I think. -- PMM
On Wed, Jan 12, 2022 at 10:56:07AM +0000, Peter Maydell wrote: > We could have vmstate_register_with_alias_id() iterate through > and assert presence of the right terminator (probably only if > qtest enabled, or some other suitable condition). Then the > existing tests that do the basic "check we can instantiate every > device and initialize every board model" would run that code > and catch most missing terminator cases, I think. Agreed. How about assert it even without qtest? We do tons of assertion for programming errors anyway in QEMU. Thanks,
On Thu, 13 Jan 2022 at 01:21, Peter Xu <peterx@redhat.com> wrote: > > On Wed, Jan 12, 2022 at 10:56:07AM +0000, Peter Maydell wrote: > > We could have vmstate_register_with_alias_id() iterate through > > and assert presence of the right terminator (probably only if > > qtest enabled, or some other suitable condition). Then the > > existing tests that do the basic "check we can instantiate every > > device and initialize every board model" would run that code > > and catch most missing terminator cases, I think. > > Agreed. How about assert it even without qtest? We do tons of assertion for > programming errors anyway in QEMU. I don't inherently object, but in this case to do the assertion we'd need to do a scan over the fields arrays which we wouldn't otherwise need to, so the cost of the assert is not simply the compare-and-branch but also the loop over the array. If that's not significant in terms of start-up time costs we can just go ahead and do it (which would be nicer for debugging and making it really obvious to people writing new devices) but my remark above was a gesture towards "maybe we need to not do it for normal startup".. -- PMM
On Thu, Jan 13, 2022 at 11:00:20AM +0000, Peter Maydell wrote: > On Thu, 13 Jan 2022 at 01:21, Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, Jan 12, 2022 at 10:56:07AM +0000, Peter Maydell wrote: > > > We could have vmstate_register_with_alias_id() iterate through > > > and assert presence of the right terminator (probably only if > > > qtest enabled, or some other suitable condition). Then the > > > existing tests that do the basic "check we can instantiate every > > > device and initialize every board model" would run that code > > > and catch most missing terminator cases, I think. > > > > Agreed. How about assert it even without qtest? We do tons of assertion for > > programming errors anyway in QEMU. > > I don't inherently object, but in this case to do the assertion > we'd need to do a scan over the fields arrays which we wouldn't > otherwise need to, so the cost of the assert is not simply > the compare-and-branch but also the loop over the array. If > that's not significant in terms of start-up time costs we can > just go ahead and do it (which would be nicer for debugging > and making it really obvious to people writing new devices) > but my remark above was a gesture towards "maybe we need to > not do it for normal startup".. Hmm.. Then how about put it into a "#ifdef CONFIG_DEBUG"? We may need some extra lines in configure, though: if test "$debug" = "yes"; then echo "CONFIG_DEBUG=y" >> $config_host_mak fi PS: I'm a bit surprised we don't have CONFIG_DEBUG already.. Thanks,
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 017c03675c..b50708e57a 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -147,6 +147,9 @@ enum VMStateFlags { * VMStateField.struct_version_id to tell which version of the * structure we are referencing to use. */ VMS_VSTRUCT = 0x8000, + + /* Marker for end of list */ + VMS_END = 0x10000 }; typedef enum { @@ -1163,7 +1166,9 @@ extern const VMStateInfo vmstate_info_qlist; VMSTATE_UNUSED_BUFFER(_test, 0, _size) #define VMSTATE_END_OF_LIST() \ - {} + { \ + .flags = VMS_END, \ + } int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, void *opaque, int version_id); diff --git a/migration/vmstate.c b/migration/vmstate.c index 05f87cdddc..181ba08c7d 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -160,6 +160,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, } field++; } + assert(field->flags == VMS_END); ret = vmstate_subsection_load(f, vmsd, opaque); if (ret != 0) { return ret; @@ -413,6 +414,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, } field++; } + assert(field->flags == VMS_END); if (vmdesc) { json_writer_end_array(vmdesc);