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

[Hanna Czenczek]

On 15.10.24 18:06, Peter Xu wrote:
> 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?

Not just perhaps, that seems kind of vital indeed...

(I blame my brain’s less-than-stellar pattern matching that might have 
interpreted this as the error path *cough*)

> Other than that it looks good.

Thanks for the quick review!  Will respin.

Hanna

> 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