- 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         | 47 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 6 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 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 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,10 +245,17 @@ 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, "VM Post load failed for: %s, version_id: %d,"
+            error_setg(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);
         }
@@ -410,11 +426,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,7 +548,12 @@ 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) {
+                    if (vmsd->post_save_errp) {
+                        ret = vmsd->post_save_errp(opaque, &local_err);
+                        if (ret < 0) {
+                            error_propagate(errp, local_err);
+                        }
+                    } else if (vmsd->post_save) {
                         vmsd->post_save(opaque);
                     }
                     return ret;
@@ -552,7 +581,13 @@ 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 (vmsd->post_save) {
         int ps_ret = vmsd->post_save(opaque);
         if (!ret && ps_ret) {
             ret = ps_ret;

-- 
2.50.0

On Mon, Jul 21, 2025 at 04:59:28PM +0530, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 6 deletions(-)
> 

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c

> @@ -524,7 +548,12 @@ 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) {
> +                    if (vmsd->post_save_errp) {
> +                        ret = vmsd->post_save_errp(opaque, &local_err);
> +                        if (ret < 0) {
> +                            error_propagate(errp, local_err);
> +                        }

This is still broken. 'errp' is already set a few lines earlier, so you
can't propagate a new error over the top

> +                    } else if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }
>                      return ret;
> @@ -552,7 +581,13 @@ 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);
> +        }

Again, propagating over the top of an existing error

> +    } else if (vmsd->post_save) {
>          int ps_ret = vmsd->post_save(opaque);
>          if (!ret && ps_ret) {
>              ret = ps_ret;
> 
> -- 
> 2.50.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 :|

Hi,
Thank you for the review.

On Mon, Jul 21, 2025 at 02:32:48PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 21, 2025 at 04:59:28PM +0530, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 52 insertions(+), 6 deletions(-)
> > 
> 
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> 
> > @@ -524,7 +548,12 @@ 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) {
> > +                    if (vmsd->post_save_errp) {
> > +                        ret = vmsd->post_save_errp(opaque, &local_err);
> > +                        if (ret < 0) {
> > +                            error_propagate(errp, local_err);
> > +                        }
> 
> This is still broken. 'errp' is already set a few lines earlier, so you
> can't propagate a new error over the top

I was wondering that we should preserve the first error that was encountered.
So even if local_err was set, and in case errp already has an error, then it will
be a no-op and local_err will be freed.
> 
> > +                    } else if (vmsd->post_save) {
> >                          vmsd->post_save(opaque);
> >                      }
> >                      return ret;
> > @@ -552,7 +581,13 @@ 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);
> > +        }
> 
> Again, propagating over the top of an existing error

Sorry, correct me if I am wrong.
Since we have 'if (!ret && ps_ret)' ,
if vmstate_subsection_save() fails, the above condition will not hold true.
Only if the first function call vmstate_subsection_save() is successful and the second one
post_save_errp() fails then we try to propagate, again hoping to preserve the first error. 

> 
> > +    } else if (vmsd->post_save) {
> >          int ps_ret = vmsd->post_save(opaque);
> >          if (!ret && ps_ret) {
> >              ret = ps_ret;
> > 
> > -- 
> > 2.50.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 :|
> 

Regards,
Arun Menon

On Mon, Jul 21, 2025 at 07:24:23PM +0530, Arun Menon wrote:
> Hi,
> Thank you for the review.
> 
> On Mon, Jul 21, 2025 at 02:32:48PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 21, 2025 at 04:59:28PM +0530, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
> > >  2 files changed, 52 insertions(+), 6 deletions(-)
> > > 
> > 
> > > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 100644
> > > --- a/migration/vmstate.c
> > > +++ b/migration/vmstate.c
> > 
> > > @@ -524,7 +548,12 @@ 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) {
> > > +                    if (vmsd->post_save_errp) {
> > > +                        ret = vmsd->post_save_errp(opaque, &local_err);
> > > +                        if (ret < 0) {
> > > +                            error_propagate(errp, local_err);
> > > +                        }
> > 
> > This is still broken. 'errp' is already set a few lines earlier, so you
> > can't propagate a new error over the top
> 
> I was wondering that we should preserve the first error that was encountered.
> So even if local_err was set, and in case errp already has an error, then it will
> be a no-op and local_err will be freed.

We know that 'local_err' is definitely set when 'post_save_errp' is called,
because there's a call to 'error_setg' right above it.



> > > +                    } else if (vmsd->post_save) {
> > >                          vmsd->post_save(opaque);
> > >                      }

... pre-existing mistake not checking return value of
post_save.

> > >                      return ret;
> > > @@ -552,7 +581,13 @@ 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);
> > > +        }
> > 
> > Again, propagating over the top of an existing error
> 
> Sorry, correct me if I am wrong.
> Since we have 'if (!ret && ps_ret)' ,
> if vmstate_subsection_save() fails, the above condition will not hold true.
> Only if the first function call vmstate_subsection_save() is successful and the second one
> post_save_errp() fails then we try to propagate, again hoping to preserve the first error.

Opps, yes, you're right - I missed the 'ps_ret' check. That means this
code is a memory leak when 'ret' is non-zero, as nothing frees 'local_err'
in that case.

> 
> > 
> > > +    } else if (vmsd->post_save) {
> > >          int ps_ret = vmsd->post_save(opaque);
> > >          if (!ret && ps_ret) {
> > >              ret = ps_ret;
> > > 
> > > -- 
> > > 2.50.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 :|
> > 
> 
> Regards,
> Arun Menon
> 

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 :|

On Mon, Jul 21, 2025 at 03:05:59PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 21, 2025 at 07:24:23PM +0530, Arun Menon wrote:
> > Hi,
> > Thank you for the review.
> > 
> > On Mon, Jul 21, 2025 at 02:32:48PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jul 21, 2025 at 04:59:28PM +0530, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
> > > >  2 files changed, 52 insertions(+), 6 deletions(-)
> > > > 
> > > 
> > > > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > > index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 100644
> > > > --- a/migration/vmstate.c
> > > > +++ b/migration/vmstate.c
> > > 
> > > > @@ -524,7 +548,12 @@ 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) {
> > > > +                    if (vmsd->post_save_errp) {
> > > > +                        ret = vmsd->post_save_errp(opaque, &local_err);
> > > > +                        if (ret < 0) {
> > > > +                            error_propagate(errp, local_err);
> > > > +                        }
> > > 
> > > This is still broken. 'errp' is already set a few lines earlier, so you
> > > can't propagate a new error over the top
> > 
> > I was wondering that we should preserve the first error that was encountered.
> > So even if local_err was set, and in case errp already has an error, then it will
> > be a no-op and local_err will be freed.
> 
> We know that 'local_err' is definitely set when 'post_save_errp' is called,
> because there's a call to 'error_setg' right above it.

mmm, error_setg() above that sets errp, local_err is set only of post_save_errp() has
errors. Do we want both the erros to be propagated? or is it okay to propagate the first
error that was encountered.
> 
> 
> 
> > > > +                    } else if (vmsd->post_save) {
> > > >                          vmsd->post_save(opaque);
> > > >                      }
> 
> ... pre-existing mistake not checking return value of
> post_save.
> 
> > > >                      return ret;
> > > > @@ -552,7 +581,13 @@ 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);
> > > > +        }
> > > 
> > > Again, propagating over the top of an existing error
> > 
> > Sorry, correct me if I am wrong.
> > Since we have 'if (!ret && ps_ret)' ,
> > if vmstate_subsection_save() fails, the above condition will not hold true.
> > Only if the first function call vmstate_subsection_save() is successful and the second one
> > post_save_errp() fails then we try to propagate, again hoping to preserve the first error.
> 
> Opps, yes, you're right - I missed the 'ps_ret' check. That means this
> code is a memory leak when 'ret' is non-zero, as nothing frees 'local_err'
> in that case.

Yes, maybe I can null check local_err and error_free() it.

> 
> > 
> > > 
> > > > +    } else if (vmsd->post_save) {
> > > >          int ps_ret = vmsd->post_save(opaque);
> > > >          if (!ret && ps_ret) {
> > > >              ret = ps_ret;
> > > > 
> > > > -- 
> > > > 2.50.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 :|
> > > 
> > 
> > Regards,
> > Arun Menon
> > 
> 
> 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 :|
> 
Regards,
Arun

Hi Daniel,

On Mon, Jul 21, 2025 at 08:40:10PM +0530, Arun Menon wrote:
> On Mon, Jul 21, 2025 at 03:05:59PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 21, 2025 at 07:24:23PM +0530, Arun Menon wrote:
> > > Hi,
> > > Thank you for the review.
> > > 
> > > On Mon, Jul 21, 2025 at 02:32:48PM +0100, Daniel P. Berrangé wrote:
> > > > On Mon, Jul 21, 2025 at 04:59:28PM +0530, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
> > > > >  2 files changed, 52 insertions(+), 6 deletions(-)
> > > > > 
> > > > 
> > > > > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > > > index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 100644
> > > > > --- a/migration/vmstate.c
> > > > > +++ b/migration/vmstate.c
> > > > 
> > > > > @@ -524,7 +548,12 @@ 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) {
> > > > > +                    if (vmsd->post_save_errp) {
> > > > > +                        ret = vmsd->post_save_errp(opaque, &local_err);
> > > > > +                        if (ret < 0) {
> > > > > +                            error_propagate(errp, local_err);
> > > > > +                        }
> > > > 
> > > > This is still broken. 'errp' is already set a few lines earlier, so you
> > > > can't propagate a new error over the top
> > > 
> > > I was wondering that we should preserve the first error that was encountered.
> > > So even if local_err was set, and in case errp already has an error, then it will
> > > be a no-op and local_err will be freed.
> > 
> > We know that 'local_err' is definitely set when 'post_save_errp' is called,
> > because there's a call to 'error_setg' right above it.
> 
> mmm, error_setg() above that sets errp, local_err is set only of post_save_errp() has
> errors. Do we want both the erros to be propagated? or is it okay to propagate the first
> error that was encountered.

I see your point. If we set errp, then there is no point in writing error_propagate()
because that will never allow local_err to be propagated because the first param is set.

I am intending to catch both the errors,
a. from vmstate_save_state_v() or explicitly set by us using error_setg(), in errp
b. from calling post_save_errp() , in local_err
and then, if we have both a and b, then we propagate  b.
and if we only have a, then a will be propagated.

I think this will be consistent with the original logic of setting the error using 
error_setg() and calling vmsd->post_save() only in the failure path. Please correct me
if I am wrong.

> > 
> > 
> > 
> > > > > +                    } else if (vmsd->post_save) {
> > > > >                          vmsd->post_save(opaque);
> > > > >                      }
> > 
> > ... pre-existing mistake not checking return value of
> > post_save.
> > 
> > > > >                      return ret;
> > > > > @@ -552,7 +581,13 @@ 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);
> > > > > +        }
> > > > 
> > > > Again, propagating over the top of an existing error
> > > 
> > > Sorry, correct me if I am wrong.
> > > Since we have 'if (!ret && ps_ret)' ,
> > > if vmstate_subsection_save() fails, the above condition will not hold true.
> > > Only if the first function call vmstate_subsection_save() is successful and the second one
> > > post_save_errp() fails then we try to propagate, again hoping to preserve the first error.
> > 
> > Opps, yes, you're right - I missed the 'ps_ret' check. That means this
> > code is a memory leak when 'ret' is non-zero, as nothing frees 'local_err'
> > in that case.
> 
> Yes, maybe I can null check local_err and error_free() it.
> 
> > 
> > > 
> > > > 
> > > > > +    } else if (vmsd->post_save) {
> > > > >          int ps_ret = vmsd->post_save(opaque);
> > > > >          if (!ret && ps_ret) {
> > > > >              ret = ps_ret;
> > > > > 
> > > > > -- 
> > > > > 2.50.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 :|
> > > > 
> > > 
> > > Regards,
> > > Arun Menon
> > > 
> > 
> > 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 :|
> > 
> Regards,
> Arun

Regards,
Arun

On 2025/07/21 20:29, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
>   2 files changed, 52 insertions(+), 6 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);

I think the new functions should have void as return value instead.

As I discussed before, I think having an integer return value is a 
source of confusion:
https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/

In the previous discussion, I suggested using bool, but void fits better 
in this particular case.

include/qapi/error.h says:
 > Whenever practical, also return a value that indicates success /
 > failure.  This can make the error checking more concise, and can avoid
 > useless error object creation and destruction.  Note that we still
 > have many functions returning void.

There will be more implementations of these function pointers than their 
callers, so it makes more sense to let return void and make 
implementations more concise while making the callers less so. There is 
also DeviceRealize, an example of function pointer type that takes errp 
but returns void.

>       bool (*needed)(void *opaque);
>       bool (*dev_unplug_pending)(void *opaque);
>   
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77 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,10 +245,17 @@ 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, "VM Post load failed for: %s, version_id: %d,"
> +            error_setg(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);
>           }
> @@ -410,11 +426,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,7 +548,12 @@ 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) {
> +                    if (vmsd->post_save_errp) {
> +                        ret = vmsd->post_save_errp(opaque, &local_err);
> +                        if (ret < 0) {
> +                            error_propagate(errp, local_err);
> +                        }
> +                    } else if (vmsd->post_save) {
>                           vmsd->post_save(opaque);
>                       }
>                       return ret;
> @@ -552,7 +581,13 @@ 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 (vmsd->post_save) {
>           int ps_ret = vmsd->post_save(opaque);
>           if (!ret && ps_ret) {
>               ret = ps_ret;
>

On Mon, Jul 21, 2025 at 10:14:30PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
> >   2 files changed, 52 insertions(+), 6 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);
> 
> I think the new functions should have void as return value instead.
> 
> As I discussed before, I think having an integer return value is a source of
> confusion:
> https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/
> 
> In the previous discussion, I suggested using bool, but void fits better in
> this particular case.
> 
> include/qapi/error.h says:
> > Whenever practical, also return a value that indicates success /
> > failure.  This can make the error checking more concise, and can avoid
> > useless error object creation and destruction.  Note that we still
> > have many functions returning void.
> 
> There will be more implementations of these function pointers than their
> callers, so it makes more sense to let return void and make implementations
> more concise while making the callers less so. There is also DeviceRealize,
> an example of function pointer type that takes errp but returns void.

No, please do NOT make these functions void. As that text you quote
says, we want functions to return a value indicating success/failure.
'void' return is a historical practice we don't want to continue
in QEMU.

Given that the existing methods all return 'int', we should remain
consistent with the new functions and return 'int', with -1 for
failure, 0 for success, and not use bool.

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 :|

On 2025/07/21 22:29, Daniel P. Berrangé wrote:
> On Mon, Jul 21, 2025 at 10:14:30PM +0900, Akihiko Odaki wrote:
>> On 2025/07/21 20:29, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
>>>    2 files changed, 52 insertions(+), 6 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);
>>
>> I think the new functions should have void as return value instead.
>>
>> As I discussed before, I think having an integer return value is a source of
>> confusion:
>> https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/
>>
>> In the previous discussion, I suggested using bool, but void fits better in
>> this particular case.
>>
>> include/qapi/error.h says:
>>> Whenever practical, also return a value that indicates success /
>>> failure.  This can make the error checking more concise, and can avoid
>>> useless error object creation and destruction.  Note that we still
>>> have many functions returning void.
>>
>> There will be more implementations of these function pointers than their
>> callers, so it makes more sense to let return void and make implementations
>> more concise while making the callers less so. There is also DeviceRealize,
>> an example of function pointer type that takes errp but returns void.
> 
> No, please do NOT make these functions void. As that text you quote
> says, we want functions to return a value indicating success/failure.
> 'void' return is a historical practice we don't want to continue
> in QEMU.
> 
> Given that the existing methods all return 'int', we should remain
> consistent with the new functions and return 'int', with -1 for
> failure, 0 for success, and not use bool.

Markus, I'd also like to hear your opinion since you are the maintainer 
of the error reporting facility.

Regards,
Akihiko Odaki

Almost missed this, sorry.

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/07/21 22:29, Daniel P. Berrangé wrote:
>> On Mon, Jul 21, 2025 at 10:14:30PM +0900, Akihiko Odaki wrote:
>>> On 2025/07/21 20:29, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
>>>>  2 files changed, 52 insertions(+), 6 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);
>>>
>>> I think the new functions should have void as return value instead.
>>>
>>> As I discussed before, I think having an integer return value is a source of
>>> confusion:
>>> https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/

I disagree.

We've discussed this a few times.  Here's a recent instance:
https://lore.kernel.org/qemu-devel/87jz5tbbqx.fsf@pond.sub.org/

>>> In the previous discussion, I suggested using bool, but void fits better in
>>> this particular case.
>>>
>>> include/qapi/error.h says:
>>>> Whenever practical, also return a value that indicates success /
>>>> failure.  This can make the error checking more concise, and can avoid
>>>> useless error object creation and destruction.  Note that we still
>>>> have many functions returning void.
>>>
>>> There will be more implementations of these function pointers than their
>>> callers, so it makes more sense to let return void and make implementations
>>> more concise while making the callers less so. There is also DeviceRealize,
>>> an example of function pointer type that takes errp but returns void.
>>
>> No, please do NOT make these functions void. As that text you quote
>> says, we want functions to return a value indicating success/failure.
>> 'void' return is a historical practice we don't want to continue
>> in QEMU.
>>
>> Given that the existing methods all return 'int', we should remain
>> consistent with the new functions and return 'int', with -1 for
>> failure, 0 for success, and not use bool.
>
> Markus, I'd also like to hear your opinion since you are the maintainer of the error reporting facility.

I'm with Daniel.

New code should stick to the rules.

Changing existing code from "sticks to the rules" to not requires pretty
compelling justification.

The other direction is more welcome, but whether the juice is worth the
squeeze still needs to be decided case by case.

On 2025/08/09 17:17, Markus Armbruster wrote:
> Almost missed this, sorry.
> 
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> On 2025/07/21 22:29, Daniel P. Berrangé wrote:
>>> On Mon, Jul 21, 2025 at 10:14:30PM +0900, Akihiko Odaki wrote:
>>>> On 2025/07/21 20:29, 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         | 47 +++++++++++++++++++++++++++++++++++++++------
>>>>>   2 files changed, 52 insertions(+), 6 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);
>>>>
>>>> I think the new functions should have void as return value instead.
>>>>
>>>> As I discussed before, I think having an integer return value is a source of
>>>> confusion:
>>>> https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c7211784a7@rsg.ci.i.u-tokyo.ac.jp/
> 
> I disagree.
> 
> We've discussed this a few times.  Here's a recent instance:
> https://lore.kernel.org/qemu-devel/87jz5tbbqx.fsf@pond.sub.org/
> 
>>>> In the previous discussion, I suggested using bool, but void fits better in
>>>> this particular case.
>>>>
>>>> include/qapi/error.h says:
>>>>> Whenever practical, also return a value that indicates success /
>>>>> failure.  This can make the error checking more concise, and can avoid
>>>>> useless error object creation and destruction.  Note that we still
>>>>> have many functions returning void.
>>>>
>>>> There will be more implementations of these function pointers than their
>>>> callers, so it makes more sense to let return void and make implementations
>>>> more concise while making the callers less so. There is also DeviceRealize,
>>>> an example of function pointer type that takes errp but returns void.
>>>
>>> No, please do NOT make these functions void. As that text you quote
>>> says, we want functions to return a value indicating success/failure.
>>> 'void' return is a historical practice we don't want to continue
>>> in QEMU.
>>>
>>> Given that the existing methods all return 'int', we should remain
>>> consistent with the new functions and return 'int', with -1 for
>>> failure, 0 for success, and not use bool.
>>
>> Markus, I'd also like to hear your opinion since you are the maintainer of the error reporting facility.
> 
> I'm with Daniel.
> 
> New code should stick to the rules.
> 
> Changing existing code from "sticks to the rules" to not requires pretty
> compelling justification.
> 
> The other direction is more welcome, but whether the juice is worth the
> squeeze still needs to be decided case by case.

What do you refer with the rules?

There were three options on the table: bool, int, and void.

The previous discussion you referred explains why void should be 
avoided, and include/qapi/error.h also says void should be avoided.

There is pre_load() that does not use Error returns int, but now we are 
adding pre_load_errp() that uses Error.

Then what pre_load_errp() should return: bool or int?

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/08/09 17:17, Markus Armbruster wrote:
>> Almost missed this, sorry.
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>> 
>>> On 2025/07/21 22:29, Daniel P. Berrangé wrote:

[...]

>>>> No, please do NOT make these functions void. As that text you quote
>>>> says, we want functions to return a value indicating success/failure.
>>>> 'void' return is a historical practice we don't want to continue
>>>> in QEMU.
>>>>
>>>> Given that the existing methods all return 'int', we should remain
>>>> consistent with the new functions and return 'int', with -1 for
>>>> failure, 0 for success, and not use bool.
>>>
>>> Markus, I'd also like to hear your opinion since you are the maintainer of the error reporting facility.
>>
>> I'm with Daniel.
>>
>> New code should stick to the rules.
>>
>> Changing existing code from "sticks to the rules" to not requires pretty
>> compelling justification.
>>
>> The other direction is more welcome, but whether the juice is worth the
>> squeeze still needs to be decided case by case.
>
> What do you refer with the rules?

The big comment in qapi/error.h starts with a section = Rules =.

> There were three options on the table: bool, int, and void.
>
> The previous discussion you referred explains why void should be avoided, and include/qapi/error.h also says void should be avoided.
>
> There is pre_load() that does not use Error returns int, but now we are adding pre_load_errp() that uses Error.
>
> Then what pre_load_errp() should return: bool or int?

I like bool when it's all we need.

When we need to return a non-negative int on success, use int and return
-1 or a negative error code on failure.

Another reason to pick int is local consistency: related functions
already use int.

Changing working code from int to bool doesn't seem worth the bother.

Questions?

On 2025/08/09 23:30, Markus Armbruster wrote:
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
> 
>> On 2025/08/09 17:17, Markus Armbruster wrote:
>>> Almost missed this, sorry.
>>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:
>>>
>>>> On 2025/07/21 22:29, Daniel P. Berrangé wrote:
> 
> [...]
> 
>>>>> No, please do NOT make these functions void. As that text you quote
>>>>> says, we want functions to return a value indicating success/failure.
>>>>> 'void' return is a historical practice we don't want to continue
>>>>> in QEMU.
>>>>>
>>>>> Given that the existing methods all return 'int', we should remain
>>>>> consistent with the new functions and return 'int', with -1 for
>>>>> failure, 0 for success, and not use bool.
>>>>
>>>> Markus, I'd also like to hear your opinion since you are the maintainer of the error reporting facility.
>>>
>>> I'm with Daniel.
>>>
>>> New code should stick to the rules.
>>>
>>> Changing existing code from "sticks to the rules" to not requires pretty
>>> compelling justification.
>>>
>>> The other direction is more welcome, but whether the juice is worth the
>>> squeeze still needs to be decided case by case.
>>
>> What do you refer with the rules?
> 
> The big comment in qapi/error.h starts with a section = Rules =.
> 
>> There were three options on the table: bool, int, and void.
>>
>> The previous discussion you referred explains why void should be avoided, and include/qapi/error.h also says void should be avoided.
>>
>> There is pre_load() that does not use Error returns int, but now we are adding pre_load_errp() that uses Error.
>>
>> Then what pre_load_errp() should return: bool or int?
> 
> I like bool when it's all we need.
> 
> When we need to return a non-negative int on success, use int and return
> -1 or a negative error code on failure.
> 
> Another reason to pick int is local consistency: related functions
> already use int.
> 
> Changing working code from int to bool doesn't seem worth the bother.
> 
> Questions?
> 

I at least see what "[PATCH v9 26/27] migration: Add error-parameterized 
function variants in VMSD struct" says is undesirable. It says:
 > For the errp variants,
 > Returns: 0 on success,
 >          <0 on error where -value is an error number from errno.h

There are already non-errp variant implementations that return -1 (e.g., 
dbus_vmstate_post_load). Adding errp variants that require to return 
errno is degradation.

I'm still not sure what conclusion will be drawn. I can make two 
opposite conclusions:

- Non-errp variants return int, so errp variants should also return int
   for local consistency.

- bool is all we need, but changing non-errp variants doesn't seem worth
   the bother, so only errp variants should return bool.

Please excuse the delayed response, I was on vacation.

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/08/09 23:30, Markus Armbruster wrote:
>> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

[...]

>>> There were three options on the table: bool, int, and void.
>>>
>>> The previous discussion you referred explains why void should be avoided, and include/qapi/error.h also says void should be avoided.
>>>
>>> There is pre_load() that does not use Error returns int, but now we are adding pre_load_errp() that uses Error.
>>>
>>> Then what pre_load_errp() should return: bool or int?
>>
>> I like bool when it's all we need.
>>
>> When we need to return a non-negative int on success, use int and return
>> -1 or a negative error code on failure.
>>
>> Another reason to pick int is local consistency: related functions
>> already use int.
>>
>> Changing working code from int to bool doesn't seem worth the bother.
>>
>> Questions?
>
> I at least see what "[PATCH v9 26/27] migration: Add error-parameterized function variants in VMSD struct" says is undesirable. It says:
>
>> For the errp variants,
>> Returns: 0 on success,
>>          <0 on error where -value is an error number from errno.h

Does any caller use errno codes to discriminate between failures?

> There are already non-errp variant implementations that return -1 (e.g., dbus_vmstate_post_load). Adding errp variants that require to return errno is degradation.

However, the non-errp variants are intended to be removed.  Any
inconsistency with them would hopefully be temporary.  We can accept
that when other considerations call for it.

> I'm still not sure what conclusion will be drawn. I can make two opposite conclusions:
>
> - Non-errp variants return int, so errp variants should also return int
>   for local consistency.
>
> - bool is all we need, but changing non-errp variants doesn't seem worth
>   the bother, so only errp variants should return bool.

Here's my recommendation.

If any caller needs to use errno codes, use int and return -errno on
failure.  This is inconsistent with the old callbacks, but callers'
needs trump that.

Else if we want consistency with the old callbacks even though we intend
to remove them, use int and return -1 on failure.

Else use bool and return false on failure.