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