| Message ID | 20220113194452.254011-4-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> > > Perform a check on vmsd structures during test runs in the hope > of catching any missing terminators and other simple screwups. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > migration/savevm.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Perform a check on vmsd structures during test runs in the hope > of catching any missing terminators and other simple screwups. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Perform a check on vmsd structures during test runs in the hope > > of catching any missing terminators and other simple screwups. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > queued. Careful; I think that'll break with slirp until libslirp gets updated first. Dave
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> > >> > Perform a check on vmsd structures during test runs in the hope >> > of catching any missing terminators and other simple screwups. >> > >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> >> queued. > > Careful; I think that'll break with slirp until libslirp gets updated > first. As expected, it broke it. I resend the PULL request wihtout that two patches. Once that we are here, how it is that make check didn't catch this? Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> > > >> > Perform a check on vmsd structures during test runs in the hope > >> > of catching any missing terminators and other simple screwups. > >> > > >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> > >> Reviewed-by: Juan Quintela <quintela@redhat.com> > >> > >> queued. > > > > Careful; I think that'll break with slirp until libslirp gets updated > > first. > > As expected, it broke it. > > I resend the PULL request wihtout that two patches. > > Once that we are here, how it is that make check didn't catch this? Because in my local world I did the changes to libslirp; I wanted to make sure qemu people were happy with the changes before proposing them to libslirp. Which I've just done: https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112 Dave > Later, Juan. >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > * Juan Quintela (quintela@redhat.com) wrote: >> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: >> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> >> > >> >> > Perform a check on vmsd structures during test runs in the hope >> >> > of catching any missing terminators and other simple screwups. >> >> > >> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> >> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> >> >> >> queued. >> > >> > Careful; I think that'll break with slirp until libslirp gets updated >> > first. >> >> As expected, it broke it. >> >> I resend the PULL request wihtout that two patches. >> >> Once that we are here, how it is that make check didn't catch this? > > Because in my local world I did the changes to libslirp; I wanted to > make sure qemu people were happy with the changes before proposing them > to libslirp. > > Which I've just done: > > https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112 I mean make check. It worked for me on my PULL request. I would have assumed that it checks slirp. Later, Juan.
On Wed, 2 Feb 2022 at 11:32, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > Because in my local world I did the changes to libslirp; I wanted to > make sure qemu people were happy with the changes before proposing them > to libslirp. > > Which I've just done: > > https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/112 Does QEMU's own vmstate handling code see the libslirp vmstate structures ? Looking at the code it seems to me like QEMU's migration code only interacts with slirp via the slirp_state_save() and slirp_state_load() functions. Internally those work with some use of a vmstate structure, but the code that iterates over field arrays in those is all inside slirp itself (in src/slirp/vmstate.c if you're looking at the in-tree copy). So maybe I'm missing something but I'm not sure there really is a dependency on the libslirp change here... -- PMM
diff --git a/migration/savevm.c b/migration/savevm.c index 8077393d11..97a4471220 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -66,6 +66,7 @@ #include "net/announce.h" #include "qemu/yank.h" #include "yank_functions.h" +#include "sysemu/qtest.h" const unsigned int postcopy_ram_discard_version; @@ -839,6 +840,39 @@ void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque) } } +/* + * Perform some basic checks on vmsd's at registration + * time. + */ +static void vmstate_check(const VMStateDescription *vmsd) +{ + const VMStateField *field = vmsd->fields; + const VMStateDescription **subsection = vmsd->subsections; + + if (field) { + while (field->name) { + if (field->flags & (VMS_STRUCT | VMS_VSTRUCT)) { + /* Recurse to sub structures */ + vmstate_check(field->vmsd); + } + /* Carry on */ + field++; + } + /* Check for the end of field list canary */ + assert(field->flags == VMS_END); + } + + while (subsection && *subsection) { + /* + * The name of a subsection should start with the name of the + * current object. + */ + assert(!strncmp(vmsd->name, (*subsection)->name, strlen(vmsd->name))); + vmstate_check(*subsection); + subsection++; + } +} + int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, const VMStateDescription *vmsd, void *opaque, int alias_id, @@ -884,6 +918,11 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id, } else { se->instance_id = instance_id; } + + /* Perform a recursive sanity check during the test runs */ + if (qtest_enabled()) { + vmstate_check(vmsd); + } assert(!se->compat || se->instance_id == 0); savevm_state_handler_insert(se); return 0;