Re: [PATCH v3 03/18] migration: make .post_save() a void function

[Fabiano Rosas <farosas@suse.de>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> 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>
> ---
>  docs/devel/migration/main.rst |  2 +-
>  hw/ppc/spapr_pci.c            |  3 +--
>  include/migration/vmstate.h   |  8 +++++++-
>  migration/savevm.c            |  3 +--
>  migration/vmstate.c           | 12 +++---------
>  target/arm/machine.c          |  4 +---
>  6 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> index 234d280249a..2de70507640 100644
> --- 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 ea998bdff15..1dc3b02659f 100644
> --- 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 3695afd483f..85838a49aee 100644
> --- 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);

+CC Paolo, Zhao

This change breaks the rust build:
https://gitlab.com/farosas/qemu/-/jobs/13393119755

  error[E0308]: mismatched types
     --> ../rust/migration/src/vmstate.rs:605:18
      |
  605 |             Some(vmstate_no_version_cb::<T, F>)
      |             ---- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found fn item
      |             |
      |             arguments to this enum variant are incorrect
      |
      = note: expected fn pointer `unsafe extern "C" fn(_) -> ()`
                    found fn item `unsafe extern "C" fn(_) -> i32 {vmstate_no_version_cb::<T, F, impl Into<Errno>>}`


Is something like the following diff the right fix? It'd be nice to get
an ack so this series doesn't miss the freeze. (CI run:
https://gitlab.com/farosas/qemu/-/pipelines/2369688593)

-- >8 --
From 90a7b53d2b0d53692df1ef2cfe7607b13e4961d8 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Fri, 6 Mar 2026 18:53:14 -0300
Subject: [PATCH] fixup! migration: make .post_save() a void function

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 rust/migration/src/migratable.rs |  3 +--
 rust/migration/src/vmstate.rs    | 12 +++++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/rust/migration/src/migratable.rs b/rust/migration/src/migratable.rs
index 7748aac2f2..7088c1b156 100644
--- a/rust/migration/src/migratable.rs
+++ b/rust/migration/src/migratable.rs
@@ -406,10 +406,9 @@ fn pre_save(&self) -> Result<(), InvalidError> {
         Ok(())
     }
 
-    fn post_save(&self) -> Result<(), InvalidError> {
+    fn post_save(&self) {
         let state = unsafe { Box::from_raw(self.migration_state.replace(ptr::null_mut())) };
         drop(state);
-        Ok(())
     }
 
     fn pre_load(&self) -> Result<(), InvalidError> {
diff --git a/rust/migration/src/vmstate.rs b/rust/migration/src/vmstate.rs
index edc7c70265..f34a36f680 100644
--- a/rust/migration/src/vmstate.rs
+++ b/rust/migration/src/vmstate.rs
@@ -492,6 +492,11 @@ fn from(_value: InvalidError) -> Errno {
     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 @@ pub const fn pre_save<F: for<'a> FnCall<(&'a T,), Result<(), impl Into<Errno>>>>
     }
 
     #[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
         };
-- 
2.51.0