Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
On Wed, 2 Feb 2022 at 11:32, Dr. David Alan Gilbert 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
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
"Dr. David Alan Gilbert" wrote: > * Juan Quintela (quint...@redhat.com) wrote: >> "Dr. David Alan Gilbert" wrote: >> > * Juan Quintela (quint...@redhat.com) wrote: >> >> "Dr. David Alan Gilbert (git)" wrote: >> >> > From: "Dr. David Alan Gilbert" >> >> > >> >> > 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 >> >> >> >> Reviewed-by: Juan Quintela >> >> >> >> 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.
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
* Juan Quintela (quint...@redhat.com) wrote: > "Dr. David Alan Gilbert" wrote: > > * Juan Quintela (quint...@redhat.com) wrote: > >> "Dr. David Alan Gilbert (git)" wrote: > >> > From: "Dr. David Alan Gilbert" > >> > > >> > 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 > >> > >> Reviewed-by: Juan Quintela > >> > >> 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 / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
"Dr. David Alan Gilbert" wrote: > * Juan Quintela (quint...@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" wrote: >> > From: "Dr. David Alan Gilbert" >> > >> > 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 >> >> Reviewed-by: Juan Quintela >> >> 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.
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
* Juan Quintela (quint...@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > Reviewed-by: Juan Quintela > > queued. Careful; I think that'll break with slirp until libslirp gets updated first. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
"Dr. David Alan Gilbert (git)" wrote: > From: "Dr. David Alan Gilbert" > > 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 Reviewed-by: Juan Quintela queued.
Re: [PATCH v2 3/3] migration: Perform vmsd structure check during tests
On Thu, 13 Jan 2022 at 19:45, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > 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 > --- > migration/savevm.c | 39 +++ > 1 file changed, 39 insertions(+) Reviewed-by: Peter Maydell thanks -- PMM
[PATCH v2 3/3] migration: Perform vmsd structure check during tests
From: "Dr. David Alan Gilbert"
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
---
migration/savevm.c | 39 +++
1 file changed, 39 insertions(+)
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;
--
2.34.1
