Hi Akihiko,
Thanks for the review.
On Sat, Jul 26, 2025 at 09:48:12PM +0900, Akihiko Odaki wrote:
> On 2025/07/25 21:19, 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 | 62
> > +++++++++++++++++++++++++++++++++++++++------
> > 2 files changed, 65 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index
> > 056781b1c21e737583f081594d9f88b32adfd674..53fa72c1bbde399be02c88fc8745fdbb79bfd7c8
> > 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -200,15 +200,26 @@ struct VMStateDescription {
> > * exclusive. For this reason, also early_setup VMSDs are migrated in
> > a
> > * QEMU_VM_SECTION_FULL section, while save_setup() data is migrated
> > in
> > * a QEMU_VM_SECTION_START section.
> > + *
> > + * There are duplicate impls of the post/pre save/load hooks.
> > + * 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.
> > */
> > +
> > bool early_setup;
> > int version_id;
> > int minimum_version_id;
> > MigrationPriority priority;
> > int (*pre_load)(void *opaque);
> > + int (*pre_load_errp)(void *opaque, Error **errp);
> > int (*post_load)(void *opaque, int version_id);
> > + int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> > int (*pre_save)(void *opaque);
> > + int (*pre_save_errp)(void *opaque, Error **errp);
> > int (*post_save)(void *opaque);
> > + int (*post_save_errp)(void *opaque, Error **errp);
> > bool (*needed)(void *opaque);
> > bool (*dev_unplug_pending)(void *opaque);
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index
> > bb5e9bf38d6ee7619ceb3e9da29209581c3c12eb..e427ef49b2b1991b0a3cdb14d641c197e00014b0
> > 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -152,7 +152,16 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
> > return -EINVAL;
> > }
> > - if (vmsd->pre_load) {
> > + if (vmsd->pre_load_errp) {
> > + ret = vmsd->pre_load_errp(opaque, errp);
> > + if (ret) {
> > + error_prepend(errp, "VM pre load failed for: '%s', "
> > + "version_id: '%d', minimum version_id: '%d', "
> > + "ret: %d: ", vmsd->name, vmsd->version_id,
> > + vmsd->minimum_version_id, ret);
> > + return ret;
> > + }
> > + } else if (vmsd->pre_load) {
> > ret = vmsd->pre_load(opaque);
> > if (ret) {
> > error_setg(errp, "VM pre load failed for: '%s', "
> > @@ -236,7 +245,14 @@ int vmstate_load_state(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > qemu_file_set_error(f, ret);
> > return ret;
> > }
> > - if (vmsd->post_load) {
> > + if (vmsd->post_load_errp) {
> > + ret = vmsd->post_load_errp(opaque, version_id, errp);
> > + if (ret < 0) {
> > + error_prepend(errp, "VM Post load failed for: %s, version_id:
> > %d, "
> > + "minimum_version: %d, ret: %d: ", vmsd->name,
> > + vmsd->version_id, vmsd->minimum_version_id, ret);
> > + }
> > + } else if (vmsd->post_load) {
> > ret = vmsd->post_load(opaque, version_id);
> > if (ret < 0) {
> > error_setg(errp,
> > @@ -412,11 +428,19 @@ int vmstate_save_state_v(QEMUFile *f, const
> > VMStateDescription *vmsd,
> > void *opaque, JSONWriter *vmdesc, int
> > version_id, Error **errp)
> > {
> > int ret = 0;
> > + Error *local_err = NULL;
> > const VMStateField *field = vmsd->fields;
> > trace_vmstate_save_state_top(vmsd->name);
> > - if (vmsd->pre_save) {
> > + if (vmsd->pre_save_errp) {
> > + ret = vmsd->pre_save_errp(opaque, errp);
> > + trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > + if (ret) {
> > + error_prepend(errp, "pre-save failed: %s: ", vmsd->name);
> > + return ret;
> > + }
> > + } else if (vmsd->pre_save) {
> > ret = vmsd->pre_save(opaque);
> > trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> > if (ret) {
> > @@ -524,10 +548,22 @@ 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) {
> > - vmsd->post_save(opaque);
> > + if (*errp == NULL) {
>
> include/qapi/error.h says:
> > * - You may pass NULL to not receive the error, &error_abort to abort
> > * on error, &error_fatal to exit(1) on error, or a pointer to a
> > * variable containing NULL to receive the error.
>
> > * - The function may pass @errp to functions it calls to pass on
> > * their errors to its caller. If it dereferences @errp to check
> > * for errors, it must use ERRP_GUARD().
Yes, we can add an ERRP_GUARD() before the null check (*errp == NULL), iff we
need a check, more on that below.
>
> I also think this change is irrelevant with the addition of the new 'errp'
> variants; it fixes an assertion failure when vmstate_save_state_v() failed
> and set errp. It is not a new problem caused by the 'errp' variants.
>
You are right.
vmstate_save_state() function, inturn calls vmstate_save_state_v() with NULL
errp.
So that can be ignored for now, as the errors will not be set in errp at all.
Calling error_setg() exclusively later, will not cause any trouble.
On the other hand,
vmstate_save_state_v() will set errp.
And I thought re-setting it here, using error_setg() will crash QEMU.
commit 848a0503422d043d541130d5e3e2f7bc147cdef9 introduced error_setg()
in place of error_report()
However, on review, I do think nothing has changed indeed as far as
this patchset is concerned. I do wonder though, how resetting this
does not crash QEMU. Maybe I am missing something.
> If that's true, this change should have its own explanation in the patch
> message, and also be split into another patch as "a commit message that
> mentions 'Also, ...' is often a good candidate for splitting into multiple
> patches." (docs/devel/submitting-a-patch.rst)
>
> > + error_setg(errp, "Save of field %s/%s failed",
> > + vmsd->name, field->name);
> > + }
> > + if (vmsd->post_save_errp) {
> > + int ps_ret = vmsd->post_save_errp(opaque,
> > &local_err);
> > + if (ps_ret < 0) {
>
> The error checks else this and next ps_ret checks only care if it's zero or
> not, but this checks for the sign, leading to inconsistency.
Yes, I shall keep it consistent, a non-zero value means failure. No need of
sign check.
>
> > + error_free_or_abort(errp);
> > + error_propagate(errp, local_err);
> > + ret = ps_ret;
> > + }
> > + } else if (vmsd->post_save) {
> > + int ps_ret = vmsd->post_save(opaque);
> > + if (ps_ret < 0) {
> > + ret = ps_ret;
> > + }
> > }
> > return ret;
> > }
> > @@ -554,7 +590,17 @@ 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);
> > + } else if (ret && ps_ret) {
> > + error_free_or_abort(errp);
> > + error_propagate(errp, local_err);
> > + ret = ps_ret;
> > + }
>
> Simpler:
>
> if (ps_ret) {
> if (ret) {
> error_free_or_abort(errp);
> }
> ret = ps_ret;
> error_propagate(errp, local_err);
> }
>
Will do.
> > + } else if (vmsd->post_save) {
> > int ps_ret = vmsd->post_save(opaque);
> > if (!ret && ps_ret) {
> > ret = ps_ret;
>
> When there is a preceding error, this code still returns it and dismisses
> the post_save() error although the other part of this function is changed to
> propagate the error of post-save unconditionally. Please keep them
> consistent.
I do feel that in the new implementation (post_save_errp), we should be
dismissing
the preceeding error and propagating the new error from post_save().
Because, that way, it makes sense to run the post_save_errp() part even after
encountering
an error in the preceeding section (vmstate_subsection_save).
Not sure if I should change the old impl post_save to match post_save_errp,
Or should I match the new impl with the old one.
>
> >
>
Regards,
Arun
