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.
>
> > + } 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
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.
>
> > + } 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
