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 <arme...@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
