Re: [PATCH v6 23/24] migration: Add error-parameterized function variants in VMSD struct

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

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