On 2025/08/06 3:25, 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>
---
docs/devel/migration/main.rst | 24 ++++++++++++++++++++++++
include/migration/vmstate.h | 15 +++++++++++++++
migration/vmstate.c | 34 ++++++++++++++++++++++++++++++----
3 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index
6493c1d2bca48a2fa34d92f6c0979c215c56b8d5..35bf5ae26c87f3c82964eb15618be373ab5a41fc
100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -444,6 +444,30 @@ The functions to do that are inside a vmstate definition,
and are called:
This function is called after we save the state of one device
(even upon failure, unless the call to pre_save returned an error).
+Following are the errp variants of these functions.
+
+- ``int (*pre_load_errp)(void *opaque, Error **errp);``
+
+ This function is called before we load the state of one device.
+
+- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
+
+ This function is called after we load the state of one device.
+
+- ``int (*pre_save_errp)(void *opaque, Error **errp);``
+
+ This function is called before we save the state of one device.
+
+- ``int (*post_save_errp)(void *opaque, Error **errp);``
+
+ This function is called after we save the state of one device
+ (even upon failure, unless the call to pre_save returned an error).
Reviewing "[PATCH v9 24/27] migration: Propagate last encountered error
in vmstate_save_state_v() function", I wondered why it has never been a
problem post_load(), and this says only post_save_errp() is called upon
failure.
Now I suspect it may be better to clarify their differences and avoid
introducing post_save_errp() instead.
My guess is that post_save_errp() is being introduced for consistency
with post_load(), but they cannot have "consistency" if post_load() and
post_save() are not corresponding functions of save/load but are
independent two functions. Perhaps the true problem here is that
post_load() and post_save() look similar; if so, that can be solved by:
- Renaming post_save() to e.g., cleanup_save()
- Changing it to return void
+
+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.
+
Example: You can look at hpet.c, that uses the first three functions
to massage the state that is transferred.
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index
5fe9bbf39058d0cf97c1adab54cc516dbe8dc32a..51baf6c7f9d061ee33949d7e798f2184e50b4cbf
100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -200,15 +200,30 @@ 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.
+ *
+ * For the errp variants,
+ * Returns: 0 on success,
+ * <0 on error where -value is an error number from errno.h
*/
+
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
569e66ea983f833e6a0d651d2a751f34a64e8f5c..0acaa855cfec8ddac63e33d4562e39c345856213
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', "
@@ -242,7 +251,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,
@@ -411,8 +427,16 @@ int vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
static int pre_save_dispatch(const VMStateDescription *vmsd, void *opaque,
Error **errp)
{
+ ERRP_GUARD();
int ret = 0;
- 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 for %s failed, ret: %d: ",
+ vmsd->name, ret);
+ }
+ } else if (vmsd->pre_save) {
ret = vmsd->pre_save(opaque);
trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
if (ret) {
@@ -427,7 +451,9 @@ static int post_save_dispatch(const VMStateDescription
*vmsd, void *opaque,
Error **errp)
{
int ret = 0;
- if (vmsd->post_save) {
+ if (vmsd->post_save_errp) {
+ ret = vmsd->post_save_errp(opaque, errp);
+ } else if (vmsd->post_save) {
ret = vmsd->post_save(opaque);
error_setg(errp, "post-save for %s failed, ret: %d",
vmsd->name, ret);
