Re: [PATCH] migration: Ensure vmstate_save() sets errp

[Peter Xu]

On Tue, Oct 15, 2024 at 04:15:15PM +0200, Hanna Czenczek wrote:
> migration/savevm.c contains some calls to vmstate_save() that are
> followed by migrate_set_error() if the integer return value indicates an
> error.  migrate_set_error() requires that the `Error *` object passed to
> it is set.  Therefore, vmstate_save() is assumed to always set *errp on
> error.
> 
> Right now, that assumption is not met: vmstate_save_state_v() (called
> internally by vmstate_save()) will not set *errp if
> vmstate_subsection_save() or vmsd->post_save() fail.  Fix that by adding
> an *errp parameter to vmstate_subsection_save(), and by generating a
> generic error in case post_save() fails (as is already done for
> pre_save()).
> 
> Without this patch, qemu will crash after vmstate_subsection_save() or
> post_save() have failed inside of a vmstate_save() call (unless
> migrate_set_error() then happen to discard the new error because
> s->error is already set).  This happens e.g. when receiving the state
> from a virtio-fs back-end (virtiofsd) fails.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  migration/vmstate.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index ff5d589a6d..13532f2807 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -22,7 +22,8 @@
>  #include "trace.h"
>  
>  static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
> *vmsd,
> -                                   void *opaque, JSONWriter *vmdesc);
> +                                   void *opaque, JSONWriter *vmdesc,
> +                                   Error **errp);
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription 
> *vmsd,
>                                     void *opaque);
>  
> @@ -441,12 +442,13 @@ int vmstate_save_state_v(QEMUFile *f, const 
> VMStateDescription *vmsd,
>          json_writer_end_array(vmdesc);
>      }
>  
> -    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> +    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
>  
>      if (vmsd->post_save) {
>          int ps_ret = vmsd->post_save(opaque);
>          if (!ret) {

Perhaps here it needs to be "if (!ret && ps_ret)" now, otherwise the error
will be attached even if no error for both retvals?

Other than that it looks good.

Thanks,

>              ret = ps_ret;
> +            error_setg(errp, "post-save failed: %s", vmsd->name);
>          }
>      }
>      return ret;
> @@ -518,7 +520,8 @@ static int vmstate_subsection_load(QEMUFile *f, const 
> VMStateDescription *vmsd,
>  }
>  
>  static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription 
> *vmsd,
> -                                   void *opaque, JSONWriter *vmdesc)
> +                                   void *opaque, JSONWriter *vmdesc,
> +                                   Error **errp)
>  {
>      const VMStateDescription * const *sub = vmsd->subsections;
>      bool vmdesc_has_subsections = false;
> @@ -546,7 +549,7 @@ static int vmstate_subsection_save(QEMUFile *f, const 
> VMStateDescription *vmsd,
>              qemu_put_byte(f, len);
>              qemu_put_buffer(f, (uint8_t *)vmsdsub->name, len);
>              qemu_put_be32(f, vmsdsub->version_id);
> -            ret = vmstate_save_state(f, vmsdsub, opaque, vmdesc);
> +            ret = vmstate_save_state_with_err(f, vmsdsub, opaque, vmdesc, 
> errp);
>              if (ret) {
>                  return ret;
>              }
> -- 
> 2.45.2
> 

-- 
Peter Xu