Re: [PATCH v3 2/3] migration: Introduce a post_load_with_error hook

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Jul 02, 2025 at 05:06:51PM +0530, Arun Menon wrote:
> - Introduces a temporary `post_load_with_error()` hook.
>   This enables a gradual transition of error reporting,
>   eventually replacing `post_load` throughout the codebase.
> - Deliberately called in mutual exclusion from post_load() hook
>   to prevent conflicts during the transition.
> - Briefly discussed here : https://issues.redhat.com/browse/RHEL-82826

While the ticket is public, many of the comments are private
to RH. Generally any relevant info should be direclty included
in the commit message, so we don't rely on downstream vendor
bug trackers which may or may no be public long term.

> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  include/migration/vmstate.h | 1 +
>  migration/vmstate.c         | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 056781b1c21e737583f081594d9f88b32adfd674..1c6e89c3b08a3914cde6dce3be5955978b6b7d0b 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -207,6 +207,7 @@ struct VMStateDescription {
>      MigrationPriority priority;
>      int (*pre_load)(void *opaque);

This is called from the same context as post_load, so deserves
the same change to add an "errp".

>      int (*post_load)(void *opaque, int version_id);
> +    int (*post_load_with_error)(void *opaque, int version_id, Error **errp);

This is a bit overly verbose, 'post_load_errp' is sufficient.

>      int (*pre_save)(void *opaque);
>      int (*post_save)(void *opaque);

I'm inclined to suggest we add 'errp' variants of these too.

IOW, do the general design fix, not the bare minimum for this
specific tpm problem.

Then, we should also add an explanatory comment

 /*
  * New impls should preferentally use 'errp' variants of
  * these methods, and existing impls incrementally converted.
  * The variants without 'errp' are intended to be removed
  * once all usage is converted.
  */

>      bool (*needed)(void *opaque);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 158dcc3fcddfe70ab268dc5ace6e4573fa1ccbab..0048c4e1e7ee1d6ff3a3349abb0dc1578985a56e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -236,7 +236,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          }
>          return ret;
>      }
> -    if (vmsd->post_load) {
> +    if (vmsd->post_load_with_error) {
> +        ret = vmsd->post_load_with_error(opaque, version_id, errp);
> +    } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
>      }
>      trace_vmstate_load_state_end(vmsd->name, "end", ret);
> 
> -- 
> 2.49.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|