Re: [PATCH] migration: Unify and trace vmstate field_exists() checks

[Peter Xu]

On Thu, Sep 07, 2023 at 08:55:52AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <peterx@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 <farosas@suse.de>

Thanks!

-- 
Peter Xu