On Thu, Sep 07, 2023 at 08:55:52AM -0300, Fabiano Rosas wrote: > Peter Xu <pet...@redhat.com> writes: > > > For both save/load we actually share the logic on deciding whether a field > > should exist. Merge the checks into a helper and use it for both save and > > load. When doing so, add documentations and reformat the code to make it > > much easier to read. > > > > The real benefit here (besides code cleanups) is we add a trace-point for > > this; this is a known spot where we can easily break migration > > compatibilities between binaries, and this trace point will be critical for > > us to identify such issues. > > > > For example, this will be handy when debugging things like: > > > > https://gitlab.com/qemu-project/qemu/-/issues/932 > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > migration/vmstate.c | 34 ++++++++++++++++++++++++++-------- > > migration/trace-events | 1 + > > 2 files changed, 27 insertions(+), 8 deletions(-) > > > > diff --git a/migration/vmstate.c b/migration/vmstate.c > > index 31842c3afb..73e74ddea0 100644 > > --- a/migration/vmstate.c > > +++ b/migration/vmstate.c > > @@ -25,6 +25,30 @@ static int vmstate_subsection_save(QEMUFile *f, const > > VMStateDescription *vmsd, > > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription > > *vmsd, > > void *opaque); > > > > +/* Whether this field should exist for either save or load the VM? */ > > +static bool > > +vmstate_field_exists(const VMStateDescription *vmsd, const VMStateField > > *field, > > + void *opaque, int version_id) > > +{ > > + bool result; > > + > > + if (field->field_exists) { > > + /* If there's the function checker, that's the solo truth */ > > + result = field->field_exists(opaque, version_id); > > + trace_vmstate_field_exists(vmsd->name, field->name, > > field->version_id, > > + version_id, result); > > + } else { > > + /* > > + * Otherwise, we only save/load if field version is same or older. > > + * For example, when loading from an old binary with old version, > > + * we ignore new fields with newer version_ids. > > + */ > > + result = field->version_id <= version_id; > > This one doesn't get a trace?
Right, I didn't add that since I found mostly the bug comes from field_exists() returning different values for different qemu binaries. So I explicitly didn't include that otherwise we'll see tons of entries under that regard but is probably safe. > > Aside from that: > > Reviewed-by: Fabiano Rosas <faro...@suse.de> Thanks! -- Peter Xu