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

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

[Vladimir Sementsov-Ogievskiy]

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);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 3a16c467b25..c5236e71ba1 100644
--- 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 651c3fe0115..5111e7a141f 100644
--- 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/target/arm/machine.c b/target/arm/machine.c
index bbaae344492..de810220e25 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -993,15 +993,13 @@ 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;
 
     if (!kvm_enabled()) {
         pmu_op_finish(&cpu->env);
     }
-
-    return 0;
 }
 
 static int cpu_pre_load(void *opaque)
-- 
2.52.0

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

[Fabiano Rosas]

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

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

[noreply77-demartz@thinocorp.com]

[-- Attachment #1: Type: text/plain, Size: 7116 bytes --]

Hola,

Espero que este mensaje se encuentre bien.

Con respecto a la propuesta de Vladimir de cambiar .post_save() a una
función void en el proceso de migración, Fabiano ha señalado que este
cambio afecta actualmente la compilación en Rust.

Para evitar inconvenientes con el próximo "freeze", Fabiano ya ha sugerido
una solución técnica para corregir este error de compatibilidad. Quedamos a
la espera de sus comentarios o de su aprobación para proceder con los
ajustes necesarios y asegurar que la serie de parches pueda integrarse sin
problemas.

Saludos cordiales,

Jhosenad

Jhosenad@thinocorp.com

El vie, 6 de mar de 2026, 16:21, Fabiano Rosas <farosas@suse.de> escribió:

> 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
>
>
>

[-- Attachment #2: Type: text/html, Size: 9534 bytes --]

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

[Zhao Liu]

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

What about this?

fn post_save(&self) {
    let _ = unsafe { Box::from_raw(self.migration_state.replace(ptr::null_mut())) };
}

Others LGTM,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>