[PATCH v14 25/27] migration: Rename post_save() to cleanup_save() and make it void

Arun Menon posted 27 patches 4 months, 3 weeks ago
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Hailiang Zhang <zhanghailiang@xfusion.com>, Steve Sistare <steven.sistare@oracle.com>, Peter Maydell <peter.maydell@linaro.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
[PATCH v14 25/27] migration: Rename post_save() to cleanup_save() and make it void
Posted by Arun Menon 4 months, 3 weeks ago
The post_save() function call is responsible for cleaning up resources
after the device state has been saved.
Currently it is infallible, and does not return an error.

It is called regardless of whether there is a preceding error from
save or subsection save. That is, save and post_save() together are
considered to be an atomic logical operation.

It should not be confused as a counterpart of the post_load() function
because post_load() does some sanity checks and returns an error if it
fails. This commit, therefore, renames post_save() to cleanup_save()
and makes it a void function.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Signed-off-by: Arun Menon <armenon@redhat.com>
---
 docs/devel/migration/main.rst |  2 +-
 hw/ppc/spapr_pci.c            |  5 ++---
 include/migration/vmstate.h   |  2 +-
 migration/savevm.c            |  5 ++---
 migration/vmstate.c           | 12 ++++--------
 target/arm/machine.c          |  6 ++----
 6 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 6493c1d2bca48a2fa34d92f6c0979c215c56b8d5..a39fec2e21c26c4315c0cf13b105176d70679d4d 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 (*cleanup_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 1ac1185825e84ca908fd878f6cbe7e8cacac1d89..135265f075dff502af59fbc91babca1f9a26c54d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2118,14 +2118,13 @@ static int spapr_pci_pre_save(void *opaque)
     return 0;
 }
 
-static int spapr_pci_post_save(void *opaque)
+static void spapr_pci_cleanup_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)
@@ -2152,7 +2151,7 @@ static const VMStateDescription vmstate_spapr_pci = {
     .version_id = 2,
     .minimum_version_id = 2,
     .pre_save = spapr_pci_pre_save,
-    .post_save = spapr_pci_post_save,
+    .cleanup_save = spapr_pci_cleanup_save,
     .post_load = spapr_pci_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 5fe9bbf39058d0cf97c1adab54cc516dbe8dc32a..c1d8e5b7a7d9d544fc8ce181372660f5538ef66b 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -208,7 +208,7 @@ struct VMStateDescription {
     int (*pre_load)(void *opaque);
     int (*post_load)(void *opaque, int version_id);
     int (*pre_save)(void *opaque);
-    int (*post_save)(void *opaque);
+    void (*cleanup_save)(void *opaque);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 2c05b763148295d7e2095027aa143f45bc4c4676..693f77b728604f2072fb40bd2ec268b217da9727 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -322,14 +322,13 @@ static int configuration_pre_save(void *opaque)
     return 0;
 }
 
-static int configuration_post_save(void *opaque)
+static void configuration_cleanup_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)
@@ -544,7 +543,7 @@ static const VMStateDescription vmstate_configuration = {
     .pre_load = configuration_pre_load,
     .post_load = configuration_post_load,
     .pre_save = configuration_pre_save,
-    .post_save = configuration_post_save,
+    .cleanup_save = configuration_cleanup_save,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32(len, SaveState),
         VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
diff --git a/migration/vmstate.c b/migration/vmstate.c
index ad8e5b71ae2ce78e66a6426602e5c20405ec57c0..a7562a5cfd8627e20c90e286e35c5e3429c48150 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -529,8 +529,8 @@ 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) {
-                        vmsd->post_save(opaque);
+                    if (vmsd->cleanup_save) {
+                        vmsd->cleanup_save(opaque);
                     }
                     return ret;
                 }
@@ -557,12 +557,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 
     ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc, errp);
 
-    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);
-        }
+    if (vmsd->cleanup_save) {
+        vmsd->cleanup_save(opaque);
     }
     return ret;
 }
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 6666a0c50c45504082f02fab42f499ca58dc3597..cb6b9ee653915cd07b7d93e2c9ec63be1a3365a0 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -883,15 +883,13 @@ static int cpu_pre_save(void *opaque)
     return 0;
 }
 
-static int cpu_post_save(void *opaque)
+static void cpu_cleanup_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
     if (!kvm_enabled()) {
         pmu_op_finish(&cpu->env);
     }
-
-    return 0;
 }
 
 static int cpu_pre_load(void *opaque)
@@ -1028,7 +1026,7 @@ const VMStateDescription vmstate_arm_cpu = {
     .version_id = 22,
     .minimum_version_id = 22,
     .pre_save = cpu_pre_save,
-    .post_save = cpu_post_save,
+    .cleanup_save = cpu_cleanup_save,
     .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields = (const VMStateField[]) {

-- 
2.51.0
Re: [PATCH v14 25/27] migration: Rename post_save() to cleanup_save() and make it void
Posted by Peter Xu 4 months, 1 week ago
On Thu, Sep 18, 2025 at 08:53:42PM +0530, Arun Menon wrote:
> The post_save() function call is responsible for cleaning up resources
> after the device state has been saved.
> Currently it is infallible, and does not return an error.
> 
> It is called regardless of whether there is a preceding error from
> save or subsection save. That is, save and post_save() together are
> considered to be an atomic logical operation.
> 
> It should not be confused as a counterpart of the post_load() function
> because post_load() does some sanity checks and returns an error if it
> fails. This commit, therefore, renames post_save() to cleanup_save()
> and makes it a void function.
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> Signed-off-by: Arun Menon <armenon@redhat.com>

I'll need to drop this one patch because it breaks Rust.  Please feel free
to send it separately or just leave post_save() as-is for now.

PS: IMHO post_save() is still a good name to me, pairing well with
pre_save() and all *_load*() functions too.  Dropping the retval should
already imply it cannot fail with/without a name change (and also because
modules can do more than "cleanups" in post_save()..).

-- 
Peter Xu
Re: [PATCH v14 25/27] migration: Rename post_save() to cleanup_save() and make it void
Posted by Arun Menon 4 months, 1 week ago
Hi Peter,

On Mon, Sep 29, 2025 at 02:50:45PM -0400, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 08:53:42PM +0530, Arun Menon wrote:
> > The post_save() function call is responsible for cleaning up resources
> > after the device state has been saved.
> > Currently it is infallible, and does not return an error.
> > 
> > It is called regardless of whether there is a preceding error from
> > save or subsection save. That is, save and post_save() together are
> > considered to be an atomic logical operation.
> > 
> > It should not be confused as a counterpart of the post_load() function
> > because post_load() does some sanity checks and returns an error if it
> > fails. This commit, therefore, renames post_save() to cleanup_save()
> > and makes it a void function.
> > 
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Suggested-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> 
> I'll need to drop this one patch because it breaks Rust.  Please feel free
> to send it separately or just leave post_save() as-is for now.

Thank you for the review. I see it now.
I used the --enable-rust flag to change my configuration.
I have removed the patch and resent the series (v15)

> 
> PS: IMHO post_save() is still a good name to me, pairing well with
> pre_save() and all *_load*() functions too.  Dropping the retval should
> already imply it cannot fail with/without a name change (and also because
> modules can do more than "cleanups" in post_save()..).

Yes, for now I have kept things as is because we do not essentially need
to do something to the existing post_save() call; the dropping of return value can
be looked into in a future patch. Thank you.

> 
> -- 
> Peter Xu
> 

Regards,
Arun Menon
Re: [PATCH v14 25/27] migration: Rename post_save() to cleanup_save() and make it void
Posted by Peter Xu 4 months, 1 week ago
On Tue, Sep 30, 2025 at 09:59:59AM +0530, Arun Menon wrote:
> Thank you for the review. I see it now.
> I used the --enable-rust flag to change my configuration.
> I have removed the patch and resent the series (v15)

Next time feel free to avoid the repost. I've managed the rebase which is
trivial.  I'll explicitly ask for a repost if help needed.

Thanks,

-- 
Peter Xu