[PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Dr. David Alan Gilbert (git)]

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.

Note: This can't go in until we update the copy of vmstate.h in slirp.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/vmstate.h | 7 ++++++-
 migration/vmstate.c         | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

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);
-- 
2.34.1

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Maydell]

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

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Dr. David Alan Gilbert]

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Maydell]

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

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Maydell]

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

Re: [PATCH] migration: Add canary to VMSTATE_END_OF_LIST

[Peter Xu]

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,

-- 
Peter Xu