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

[Fabiano Rosas]

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?

Aside from that:

Reviewed-by: Fabiano Rosas <farosas@suse.de>