Hi
On Fri, Jul 25, 2025 at 4:22 PM Arun Menon <arme...@redhat.com> 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.
>
It is not documented, but I think all the callbacks return value should be
0 on success, <0 on error with -value an error number from errno.h. Could
you document it?
> */
> +
> 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);
>
You will also need to update docs/devel/migration/main.rst to reflect the
new callbacks
>
> 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);
>
(but we don't have error_prepend_errno, ok)
> + 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);
>
Here, it would be helpful to report "ret" too.
> + return ret;
> + }
> + } else if (vmsd->pre_save) {
>
Imho, you should try to introduce a new helper function to call the
implemented callback appropriately and handle the non-errp case:
int the_callback(vmsd,.. **errp)
{
ERRP_GUARD(); // mostly for consistency
if (vmsd->the_callback_errp) {
return vmsd->the_callback_errp(args.., errp);
} else if (vmsd->the_callback) {
ret =vmsd->the_callback(args...);
error_setg_errno(-ret, "the callback failed...")
return ret;
}
return 0;
}
This should make it a bit easier to deal with.
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) {
> + 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) {
> + 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);
>
It's a bit odd how post_save() is being used here. It could be worth to
document that it is called regardless of success of migration in callback
doc.
Imho, the function vmstate_save_state_v() should be refactored to call
pre_save() and post_save() and call an internal function in between. It
will also help to shrink the function a bit.
> + 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;
> + }
> + } else if (vmsd->post_save) {
> int ps_ret = vmsd->post_save(opaque);
> if (!ret && ps_ret) {
> ret = ps_ret;
>
> --
> 2.50.0
>
>
