| Message ID | 20220113194452.254011-3-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vmsd checks | expand |
On Thu, 13 Jan 2022 at 19:45, 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. > > 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> > --- Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 13/1/22 20:44, Dr. David Alan Gilbert (git) 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. > > Note: This can't go in until we update the copy of vmstate.h in slirp. Do we need a libslirp buildsys version check to get this patch merged? Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Suggested-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > include/migration/vmstate.h | 7 ++++++- > migration/savevm.c | 1 + > migration/vmstate.c | 2 ++ > 3 files changed, 9 insertions(+), 1 deletion(-)
On 14/1/22 12:32, Philippe Mathieu-Daudé wrote: > On 13/1/22 20:44, Dr. David Alan Gilbert (git) 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. >> >> Note: This can't go in until we update the copy of vmstate.h in slirp. > > Do we need a libslirp buildsys version check to get this patch merged? In that case we should use an intermediate function which would eventually call assert() after checking SLIRP_MAJOR_VERSION/... values. > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> Suggested-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> --- >> include/migration/vmstate.h | 7 ++++++- >> migration/savevm.c | 1 + >> migration/vmstate.c | 2 ++ >> 3 files changed, 9 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/savevm.c b/migration/savevm.c index 0bef031acb..8077393d11 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -620,6 +620,7 @@ static void dump_vmstate_vmsd(FILE *out_file, field++; first = false; } + assert(field->flags == VMS_END); fprintf(out_file, "\n%*s]", indent, ""); } if (vmsd->subsections != NULL) { 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);