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().
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.
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.
+ 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);
}
+ } 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.