- We need to have good error reporting in the callbacks in
  VMStateDescription struct. Specifically pre_save, post_save,
  pre_load and post_load callbacks.
- It is not possible to change these functions everywhere in one
  patch, therefore, we introduce a duplicate set of callbacks
  with Error object passed to them.
- So, in this commit, we implement 'errp' variants of these callbacks,
  introducing an explicit Error object parameter.
- This is a functional step towards transitioning the entire codebase
  to the new error-parameterized functions.
- Deliberately called in mutual exclusion from their counterparts,
  to prevent conflicts during the transition.
- 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.

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 include/migration/vmstate.h | 11 ++++++++
 migration/vmstate.c         | 62 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -200,15 +200,26 @@ struct VMStateDescription {
      * exclusive. For this reason, also early_setup VMSDs are migrated in a
      * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated in
      * a QEMU_VM_SECTION_START section.
+     *
+     * There are duplicate impls of the post/pre save/load hooks.
+     * 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 early_setup;
     int version_id;
     int minimum_version_id;
     MigrationPriority priority;
     int (*pre_load)(void *opaque);
+    int (*pre_load_errp)(void *opaque, Error **errp);
     int (*post_load)(void *opaque, int version_id);
+    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
     int (*pre_save)(void *opaque);
+    int (*pre_save_errp)(void *opaque, Error **errp);
     int (*post_save)(void *opaque);
+    int (*post_save_errp)(void *opaque, Error **errp);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
 
diff --git a/migration/vmstate.c b/migration/vmstate.c
index bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
         return -EINVAL;
     }
-    if (vmsd->pre_load) {
+    if (vmsd->pre_load_errp) {
+        ret = vmsd->pre_load_errp(opaque, errp);
+        if (ret) {
+            error_prepend(errp, "VM pre load failed for: '%s', "
+                          "version_id: '%d', minimum version_id: '%d', "
+                          "ret: %d: ", vmsd->name, vmsd->version_id,
+                          vmsd->minimum_version_id, ret);
+            return ret;
+        }
+    } else if (vmsd->pre_load) {
         ret = vmsd->pre_load(opaque);
         if (ret) {
             error_setg(errp, "VM pre load failed for: '%s', "
@@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         qemu_file_set_error(f, ret);
         return ret;
     }
-    if (vmsd->post_load) {
+    if (vmsd->post_load_errp) {
+        ret = vmsd->post_load_errp(opaque, version_id, errp);
+        if (ret < 0) {
+            error_prepend(errp, "VM Post load failed for: %s, version_id: %d, "
+                          "minimum_version: %d, ret: %d: ", vmsd->name,
+                          vmsd->version_id, vmsd->minimum_version_id, ret);
+        }
+    } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
         if (ret < 0) {
             error_setg(errp,
@@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                          void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
 {
     int ret = 0;
+    Error *local_err = NULL;
     const VMStateField *field = vmsd->fields;
 
     trace_vmstate_save_state_top(vmsd->name);
 
-    if (vmsd->pre_save) {
+    if (vmsd->pre_save_errp) {
+        ret = vmsd->pre_save_errp(opaque, errp);
+        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
+        if (ret) {
+            error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
+            return ret;
+        }
+    } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret) {
@@ -524,10 +548,22 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 }
 
                 if (ret) {
-                    error_setg(errp, "Save of field %s/%s failed",
-                                vmsd->name, field->name);
-                    if (vmsd->post_save) {
-                        vmsd->post_save(opaque);
+                    if (*errp == NULL) {
+                        error_setg(errp, "Save of field %s/%s failed",
+                                   vmsd->name, field->name);
+                    }
+                    if (vmsd->post_save_errp) {
+                        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
+                        if (ps_ret < 0) {
+                            error_free_or_abort(errp);
+                            error_propagate(errp, local_err);
+                            ret = ps_ret;
+                        }
+                    } else if (vmsd->post_save) {
+                        int ps_ret = vmsd->post_save(opaque);
+                        if (ps_ret < 0) {
+                            ret = ps_ret;
+                        }
                     }
                     return ret;
                 }
@@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
     ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
 
-    if (vmsd->post_save) {
+    if (vmsd->post_save_errp) {
+        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
+        if (!ret && ps_ret) {
+            ret = ps_ret;
+            error_propagate(errp, local_err);
+        } else if (ret && ps_ret) {
+            error_free_or_abort(errp);
+            error_propagate(errp, local_err);
+            ret = ps_ret;
+        }
+    } else if (vmsd->post_save) {
         int ps_ret = vmsd->post_save(opaque);
         if (!ret && ps_ret) {
             ret = ps_ret;

-- 
2.50.0

Hi

On Fri, Jul 25, 2025 at 4:22 PM Arun Menon <armenon@redhat.com> wrote:

> - We need to have good error reporting in the callbacks in
>   VMStateDescription struct. Specifically pre_save, post_save,
>   pre_load and post_load callbacks.
> - It is not possible to change these functions everywhere in one
>   patch, therefore, we introduce a duplicate set of callbacks
>   with Error object passed to them.
> - So, in this commit, we implement 'errp' variants of these callbacks,
>   introducing an explicit Error object parameter.
> - This is a functional step towards transitioning the entire codebase
>   to the new error-parameterized functions.
> - Deliberately called in mutual exclusion from their counterparts,
>   to prevent conflicts during the transition.
> - 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.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  include/migration/vmstate.h | 11 ++++++++
>  migration/vmstate.c         | 62
> +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 65 insertions(+), 8 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index
> 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8
> 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -200,15 +200,26 @@ struct VMStateDescription {
>       * exclusive. For this reason, also early_setup VMSDs are migrated in
> a
>       * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated
> in
>       * a QEMU_VM_SECTION_START section.
> +     *
> +     * There are duplicate impls of the post/pre save/load hooks.
> +     * 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.
>

It is not documented, but I think all the callbacks return value should be
0 on success, <0 on error with -value an error number from errno.h. Could
you document it?


>       */
> +
>      bool early_setup;
>      int version_id;
>      int minimum_version_id;
>      MigrationPriority priority;
>      int (*pre_load)(void *opaque);
> +    int (*pre_load_errp)(void *opaque, Error **errp);
>      int (*post_load)(void *opaque, int version_id);
> +    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
>      int (*pre_save)(void *opaque);
> +    int (*pre_save_errp)(void *opaque, Error **errp);
>      int (*post_save)(void *opaque);
> +    int (*post_save_errp)(void *opaque, Error **errp);
>      bool (*needed)(void *opaque);
>      bool (*dev_unplug_pending)(void *opaque);
>

You will also need to update docs/devel/migration/main.rst to reflect the
new callbacks


>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index
> bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0
> 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const
> VMStateDescription *vmsd,
>          trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>          return -EINVAL;
>      }
> -    if (vmsd->pre_load) {
> +    if (vmsd->pre_load_errp) {
> +        ret = vmsd->pre_load_errp(opaque, errp);
> +        if (ret) {
> +            error_prepend(errp, "VM pre load failed for: '%s', "
> +                          "version_id: '%d', minimum version_id: '%d', "
> +                          "ret: %d: ", vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id, ret);
>

(but we don't have error_prepend_errno, ok)


> +            return ret;

+        }
> +    } else if (vmsd->pre_load) {
>          ret = vmsd->pre_load(opaque);
>          if (ret) {
>              error_setg(errp, "VM pre load failed for: '%s', "
> @@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const
> VMStateDescription *vmsd,
>          qemu_file_set_error(f, ret);
>          return ret;
>      }
> -    if (vmsd->post_load) {
> +    if (vmsd->post_load_errp) {
> +        ret = vmsd->post_load_errp(opaque, version_id, errp);
> +        if (ret < 0) {
> +            error_prepend(errp, "VM Post load failed for: %s, version_id:
> %d, "
> +                          "minimum_version: %d, ret: %d: ", vmsd->name,
> +                          vmsd->version_id, vmsd->minimum_version_id,
> ret);
> +        }
> +    } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
>          if (ret < 0) {
>              error_setg(errp,
> @@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const
> VMStateDescription *vmsd,
>                           void *opaque, JSONWriter *vmdesc, int
> version_id, Error **errp)
>  {
>      int ret = 0;
> +    Error *local_err = NULL;
>      const VMStateField *field = vmsd->fields;
>
>      trace_vmstate_save_state_top(vmsd->name);
>
> -    if (vmsd->pre_save) {
> +    if (vmsd->pre_save_errp) {
> +        ret = vmsd->pre_save_errp(opaque, errp);
> +        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> +        if (ret) {
> +            error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
>

Here, it would be helpful to report "ret" too.


> +            return ret;
> +        }
> +    } else if (vmsd->pre_save) {
>

Imho, you should try to introduce a new helper function to call the
implemented callback appropriately and handle the non-errp case:

int the_callback(vmsd,.. **errp)
{
  ERRP_GUARD(); // mostly for consistency

  if (vmsd->the_callback_errp) {
   return vmsd->the_callback_errp(args.., errp);
  } else if (vmsd->the_callback) {
    ret =vmsd->the_callback(args...);
    error_setg_errno(-ret, "the callback failed...")
    return ret;
  }

  return 0;
}

This should make it a bit easier to deal with.

         ret = vmsd->pre_save(opaque);
>          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>          if (ret) {
> @@ -524,10 +548,22 @@ int vmstate_save_state_v(QEMUFile *f, const
> VMStateDescription *vmsd,
>                  }
>
>                  if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> -                    if (vmsd->post_save) {
> -                        vmsd->post_save(opaque);
> +                    if (*errp == NULL) {
> +                        error_setg(errp, "Save of field %s/%s failed",
> +                                   vmsd->name, field->name);
> +                    }
> +                    if (vmsd->post_save_errp) {
> +                        int ps_ret = vmsd->post_save_errp(opaque,
> &local_err);
> +                        if (ps_ret < 0) {
> +                            error_free_or_abort(errp);
> +                            error_propagate(errp, local_err);
> +                            ret = ps_ret;
> +                        }
> +                    } else if (vmsd->post_save) {
> +                        int ps_ret = vmsd->post_save(opaque);
>

It's a bit odd how post_save() is being used here. It could be worth to
document that it is called regardless of success of migration in callback
doc.

Imho, the function vmstate_save_state_v() should be refactored to call
pre_save() and post_save() and call an internal function in between. It
will also help to shrink the function a bit.


> +                        if (ps_ret < 0) {
> +                            ret = ps_ret;
> +                        }
>                      }
>                      return ret;
>                  }
> @@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const
> VMStateDescription *vmsd,
>
>      ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>
> -    if (vmsd->post_save) {
> +    if (vmsd->post_save_errp) {
> +        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> +        if (!ret && ps_ret) {
> +            ret = ps_ret;
> +            error_propagate(errp, local_err);
> +        } else if (ret && ps_ret) {
> +            error_free_or_abort(errp);
> +            error_propagate(errp, local_err);
> +            ret = ps_ret;
> +        }
> +    } else if (vmsd->post_save) {
>          int ps_ret = vmsd->post_save(opaque);
>          if (!ret && ps_ret) {
>              ret = ps_ret;
>
> --
> 2.50.0
>
>

Hi Marc-André,
Thanks for the review.

On Mon, Jul 28, 2025 at 03:07:25PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jul 25, 2025 at 4:22 PM Arun Menon <armenon@redhat.com> wrote:
> 
> > - We need to have good error reporting in the callbacks in
> >   VMStateDescription struct. Specifically pre_save, post_save,
> >   pre_load and post_load callbacks.
> > - It is not possible to change these functions everywhere in one
> >   patch, therefore, we introduce a duplicate set of callbacks
> >   with Error object passed to them.
> > - So, in this commit, we implement 'errp' variants of these callbacks,
> >   introducing an explicit Error object parameter.
> > - This is a functional step towards transitioning the entire codebase
> >   to the new error-parameterized functions.
> > - Deliberately called in mutual exclusion from their counterparts,
> >   to prevent conflicts during the transition.
> > - 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.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  include/migration/vmstate.h | 11 ++++++++
> >  migration/vmstate.c         | 62
> > +++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 65 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index
> > 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8
> > 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -200,15 +200,26 @@ struct VMStateDescription {
> >       * exclusive. For this reason, also early_setup VMSDs are migrated in
> > a
> >       * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated
> > in
> >       * a QEMU_VM_SECTION_START section.
> > +     *
> > +     * There are duplicate impls of the post/pre save/load hooks.
> > +     * 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.
> >
> 
> It is not documented, but I think all the callbacks return value should be
> 0 on success, <0 on error with -value an error number from errno.h. Could
> you document it?
Sure. I shall add it in the header file and also in main.rst
> 
> 
> >       */
> > +
> >      bool early_setup;
> >      int version_id;
> >      int minimum_version_id;
> >      MigrationPriority priority;
> >      int (*pre_load)(void *opaque);
> > +    int (*pre_load_errp)(void *opaque, Error **errp);
> >      int (*post_load)(void *opaque, int version_id);
> > +    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> >      int (*pre_save)(void *opaque);
> > +    int (*pre_save_errp)(void *opaque, Error **errp);
> >      int (*post_save)(void *opaque);
> > +    int (*post_save_errp)(void *opaque, Error **errp);
> >      bool (*needed)(void *opaque);
> >      bool (*dev_unplug_pending)(void *opaque);
> >
> 
> You will also need to update docs/devel/migration/main.rst to reflect the
> new callbacks
Yes
> 
> 
> >
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index
> > bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0
> > 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> >          trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> >          return -EINVAL;
> >      }
> > -    if (vmsd->pre_load) {
> > +    if (vmsd->pre_load_errp) {
> > +        ret = vmsd->pre_load_errp(opaque, errp);
> > +        if (ret) {
> > +            error_prepend(errp, "VM pre load failed for: '%s', "
> > +                          "version_id: '%d', minimum version_id: '%d', "
> > +                          "ret: %d: ", vmsd->name, vmsd->version_id,
> > +                          vmsd->minimum_version_id, ret);
> >
> 
> (but we don't have error_prepend_errno, ok)
> 
> 
> > +            return ret;
> 
> +        }
> > +    } else if (vmsd->pre_load) {
> >          ret = vmsd->pre_load(opaque);
> >          if (ret) {
> >              error_setg(errp, "VM pre load failed for: '%s', "
> > @@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> >          qemu_file_set_error(f, ret);
> >          return ret;
> >      }
> > -    if (vmsd->post_load) {
> > +    if (vmsd->post_load_errp) {
> > +        ret = vmsd->post_load_errp(opaque, version_id, errp);
> > +        if (ret < 0) {
> > +            error_prepend(errp, "VM Post load failed for: %s, version_id:
> > %d, "
> > +                          "minimum_version: %d, ret: %d: ", vmsd->name,
> > +                          vmsd->version_id, vmsd->minimum_version_id,
> > ret);
> > +        }
> > +    } else if (vmsd->post_load) {
> >          ret = vmsd->post_load(opaque, version_id);
> >          if (ret < 0) {
> >              error_setg(errp,
> > @@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const
> > VMStateDescription *vmsd,
> >                           void *opaque, JSONWriter *vmdesc, int
> > version_id, Error **errp)
> >  {
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >      const VMStateField *field = vmsd->fields;
> >
> >      trace_vmstate_save_state_top(vmsd->name);
> >
> > -    if (vmsd->pre_save) {
> > +    if (vmsd->pre_save_errp) {
> > +        ret = vmsd->pre_save_errp(opaque, errp);
> > +        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > +        if (ret) {
> > +            error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
> >
> 
> Here, it would be helpful to report "ret" too.
Sure, will add.
> 
> 
> > +            return ret;
> > +        }
> > +    } else if (vmsd->pre_save) {
> >
> 
> Imho, you should try to introduce a new helper function to call the
> implemented callback appropriately and handle the non-errp case:
> 
> int the_callback(vmsd,.. **errp)
> {
>   ERRP_GUARD(); // mostly for consistency
> 
>   if (vmsd->the_callback_errp) {
>    return vmsd->the_callback_errp(args.., errp);
>   } else if (vmsd->the_callback) {
>     ret =vmsd->the_callback(args...);
>     error_setg_errno(-ret, "the callback failed...")
>     return ret;
>   }
> 
>   return 0;
> }
> 
> This should make it a bit easier to deal with.
Yes, will do.
> 
>          ret = vmsd->pre_save(opaque);
> >          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> >          if (ret) {
> > @@ -524,10 +548,22 @@ int vmstate_save_state_v(QEMUFile *f, const
> > VMStateDescription *vmsd,
> >                  }
> >
> >                  if (ret) {
> > -                    error_setg(errp, "Save of field %s/%s failed",
> > -                                vmsd->name, field->name);
> > -                    if (vmsd->post_save) {
> > -                        vmsd->post_save(opaque);
> > +                    if (*errp == NULL) {
> > +                        error_setg(errp, "Save of field %s/%s failed",
> > +                                   vmsd->name, field->name);
> > +                    }
> > +                    if (vmsd->post_save_errp) {
> > +                        int ps_ret = vmsd->post_save_errp(opaque,
> > &local_err);
> > +                        if (ps_ret < 0) {
> > +                            error_free_or_abort(errp);
> > +                            error_propagate(errp, local_err);
> > +                            ret = ps_ret;
> > +                        }
> > +                    } else if (vmsd->post_save) {
> > +                        int ps_ret = vmsd->post_save(opaque);
> >
> 
> It's a bit odd how post_save() is being used here. It could be worth to
> document that it is called regardless of success of migration in callback
> doc.
I think it is present in the documentation.

+  This function is called after we save the state of one device
+  (even upon failure, unless the call to pre_save returned an error).

> 
> Imho, the function vmstate_save_state_v() should be refactored to call
> pre_save() and post_save() and call an internal function in between. It
> will also help to shrink the function a bit.
Possible. I shall give it a try. But just by adding pre-save and post-save
wrapper functions makes the code lot more readable.
> 
> 
> > +                        if (ps_ret < 0) {
> > +                            ret = ps_ret;
> > +                        }
> >                      }
> >                      return ret;
> >                  }
> > @@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const
> > VMStateDescription *vmsd,
> >
> >      ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
> >
> > -    if (vmsd->post_save) {
> > +    if (vmsd->post_save_errp) {
> > +        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> > +        if (!ret && ps_ret) {
> > +            ret = ps_ret;
> > +            error_propagate(errp, local_err);
> > +        } else if (ret && ps_ret) {
> > +            error_free_or_abort(errp);
> > +            error_propagate(errp, local_err);
> > +            ret = ps_ret;
> > +        }
> > +    } else if (vmsd->post_save) {
> >          int ps_ret = vmsd->post_save(opaque);
> >          if (!ret && ps_ret) {
> >              ret = ps_ret;
> >
> > --
> > 2.50.0
> >
> >

Regards,
Arun

On 2025/07/25 21:19, Arun Menon wrote:
> - We need to have good error reporting in the callbacks in
>    VMStateDescription struct. Specifically pre_save, post_save,
>    pre_load and post_load callbacks.
> - It is not possible to change these functions everywhere in one
>    patch, therefore, we introduce a duplicate set of callbacks
>    with Error object passed to them.
> - So, in this commit, we implement 'errp' variants of these callbacks,
>    introducing an explicit Error object parameter.
> - This is a functional step towards transitioning the entire codebase
>    to the new error-parameterized functions.
> - Deliberately called in mutual exclusion from their counterparts,
>    to prevent conflicts during the transition.
> - 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.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>   include/migration/vmstate.h | 11 ++++++++
>   migration/vmstate.c         | 62 +++++++++++++++++++++++++++++++++++++++------
>   2 files changed, 65 insertions(+), 8 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -200,15 +200,26 @@ struct VMStateDescription {
>        * exclusive. For this reason, also early_setup VMSDs are migrated in a
>        * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated in
>        * a QEMU_VM_SECTION_START section.
> +     *
> +     * There are duplicate impls of the post/pre save/load hooks.
> +     * 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 early_setup;
>       int version_id;
>       int minimum_version_id;
>       MigrationPriority priority;
>       int (*pre_load)(void *opaque);
> +    int (*pre_load_errp)(void *opaque, Error **errp);
>       int (*post_load)(void *opaque, int version_id);
> +    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
>       int (*pre_save)(void *opaque);
> +    int (*pre_save_errp)(void *opaque, Error **errp);
>       int (*post_save)(void *opaque);
> +    int (*post_save_errp)(void *opaque, Error **errp);
>       bool (*needed)(void *opaque);
>       bool (*dev_unplug_pending)(void *opaque);
>   
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>           trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>           return -EINVAL;
>       }
> -    if (vmsd->pre_load) {
> +    if (vmsd->pre_load_errp) {
> +        ret = vmsd->pre_load_errp(opaque, errp);
> +        if (ret) {
> +            error_prepend(errp, "VM pre load failed for: '%s', "
> +                          "version_id: '%d', minimum version_id: '%d', "
> +                          "ret: %d: ", vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id, ret);
> +            return ret;
> +        }
> +    } else if (vmsd->pre_load) {
>           ret = vmsd->pre_load(opaque);
>           if (ret) {
>               error_setg(errp, "VM pre load failed for: '%s', "
> @@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>           qemu_file_set_error(f, ret);
>           return ret;
>       }
> -    if (vmsd->post_load) {
> +    if (vmsd->post_load_errp) {
> +        ret = vmsd->post_load_errp(opaque, version_id, errp);
> +        if (ret < 0) {
> +            error_prepend(errp, "VM Post load failed for: %s, version_id: %d, "
> +                          "minimum_version: %d, ret: %d: ", vmsd->name,
> +                          vmsd->version_id, vmsd->minimum_version_id, ret);
> +        }
> +    } else if (vmsd->post_load) {
>           ret = vmsd->post_load(opaque, version_id);
>           if (ret < 0) {
>               error_setg(errp,
> @@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                            void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
>   {
>       int ret = 0;
> +    Error *local_err = NULL;
>       const VMStateField *field = vmsd->fields;
>   
>       trace_vmstate_save_state_top(vmsd->name);
>   
> -    if (vmsd->pre_save) {
> +    if (vmsd->pre_save_errp) {
> +        ret = vmsd->pre_save_errp(opaque, errp);
> +        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> +        if (ret) {
> +            error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
> +            return ret;
> +        }
> +    } else if (vmsd->pre_save) {
>           ret = vmsd->pre_save(opaque);
>           trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>           if (ret) {
> @@ -524,10 +548,22 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                   }
>   
>                   if (ret) {
> -                    error_setg(errp, "Save of field %s/%s failed",
> -                                vmsd->name, field->name);
> -                    if (vmsd->post_save) {
> -                        vmsd->post_save(opaque);
> +                    if (*errp == NULL) {

include/qapi/error.h says:
 >  * - You may pass NULL to not receive the error, &error_abort to abort
 >  *   on error, &error_fatal to exit(1) on error, or a pointer to a
 >  *   variable containing NULL to receive the error.

 >  * - The function may pass @errp to functions it calls to pass on
 >  *   their errors to its caller.  If it dereferences @errp to check
 >  *   for errors, it must use ERRP_GUARD().

I also think this change is irrelevant with the addition of the new 
'errp' variants; it fixes an assertion failure when 
vmstate_save_state_v() failed and set errp. It is not a new problem 
caused by the 'errp' variants.

If that's true, this change should have its own explanation in the patch 
message, and also be split into another patch as "a commit message that 
mentions 'Also, ...' is often a good candidate for splitting into 
multiple patches." (docs/devel/submitting-a-patch.rst)

> +                        error_setg(errp, "Save of field %s/%s failed",
> +                                   vmsd->name, field->name);
> +                    }
> +                    if (vmsd->post_save_errp) {
> +                        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> +                        if (ps_ret < 0) {

The error checks else this and next ps_ret checks only care if it's zero 
or not, but this checks for the sign, leading to inconsistency.

> +                            error_free_or_abort(errp);
> +                            error_propagate(errp, local_err);
> +                            ret = ps_ret;
> +                        }
> +                    } else if (vmsd->post_save) {
> +                        int ps_ret = vmsd->post_save(opaque);
> +                        if (ps_ret < 0) {
> +                            ret = ps_ret;
> +                        }
>                       }
>                       return ret;
>                   }
> @@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>   
>       ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>   
> -    if (vmsd->post_save) {
> +    if (vmsd->post_save_errp) {
> +        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> +        if (!ret && ps_ret) {
> +            ret = ps_ret;
> +            error_propagate(errp, local_err);
> +        } else if (ret && ps_ret) {
> +            error_free_or_abort(errp);
> +            error_propagate(errp, local_err);
> +            ret = ps_ret;
> +        }

Simpler:

if (ps_ret) {
     if (ret) {
       error_free_or_abort(errp);
     }
     ret = ps_ret;
     error_propagate(errp, local_err);
}

> +    } else if (vmsd->post_save) {
>           int ps_ret = vmsd->post_save(opaque);
>           if (!ret && ps_ret) {
>               ret = ps_ret;

When there is a preceding error, this code still returns it and 
dismisses the post_save() error although the other part of this function 
is changed to propagate the error of post-save unconditionally. Please 
keep them consistent.

>

Hi Akihiko,

Thanks for the review.

On Sat, Jul 26, 2025 at 09:48:12PM +0900, Akihiko Odaki wrote:
> On 2025/07/25 21:19, Arun Menon wrote:
> > - We need to have good error reporting in the callbacks in
> >    VMStateDescription struct. Specifically pre_save, post_save,
> >    pre_load and post_load callbacks.
> > - It is not possible to change these functions everywhere in one
> >    patch, therefore, we introduce a duplicate set of callbacks
> >    with Error object passed to them.
> > - So, in this commit, we implement 'errp' variants of these callbacks,
> >    introducing an explicit Error object parameter.
> > - This is a functional step towards transitioning the entire codebase
> >    to the new error-parameterized functions.
> > - Deliberately called in mutual exclusion from their counterparts,
> >    to prevent conflicts during the transition.
> > - 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.
> > 
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >   include/migration/vmstate.h | 11 ++++++++
> >   migration/vmstate.c         | 62 +++++++++++++++++++++++++++++++++++++++------
> >   2 files changed, 65 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -200,15 +200,26 @@ struct VMStateDescription {
> >        * exclusive. For this reason, also early_setup VMSDs are migrated in a
> >        * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated in
> >        * a QEMU_VM_SECTION_START section.
> > +     *
> > +     * There are duplicate impls of the post/pre save/load hooks.
> > +     * 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 early_setup;
> >       int version_id;
> >       int minimum_version_id;
> >       MigrationPriority priority;
> >       int (*pre_load)(void *opaque);
> > +    int (*pre_load_errp)(void *opaque, Error **errp);
> >       int (*post_load)(void *opaque, int version_id);
> > +    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> >       int (*pre_save)(void *opaque);
> > +    int (*pre_save_errp)(void *opaque, Error **errp);
> >       int (*post_save)(void *opaque);
> > +    int (*post_save_errp)(void *opaque, Error **errp);
> >       bool (*needed)(void *opaque);
> >       bool (*dev_unplug_pending)(void *opaque);
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >           trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> >           return -EINVAL;
> >       }
> > -    if (vmsd->pre_load) {
> > +    if (vmsd->pre_load_errp) {
> > +        ret = vmsd->pre_load_errp(opaque, errp);
> > +        if (ret) {
> > +            error_prepend(errp, "VM pre load failed for: '%s', "
> > +                          "version_id: '%d', minimum version_id: '%d', "
> > +                          "ret: %d: ", vmsd->name, vmsd->version_id,
> > +                          vmsd->minimum_version_id, ret);
> > +            return ret;
> > +        }
> > +    } else if (vmsd->pre_load) {
> >           ret = vmsd->pre_load(opaque);
> >           if (ret) {
> >               error_setg(errp, "VM pre load failed for: '%s', "
> > @@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >           qemu_file_set_error(f, ret);
> >           return ret;
> >       }
> > -    if (vmsd->post_load) {
> > +    if (vmsd->post_load_errp) {
> > +        ret = vmsd->post_load_errp(opaque, version_id, errp);
> > +        if (ret < 0) {
> > +            error_prepend(errp, "VM Post load failed for: %s, version_id: %d, "
> > +                          "minimum_version: %d, ret: %d: ", vmsd->name,
> > +                          vmsd->version_id, vmsd->minimum_version_id, ret);
> > +        }
> > +    } else if (vmsd->post_load) {
> >           ret = vmsd->post_load(opaque, version_id);
> >           if (ret < 0) {
> >               error_setg(errp,
> > @@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >                            void *opaque, JSONWriter *vmdesc, int version_id, Error **errp)
> >   {
> >       int ret = 0;
> > +    Error *local_err = NULL;
> >       const VMStateField *field = vmsd->fields;
> >       trace_vmstate_save_state_top(vmsd->name);
> > -    if (vmsd->pre_save) {
> > +    if (vmsd->pre_save_errp) {
> > +        ret = vmsd->pre_save_errp(opaque, errp);
> > +        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > +        if (ret) {
> > +            error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
> > +            return ret;
> > +        }
> > +    } else if (vmsd->pre_save) {
> >           ret = vmsd->pre_save(opaque);
> >           trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> >           if (ret) {
> > @@ -524,10 +548,22 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >                   }
> >                   if (ret) {
> > -                    error_setg(errp, "Save of field %s/%s failed",
> > -                                vmsd->name, field->name);
> > -                    if (vmsd->post_save) {
> > -                        vmsd->post_save(opaque);
> > +                    if (*errp == NULL) {
> 
> include/qapi/error.h says:
> >  * - You may pass NULL to not receive the error, &error_abort to abort
> >  *   on error, &error_fatal to exit(1) on error, or a pointer to a
> >  *   variable containing NULL to receive the error.
> 
> >  * - The function may pass @errp to functions it calls to pass on
> >  *   their errors to its caller.  If it dereferences @errp to check
> >  *   for errors, it must use ERRP_GUARD().

Yes, we can add an ERRP_GUARD() before the null check (*errp == NULL), iff we
need a check, more on that below.
> 
> I also think this change is irrelevant with the addition of the new 'errp'
> variants; it fixes an assertion failure when vmstate_save_state_v() failed
> and set errp. It is not a new problem caused by the 'errp' variants.
> 
You are right.

vmstate_save_state() function, inturn calls vmstate_save_state_v() with NULL errp.
So that can be ignored for now, as the errors will not be set in errp at all.
Calling error_setg() exclusively later, will not cause any trouble.

On the other hand,
vmstate_save_state_v() will set errp.
And I thought re-setting it here, using error_setg() will crash QEMU.
commit 848a0503422d043d541130d5e3e2f7bc147cdef9 introduced error_setg()
in place of error_report() 

However, on review, I do think nothing has changed indeed as far as
this patchset is concerned. I do wonder though, how resetting this
does not crash QEMU. Maybe I am missing something.

> If that's true, this change should have its own explanation in the patch
> message, and also be split into another patch as "a commit message that
> mentions 'Also, ...' is often a good candidate for splitting into multiple
> patches." (docs/devel/submitting-a-patch.rst)
> 
> > +                        error_setg(errp, "Save of field %s/%s failed",
> > +                                   vmsd->name, field->name);
> > +                    }
> > +                    if (vmsd->post_save_errp) {
> > +                        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> > +                        if (ps_ret < 0) {
> 
> The error checks else this and next ps_ret checks only care if it's zero or
> not, but this checks for the sign, leading to inconsistency.

Yes, I shall keep it consistent, a non-zero value means failure. No need of sign check.
> 
> > +                            error_free_or_abort(errp);
> > +                            error_propagate(errp, local_err);
> > +                            ret = ps_ret;
> > +                        }
> > +                    } else if (vmsd->post_save) {
> > +                        int ps_ret = vmsd->post_save(opaque);
> > +                        if (ps_ret < 0) {
> > +                            ret = ps_ret;
> > +                        }
> >                       }
> >                       return ret;
> >                   }
> > @@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >       ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
> > -    if (vmsd->post_save) {
> > +    if (vmsd->post_save_errp) {
> > +        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
> > +        if (!ret && ps_ret) {
> > +            ret = ps_ret;
> > +            error_propagate(errp, local_err);
> > +        } else if (ret && ps_ret) {
> > +            error_free_or_abort(errp);
> > +            error_propagate(errp, local_err);
> > +            ret = ps_ret;
> > +        }
> 
> Simpler:
> 
> if (ps_ret) {
>     if (ret) {
>       error_free_or_abort(errp);
>     }
>     ret = ps_ret;
>     error_propagate(errp, local_err);
> }
> 
Will do.
> > +    } else if (vmsd->post_save) {
> >           int ps_ret = vmsd->post_save(opaque);
> >           if (!ret && ps_ret) {
> >               ret = ps_ret;
> 
> When there is a preceding error, this code still returns it and dismisses
> the post_save() error although the other part of this function is changed to
> propagate the error of post-save unconditionally. Please keep them
> consistent.

I do feel that in the new implementation (post_save_errp), we should be dismissing
the preceeding error and propagating the new error from post_save(). 
Because, that way, it makes sense to run the post_save_errp() part even after encountering
an error in the preceeding section (vmstate_subsection_save).

Not sure if I should change the old impl post_save to match post_save_errp,
Or should I match the new impl with the old one.
> 
> > 
> 


Regards,
Arun

On 2025/07/28 19:54, Arun Menon wrote:
>>
>>> +                            error_free_or_abort(errp);
>>> +                            error_propagate(errp, local_err);
>>> +                            ret = ps_ret;
>>> +                        }
>>> +                    } else if (vmsd->post_save) {
>>> +                        int ps_ret = vmsd->post_save(opaque);
>>> +                        if (ps_ret < 0) {
>>> +                            ret = ps_ret;
>>> +                        }
>>>                        }
>>>                        return ret;
>>>                    }
>>> @@ -554,7 +590,17 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>>        ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>>> -    if (vmsd->post_save) {
>>> +    if (vmsd->post_save_errp) {
>>> +        int ps_ret = vmsd->post_save_errp(opaque, &local_err);
>>> +        if (!ret && ps_ret) {
>>> +            ret = ps_ret;
>>> +            error_propagate(errp, local_err);
>>> +        } else if (ret && ps_ret) {
>>> +            error_free_or_abort(errp);
>>> +            error_propagate(errp, local_err);
>>> +            ret = ps_ret;
>>> +        }
>>
>> Simpler:
>>
>> if (ps_ret) {
>>      if (ret) {
>>        error_free_or_abort(errp);
>>      }
>>      ret = ps_ret;
>>      error_propagate(errp, local_err);
>> }
>>
> Will do.
>>> +    } else if (vmsd->post_save) {
>>>            int ps_ret = vmsd->post_save(opaque);
>>>            if (!ret && ps_ret) {
>>>                ret = ps_ret;
>>
>> When there is a preceding error, this code still returns it and dismisses
>> the post_save() error although the other part of this function is changed to
>> propagate the error of post-save unconditionally. Please keep them
>> consistent.
> 
> I do feel that in the new implementation (post_save_errp), we should be dismissing
> the preceeding error and propagating the new error from post_save().
> Because, that way, it makes sense to run the post_save_errp() part even after encountering
> an error in the preceeding section (vmstate_subsection_save).

What to propagate to the caller shouldn't matter when running the 
post_save_errp() because the behavior is for the caller and not for the 
internal implementation of this function.

There are two requirements for this function:
1. The proceeding error should be passed to the caller
2. The proceeding error should not prevent calling post_save_errp()

Let's assume whether the proceeding error is dismissed or not affects 
both post_save_errp() and the caller. Under this assumption,
- if the proceeding error is dismissed, it will violate condition 1.
- if the proceeding error is not dismissed, it will violate condition 2.

So it is more reasonable to think that we are defining different 
handling methods for the caller and post_save_errp().

If error propagation for the caller still has an implication on the 
condition to run post_save_errp(), it means the meanings of the 
proceeding error for post_save_errp() and the caller are "different but 
somehow correlated". It sounds convoluted and I don't have an idea of 
such correlation.

I don't think there is a common answer for what error to propagate when 
there are several errors, so I see three options:
- propagate whatever error easier to do so
- somehow combine errors
   (languages that support exceptions often do this)
- make sure that no error will happen when there is a proceeding error
   by having two callbacks:
   - One callback that is called only when there is no proceeding error
     and can raise an error
   - Another that is always called but cannot raise an error

> 
> Not sure if I should change the old impl post_save to match post_save_errp,
> Or should I match the new impl with the old one.

The reasoning you provided applies for post_save() too, so I think they 
should have a consistent behavior if you stick to it.

That change is also better to be split into its own patch; by splitting 
it, you can ensure that, if this series causes a regression for TPM, 
bisect will tell what led to the regression: the return value 
propagation change or the addition of errp.

Regards,
Akihiko Odaki