On 2025/07/21 20:29, 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/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);
I think the new functions should have void as return value instead.
As I discussed before, I think having an integer return value is a
source of confusion:
https://lore.kernel.org/qemu-devel/0447e269-c242-4cd7-b68e-d0c721178...@rsg.ci.i.u-tokyo.ac.jp/
In the previous discussion, I suggested using bool, but void fits better
in this particular case.
include/qapi/error.h says:
> Whenever practical, also return a value that indicates success /
> failure. This can make the error checking more concise, and can avoid
> useless error object creation and destruction. Note that we still
> have many functions returning void.
There will be more implementations of these function pointers than their
callers, so it makes more sense to let return void and make
implementations more concise while making the callers less so. There is
also DeviceRealize, an example of function pointer type that takes errp
but returns void.
bool (*needed)(void *opaque);
bool (*dev_unplug_pending)(void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index
288b57e1ed778cce21247b64d5e97dfef41ad586..d96908d12ccffaef421e5d399a48e26cada2cb77
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,10 +245,17 @@ 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, "VM Post load failed for: %s, version_id: %d,"
+ error_setg(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);
}
@@ -410,11 +426,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,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);
+ }
+ } 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);
+ }
+ } else if (vmsd->post_save) {
int ps_ret = vmsd->post_save(opaque);
if (!ret && ps_ret) {
ret = ps_ret;
