migration: make .post_save() a void function

All other handlers now have _errp() variants. Should we go this way
for .post_save()? Actually it's rather strange, when the vmstate do
successful preparations in .pre_save(), then successfully save all
sections and subsections, end then fail when all the state is
successfully transferred to the target.

Happily, we have only three .post_save() realizations, all always
successful. Let's make this a rule.

Also note, that we call .post_save() in two places, and handle
its (theoretical) failure inconsistently. Fix that too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com> #rust
Link: https://lore.kernel.org/qemu-devel/20260304212303.667141-4-vsementsov@yandex-team.ru
Signed-off-by: Fabiano Rosas <farosas@suse.de>

diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 234d280249a0be5c42538cc44a26b8a229b86783..2de70507640fb164eb6cd133cc9cf14dd574945d 100644 (file)
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -439,7 +439,7 @@ The functions to do that are inside a vmstate definition, and are called:
 
   This function is called before we save the state of one device.
 
-- ``int (*post_save)(void *opaque);``
+- ``void (*post_save)(void *opaque);``
 
   This function is called after we save the state of one device
   (even upon failure, unless the call to pre_save returned an error).

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ea998bdff15ccfc90e2d988541e0faeba589e745..1dc3b02659fa7066dd362304346af8202a50e9de 100644 (file)
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2093,14 +2093,13 @@ static int spapr_pci_pre_save(void *opaque)
     return 0;
 }
 
-static int spapr_pci_post_save(void *opaque)
+static void spapr_pci_post_save(void *opaque)
 {
     SpaprPhbState *sphb = opaque;
 
     g_free(sphb->msi_devs);
     sphb->msi_devs = NULL;
     sphb->msi_devs_num = 0;
-    return 0;
 }
 
 static int spapr_pci_post_load(void *opaque, int version_id)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 78d97151bd509b2577d3753bec629bc1ba18e85a..9d42cf7a64dcf7b57b6fb05741d7b5bb48ad1e36 100644 (file)
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -223,7 +223,13 @@ struct VMStateDescription {
     bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
     int (*pre_save)(void *opaque);
     bool (*pre_save_errp)(void *opaque, Error **errp);
-    int (*post_save)(void *opaque);
+
+    /*
+     * Unless .pre_save() fails, .post_save() is called after saving
+     * fields and subsections. It should not fail because at this
+     * point the state has potentially already been transferred.
+     */
+    void (*post_save)(void *opaque);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
 

diff --git a/migration/savevm.c b/migration/savevm.c
index dd58f2a70510cf548cba0ca09bfae9ae82c01c65..699d2c9f8b963fdfaebe94431c521e419767f48b 100644 (file)
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -321,14 +321,13 @@ static int configuration_pre_save(void *opaque)
     return 0;
 }
 
-static int configuration_post_save(void *opaque)
+static void configuration_post_save(void *opaque)
 {
     SaveState *state = opaque;
 
     g_free(state->capabilities);
     state->capabilities = NULL;
     state->caps_count = 0;
-    return 0;
 }
 
 static int configuration_pre_load(void *opaque)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 651c3fe0115a9730a726eed15dacecd2d5dfb64b..5111e7a141fbf8e77a2a6e61ed4e4d5c52a9a44f 100644 (file)
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -550,10 +550,7 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 if (ret) {
                     error_prepend(errp, "Save of field %s/%s failed: ",
                                   vmsd->name, field->name);
-                    if (vmsd->post_save) {
-                        vmsd->post_save(opaque);
-                    }
-                    return ret;
+                    goto out;
                 }
 
                 /* Compressed arrays only care about the first element */
@@ -578,12 +575,9 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
     ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
 
+out:
     if (vmsd->post_save) {
-        int ps_ret = vmsd->post_save(opaque);
-        if (!ret && ps_ret) {
-            ret = ps_ret;
-            error_setg(errp, "post-save failed: %s", vmsd->name);
-        }
+        vmsd->post_save(opaque);
     }
     return ret;
 }

diff --git a/rust/migration/src/migratable.rs b/rust/migration/src/migratable.rs
index 7748aac2f27d7bda070d31b63b37ce578cd15b8e..b9e5e1fc15dfbb18cf354ea242d63fd56115a409 100644 (file)
--- a/rust/migration/src/migratable.rs
+++ b/rust/migration/src/migratable.rs
@@ -406,10 +406,8 @@ impl<T: ToMigrationStateShared> Migratable<T> {
         Ok(())
     }
 
-    fn post_save(&self) -> Result<(), InvalidError> {
-        let state = unsafe { Box::from_raw(self.migration_state.replace(ptr::null_mut())) };
-        drop(state);
-        Ok(())
+    fn post_save(&self) {
+        let _ = unsafe { Box::from_raw(self.migration_state.replace(ptr::null_mut())) };
     }
 
     fn pre_load(&self) -> Result<(), InvalidError> {

diff --git a/rust/migration/src/vmstate.rs b/rust/migration/src/vmstate.rs
index edc7c7026568665d0f3100a53f3c65e70e1bef6f..f34a36f68093e9fb45d9d2876844866949347049 100644 (file)
--- a/rust/migration/src/vmstate.rs
+++ b/rust/migration/src/vmstate.rs
@@ -492,6 +492,11 @@ unsafe extern "C" fn vmstate_no_version_cb<
     into_neg_errno(result)
 }
 
+unsafe extern "C" fn vmstate_post_save_cb<T, F: for<'a> FnCall<(&'a T,), ()>>(opaque: *mut c_void) {
+    // SAFETY: the function is used in T's implementation of VMState.
+    F::call((unsafe { &*(opaque.cast::<T>()) },));
+}
+
 unsafe extern "C" fn vmstate_post_load_cb<
     T,
     F: for<'a> FnCall<(&'a T, u8), Result<(), impl Into<Errno>>>,
@@ -597,12 +602,9 @@ impl<T> VMStateDescriptionBuilder<T> {
     }
 
     #[must_use]
-    pub const fn post_save<F: for<'a> FnCall<(&'a T,), Result<(), impl Into<Errno>>>>(
-        mut self,
-        _f: &F,
-    ) -> Self {
+    pub const fn post_save<F: for<'a> FnCall<(&'a T,), ()>>(mut self, _f: &F) -> Self {
         self.0.post_save = if F::IS_SOME {
-            Some(vmstate_no_version_cb::<T, F>)
+            Some(vmstate_post_save_cb::<T, F>)
         } else {
             None
         };

diff --git a/target/arm/machine.c b/target/arm/machine.c
index b0e499515cf7351d533ff8df823f38131d011bf2..50d80ffb68c84325526dc6ae66bc9c7765bde6da 100644 (file)
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -998,7 +998,7 @@ static int cpu_pre_save(void *opaque)
     return 0;
 }
 
-static int cpu_post_save(void *opaque)
+static void cpu_post_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
@@ -1008,8 +1008,6 @@ static int cpu_post_save(void *opaque)
 
     cpu->cpreg_vmstate_indexes = NULL;
     cpu->cpreg_vmstate_values = NULL;
-
-    return 0;
 }
 
 static int cpu_pre_load(void *opaque)