* [PATCH v2 0/2] migration: Fix error propagation regression and memory leak
@ 2025-10-28 6:21 Arun Menon
2025-10-28 6:21 ` [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() Arun Menon
2025-10-28 6:21 ` [PATCH v2 2/2] migration: Fix memory leak in postcopy_ram_listen_thread() Arun Menon
0 siblings, 2 replies; 5+ messages in thread
From: Arun Menon @ 2025-10-28 6:21 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Akihiko Odaki, Dmitry Osipenko,
Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic,
Eric Farman, Christian Borntraeger, Matthew Rosato, Thomas Huth,
Richard Henderson, David Hildenbrand, Ilya Leoshkevich,
Paolo Bonzini, Fam Zheng, Nicholas Piggin, Harsh Prateek Bora,
Peter Maydell, Peter Xu, Fabiano Rosas, qemu-s390x, qemu-ppc,
Arun Menon
Hi,
This series fixes the following:
commit c632ffbd74 introduced a regression of passing error_fatal
to vmstate_load_state() and vmstate_save_state() functions.
This will not allow us to propagate the error back to the caller.
commit 94272d9b45 introduced a memory leak in postcopy_ram_listen_thread().
We need to free the local_err object in the if clause as well.
---
Changes in v2:
- Refactoring of the return statement in virtio_gpu_load().
- Add another patch to fix the memory leak in postcopy_ram_listen_thread()
- Link to v1: https://lore.kernel.org/qemu-devel/20251024-solve_error_fatal_regression-v1-1-d26e6cbd0a7e@redhat.com
---
Arun Menon (2):
migration: Fix regression of passing error_fatal into vmstate_load_state()
migration: Fix memory leak in postcopy_ram_listen_thread()
hw/display/virtio-gpu.c | 19 ++++++++++++++-----
hw/pci/pci.c | 13 +++++++++++--
hw/s390x/virtio-ccw.c | 15 +++++++++++++--
hw/scsi/spapr_vscsi.c | 9 +++++++--
hw/virtio/virtio-mmio.c | 15 +++++++++++++--
hw/virtio/virtio-pci.c | 15 +++++++++++++--
hw/virtio/virtio.c | 10 +++++++---
migration/savevm.c | 1 +
8 files changed, 79 insertions(+), 18 deletions(-)
---
base-commit: 36076d24f04ea9dc3357c0fbe7bb14917375819c
change-id: 20251024-solve_error_fatal_regression-301763debd8f
Best regards,
--
Arun Menon <armenon@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() 2025-10-28 6:21 [PATCH v2 0/2] migration: Fix error propagation regression and memory leak Arun Menon @ 2025-10-28 6:21 ` Arun Menon 2025-10-28 6:42 ` Akihiko Odaki 2025-10-28 6:21 ` [PATCH v2 2/2] migration: Fix memory leak in postcopy_ram_listen_thread() Arun Menon 1 sibling, 1 reply; 5+ messages in thread From: Arun Menon @ 2025-10-28 6:21 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Akihiko Odaki, Dmitry Osipenko, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Christian Borntraeger, Matthew Rosato, Thomas Huth, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Paolo Bonzini, Fam Zheng, Nicholas Piggin, Harsh Prateek Bora, Peter Maydell, Peter Xu, Fabiano Rosas, qemu-s390x, qemu-ppc, Arun Menon error_fatal is passed to vmstate_load_state() and vmstate_save_state() functions. This was introduced in commit c632ffbd74. This would exit(1) on error, and therefore does not allow to propagate the error back to the caller. To maintain consistency with prior error handling i.e. either propagating the error to the caller or reporting it, we must set the error within a local Error object instead of using error_fatal. Reviewed-by: Cornelia Huck <cohuck@redhat.com> Signed-off-by: Arun Menon <armenon@redhat.com> --- hw/display/virtio-gpu.c | 19 ++++++++++++++----- hw/pci/pci.c | 13 +++++++++++-- hw/s390x/virtio-ccw.c | 15 +++++++++++++-- hw/scsi/spapr_vscsi.c | 9 +++++++-- hw/virtio/virtio-mmio.c | 15 +++++++++++++-- hw/virtio/virtio-pci.c | 15 +++++++++++++-- hw/virtio/virtio.c | 10 +++++++--- 7 files changed, 78 insertions(+), 18 deletions(-) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 3a555125be60aa4c243cfb870caa517995de8183..63263ecc5bda889e5327aa59ada53cb41b0219cb 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1225,7 +1225,9 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, { VirtIOGPU *g = opaque; struct virtio_gpu_simple_resource *res; + Error *err = NULL; int i; + int ret = 0; /* in 2d mode we should never find unprocessed commands here */ assert(QTAILQ_EMPTY(&g->cmdq)); @@ -1248,8 +1250,12 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, } qemu_put_be32(f, 0); /* end of list */ - return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL, - &error_fatal); + ret = vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL, + &err); + if (ret < 0) { + error_report_err(err); + } + return ret; } static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g, @@ -1289,6 +1295,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, struct virtio_gpu_simple_resource *res; uint32_t resource_id, pformat; int i; + int ret = 0; g->hostmem = 0; @@ -1348,9 +1355,11 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, } /* load & apply scanout state */ - vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal); - - return 0; + ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err); + if (ret < 0) { + error_report_err(err); + } + return ret; } static int virtio_gpu_blob_save(QEMUFile *f, void *opaque, size_t size, diff --git a/hw/pci/pci.c b/hw/pci/pci.c index acc03fd4707cdb843ba8ed8ff0e2cc8c4830932c..0090c72560de313db160f71ff494d206859ec796 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -925,8 +925,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f) * in irq_state which we are saving. * This makes us compatible with old devices * which never set or clear this bit. */ + int ret; + Error *local_err = NULL; s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT; - vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal); + ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err); + if (ret < 0) { + error_report_err(local_err); + } /* Restore the interrupt status bit. */ pci_update_irq_status(s); } @@ -934,8 +939,12 @@ void pci_device_save(PCIDevice *s, QEMUFile *f) int pci_device_load(PCIDevice *s, QEMUFile *f) { int ret; + Error *local_err = NULL; ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id, - &error_fatal); + &local_err); + if (ret < 0) { + error_report_err(local_err); + } /* Restore the interrupt status bit. */ pci_update_irq_status(s); return ret; diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 4cb1ced001ae241c53c503ebfd7c90e336799c37..41c7d62a482de3c618e71dd07c0cd23e1bcd5578 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1130,13 +1130,24 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f) static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); - vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &error_fatal); + int ret; + Error *local_err = NULL; + ret = vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &local_err); + if (ret < 0) { + error_report_err(local_err); + } } static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); - return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal); + int ret; + Error *local_err = NULL; + ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err); + if (ret < 0) { + error_report_err(local_err); + } + return ret; } static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c index f0a7dd2b882a13deec4a4c6d2eb4aae6d2fdbeb9..af4debc2f8638a0b64b5701d3d15ee9c3966cea7 100644 --- a/hw/scsi/spapr_vscsi.c +++ b/hw/scsi/spapr_vscsi.c @@ -628,10 +628,15 @@ static const VMStateDescription vmstate_spapr_vscsi_req = { static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq) { vscsi_req *req = sreq->hba_private; + int rc; + Error *local_err = NULL; assert(req->active); - vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal); - + rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err); + if (rc < 0) { + error_report_err(local_err); + return; + } trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num, req->cur_desc_offset); } diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index fb58c36452730cfc92a0d26ff13e01e2d6654960..ffdda63e279fd1795a447cd32effe9dcdced6120 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -612,15 +612,26 @@ static const VMStateDescription vmstate_virtio_mmio = { static void virtio_mmio_save_extra_state(DeviceState *opaque, QEMUFile *f) { VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); + Error *local_err = NULL; + int ret; - vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &error_fatal); + ret = vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &local_err); + if (ret < 0) { + error_report_err(local_err); + } } static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f) { VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); + int ret; + Error *local_err = NULL; - return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal); + ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err); + if (ret < 0) { + error_report_err(local_err); + } + return ret; } static bool virtio_mmio_has_extra_state(DeviceState *opaque) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 937e22f08a2005d5d9e96764358a4afc09078613..f245f5c3c5e5d469e08e9e7a27f83496e90c8f59 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -187,15 +187,26 @@ static bool virtio_pci_has_extra_state(DeviceState *d) static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f) { VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); + int ret; + Error *local_err = NULL; - vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &error_fatal); + ret = vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &local_err); + if (ret < 0) { + error_report_err(local_err); + } } static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f) { VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); + int ret; + Error *local_err = NULL; - return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal); + ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err); + if (ret < 0) { + error_report_err(local_err); + } + return ret; } static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 153ee0a0cf1dd45c3e2246e431b696856d29b161..257cda506a40403ea1c0dbcc0de38b9854372193 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3030,7 +3030,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff); - int i; + int i, ret; Error *local_err = NULL; if (k->save_config) { @@ -3075,7 +3075,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) } if (vdc->vmsd) { - int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err); + ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err); if (ret) { error_report_err(local_err); return ret; @@ -3083,7 +3083,11 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) } /* Subsections */ - return vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &error_fatal); + ret = vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &local_err); + if (ret < 0) { + error_report_err(local_err); + } + return ret; } /* A wrapper for use as a VMState .put function */ -- 2.51.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() 2025-10-28 6:21 ` [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() Arun Menon @ 2025-10-28 6:42 ` Akihiko Odaki 2025-10-28 14:26 ` Peter Xu 0 siblings, 1 reply; 5+ messages in thread From: Akihiko Odaki @ 2025-10-28 6:42 UTC (permalink / raw) To: Arun Menon, qemu-devel Cc: Alex Bennée, Dmitry Osipenko, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Christian Borntraeger, Matthew Rosato, Thomas Huth, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Paolo Bonzini, Fam Zheng, Nicholas Piggin, Harsh Prateek Bora, Peter Maydell, Peter Xu, Fabiano Rosas, qemu-s390x, qemu-ppc On 2025/10/28 15:21, Arun Menon wrote: > error_fatal is passed to vmstate_load_state() and vmstate_save_state() > functions. This was introduced in commit c632ffbd74. This would exit(1) > on error, and therefore does not allow to propagate the error back to > the caller. > > To maintain consistency with prior error handling i.e. either propagating > the error to the caller or reporting it, we must set the error within a > local Error object instead of using error_fatal. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Signed-off-by: Arun Menon <armenon@redhat.com> > --- > hw/display/virtio-gpu.c | 19 ++++++++++++++----- > hw/pci/pci.c | 13 +++++++++++-- > hw/s390x/virtio-ccw.c | 15 +++++++++++++-- > hw/scsi/spapr_vscsi.c | 9 +++++++-- > hw/virtio/virtio-mmio.c | 15 +++++++++++++-- > hw/virtio/virtio-pci.c | 15 +++++++++++++-- > hw/virtio/virtio.c | 10 +++++++--- > 7 files changed, 78 insertions(+), 18 deletions(-) > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 3a555125be60aa4c243cfb870caa517995de8183..63263ecc5bda889e5327aa59ada53cb41b0219cb 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1225,7 +1225,9 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, > { > VirtIOGPU *g = opaque; > struct virtio_gpu_simple_resource *res; > + Error *err = NULL; > int i; > + int ret = 0; I prefer not to have a variable initialized unless necessary because it suppresses compiler warnings to tell accidental usage of variable. Besides, the code change for other files in this patch and the existing code in this file doesn't initialize such variables, making the code style inconsistent. That said, I feel it is too nitpicky so I give: Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> You may or may not resend this patch to drop the initialization. > > /* in 2d mode we should never find unprocessed commands here */ > assert(QTAILQ_EMPTY(&g->cmdq)); > @@ -1248,8 +1250,12 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, > } > qemu_put_be32(f, 0); /* end of list */ > > - return vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL, > - &error_fatal); > + ret = vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL, > + &err); > + if (ret < 0) { > + error_report_err(err); > + } > + return ret; > } > > static bool virtio_gpu_load_restore_mapping(VirtIOGPU *g, > @@ -1289,6 +1295,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, > struct virtio_gpu_simple_resource *res; > uint32_t resource_id, pformat; > int i; > + int ret = 0; > > g->hostmem = 0; > > @@ -1348,9 +1355,11 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, > } > > /* load & apply scanout state */ > - vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal); > - > - return 0; > + ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err); > + if (ret < 0) { > + error_report_err(err); > + } > + return ret; > } > > static int virtio_gpu_blob_save(QEMUFile *f, void *opaque, size_t size, > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index acc03fd4707cdb843ba8ed8ff0e2cc8c4830932c..0090c72560de313db160f71ff494d206859ec796 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -925,8 +925,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f) > * in irq_state which we are saving. > * This makes us compatible with old devices > * which never set or clear this bit. */ > + int ret; > + Error *local_err = NULL; > s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT; > - vmstate_save_state(f, &vmstate_pci_device, s, NULL, &error_fatal); > + ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > /* Restore the interrupt status bit. */ > pci_update_irq_status(s); > } > @@ -934,8 +939,12 @@ void pci_device_save(PCIDevice *s, QEMUFile *f) > int pci_device_load(PCIDevice *s, QEMUFile *f) > { > int ret; > + Error *local_err = NULL; > ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id, > - &error_fatal); > + &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > /* Restore the interrupt status bit. */ > pci_update_irq_status(s); > return ret; > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index 4cb1ced001ae241c53c503ebfd7c90e336799c37..41c7d62a482de3c618e71dd07c0cd23e1bcd5578 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1130,13 +1130,24 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f) > static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > - vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &error_fatal); > + int ret; > + Error *local_err = NULL; > + ret = vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > } > > static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > - return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal); > + int ret; > + Error *local_err = NULL; > + ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > + return ret; > } > > static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp) > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c > index f0a7dd2b882a13deec4a4c6d2eb4aae6d2fdbeb9..af4debc2f8638a0b64b5701d3d15ee9c3966cea7 100644 > --- a/hw/scsi/spapr_vscsi.c > +++ b/hw/scsi/spapr_vscsi.c > @@ -628,10 +628,15 @@ static const VMStateDescription vmstate_spapr_vscsi_req = { > static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq) > { > vscsi_req *req = sreq->hba_private; > + int rc; > + Error *local_err = NULL; > assert(req->active); > > - vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal); > - > + rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err); > + if (rc < 0) { > + error_report_err(local_err); > + return; > + } > trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num, > req->cur_desc_offset); > } > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > index fb58c36452730cfc92a0d26ff13e01e2d6654960..ffdda63e279fd1795a447cd32effe9dcdced6120 100644 > --- a/hw/virtio/virtio-mmio.c > +++ b/hw/virtio/virtio-mmio.c > @@ -612,15 +612,26 @@ static const VMStateDescription vmstate_virtio_mmio = { > static void virtio_mmio_save_extra_state(DeviceState *opaque, QEMUFile *f) > { > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); > + Error *local_err = NULL; > + int ret; > > - vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &error_fatal); > + ret = vmstate_save_state(f, &vmstate_virtio_mmio, proxy, NULL, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > } > > static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f) > { > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); > + int ret; > + Error *local_err = NULL; > > - return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal); > + ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > + return ret; > } > > static bool virtio_mmio_has_extra_state(DeviceState *opaque) > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 937e22f08a2005d5d9e96764358a4afc09078613..f245f5c3c5e5d469e08e9e7a27f83496e90c8f59 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -187,15 +187,26 @@ static bool virtio_pci_has_extra_state(DeviceState *d) > static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f) > { > VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); > + int ret; > + Error *local_err = NULL; > > - vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &error_fatal); > + ret = vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > } > > static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f) > { > VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); > + int ret; > + Error *local_err = NULL; > > - return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal); > + ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > + return ret; > } > > static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 153ee0a0cf1dd45c3e2246e431b696856d29b161..257cda506a40403ea1c0dbcc0de38b9854372193 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -3030,7 +3030,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) > VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev); > uint32_t guest_features_lo = (vdev->guest_features & 0xffffffff); > - int i; > + int i, ret; > Error *local_err = NULL; > > if (k->save_config) { > @@ -3075,7 +3075,7 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) > } > > if (vdc->vmsd) { > - int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err); > + ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err); > if (ret) { > error_report_err(local_err); > return ret; > @@ -3083,7 +3083,11 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f) > } > > /* Subsections */ > - return vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &error_fatal); > + ret = vmstate_save_state(f, &vmstate_virtio, vdev, NULL, &local_err); > + if (ret < 0) { > + error_report_err(local_err); > + } > + return ret; > } > > /* A wrapper for use as a VMState .put function */ > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() 2025-10-28 6:42 ` Akihiko Odaki @ 2025-10-28 14:26 ` Peter Xu 0 siblings, 0 replies; 5+ messages in thread From: Peter Xu @ 2025-10-28 14:26 UTC (permalink / raw) To: Akihiko Odaki Cc: Arun Menon, qemu-devel, Alex Bennée, Dmitry Osipenko, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Christian Borntraeger, Matthew Rosato, Thomas Huth, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Paolo Bonzini, Fam Zheng, Nicholas Piggin, Harsh Prateek Bora, Peter Maydell, Fabiano Rosas, qemu-s390x, qemu-ppc On Tue, Oct 28, 2025 at 03:42:54PM +0900, Akihiko Odaki wrote: > On 2025/10/28 15:21, Arun Menon wrote: > > error_fatal is passed to vmstate_load_state() and vmstate_save_state() > > functions. This was introduced in commit c632ffbd74. This would exit(1) > > on error, and therefore does not allow to propagate the error back to > > the caller. > > > > To maintain consistency with prior error handling i.e. either propagating > > the error to the caller or reporting it, we must set the error within a > > local Error object instead of using error_fatal. > > > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > > Signed-off-by: Arun Menon <armenon@redhat.com> > > --- > > hw/display/virtio-gpu.c | 19 ++++++++++++++----- > > hw/pci/pci.c | 13 +++++++++++-- > > hw/s390x/virtio-ccw.c | 15 +++++++++++++-- > > hw/scsi/spapr_vscsi.c | 9 +++++++-- > > hw/virtio/virtio-mmio.c | 15 +++++++++++++-- > > hw/virtio/virtio-pci.c | 15 +++++++++++++-- > > hw/virtio/virtio.c | 10 +++++++--- > > 7 files changed, 78 insertions(+), 18 deletions(-) > > > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index 3a555125be60aa4c243cfb870caa517995de8183..63263ecc5bda889e5327aa59ada53cb41b0219cb 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1225,7 +1225,9 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, > > { > > VirtIOGPU *g = opaque; > > struct virtio_gpu_simple_resource *res; > > + Error *err = NULL; > > int i; > > + int ret = 0; > > I prefer not to have a variable initialized unless necessary because it > suppresses compiler warnings to tell accidental usage of variable. > > Besides, the code change for other files in this patch and the existing code > in this file doesn't initialize such variables, making the code style > inconsistent. > > That said, I feel it is too nitpicky so I give: > > Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> > > You may or may not resend this patch to drop the initialization. I'll fix it when queuing, thanks both. While at it, I also touched a few other things on reordering of vars (follow rev-christmas tree), spacing, etc. The fixup is attached at the end. Please shoot if anyone disagrees. I'll skip patch 2 because we already have a fix here: https://lore.kernel.org/r/20251021220407.2662288-2-peterx@redhat.com Thanks, ===8<=== From 3cd8f712b5d18b3729dc78127598ebbb2b1afae2 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Tue, 28 Oct 2025 10:23:35 -0400 Subject: [PATCH] fixup! migration: Fix regression of passing error_fatal into vmstate_load_state() Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/display/virtio-gpu.c | 6 ++---- hw/pci/pci.c | 5 +++-- hw/s390x/virtio-ccw.c | 6 ++++-- hw/scsi/spapr_vscsi.c | 3 ++- hw/virtio/virtio-mmio.c | 2 +- hw/virtio/virtio-pci.c | 4 ++-- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 63263ecc5b..43e88a4daf 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1226,8 +1226,7 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size, VirtIOGPU *g = opaque; struct virtio_gpu_simple_resource *res; Error *err = NULL; - int i; - int ret = 0; + int i, ret; /* in 2d mode we should never find unprocessed commands here */ assert(QTAILQ_EMPTY(&g->cmdq)); @@ -1294,8 +1293,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size, Error *err = NULL; struct virtio_gpu_simple_resource *res; uint32_t resource_id, pformat; - int i; - int ret = 0; + int i, ret; g->hostmem = 0; diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 0090c72560..ac16c9521d 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -921,12 +921,13 @@ const VMStateDescription vmstate_pci_device = { void pci_device_save(PCIDevice *s, QEMUFile *f) { + Error *local_err = NULL; + int ret; + /* Clear interrupt status bit: it is implicit * in irq_state which we are saving. * This makes us compatible with old devices * which never set or clear this bit. */ - int ret; - Error *local_err = NULL; s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT; ret = vmstate_save_state(f, &vmstate_pci_device, s, NULL, &local_err); if (ret < 0) { diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c index 41c7d62a48..4a3ffb84f8 100644 --- a/hw/s390x/virtio-ccw.c +++ b/hw/s390x/virtio-ccw.c @@ -1130,8 +1130,9 @@ static int virtio_ccw_load_queue(DeviceState *d, int n, QEMUFile *f) static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); - int ret; Error *local_err = NULL; + int ret; + ret = vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL, &local_err); if (ret < 0) { error_report_err(local_err); @@ -1141,8 +1142,9 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f) static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f) { VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); - int ret; Error *local_err = NULL; + int ret; + ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err); if (ret < 0) { error_report_err(local_err); diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c index af4debc2f8..a6591319db 100644 --- a/hw/scsi/spapr_vscsi.c +++ b/hw/scsi/spapr_vscsi.c @@ -628,8 +628,9 @@ static const VMStateDescription vmstate_spapr_vscsi_req = { static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq) { vscsi_req *req = sreq->hba_private; - int rc; Error *local_err = NULL; + int rc; + assert(req->active); rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err); diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c index ffdda63e27..c05c00bcd4 100644 --- a/hw/virtio/virtio-mmio.c +++ b/hw/virtio/virtio-mmio.c @@ -624,8 +624,8 @@ static void virtio_mmio_save_extra_state(DeviceState *opaque, QEMUFile *f) static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f) { VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); - int ret; Error *local_err = NULL; + int ret; ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err); if (ret < 0) { diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index f245f5c3c5..99cb30fe59 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -187,8 +187,8 @@ static bool virtio_pci_has_extra_state(DeviceState *d) static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f) { VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); - int ret; Error *local_err = NULL; + int ret; ret = vmstate_save_state(f, &vmstate_virtio_pci, proxy, NULL, &local_err); if (ret < 0) { @@ -199,8 +199,8 @@ static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f) static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f) { VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); - int ret; Error *local_err = NULL; + int ret; ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err); if (ret < 0) { -- 2.50.1 -- Peter Xu ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] migration: Fix memory leak in postcopy_ram_listen_thread() 2025-10-28 6:21 [PATCH v2 0/2] migration: Fix error propagation regression and memory leak Arun Menon 2025-10-28 6:21 ` [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() Arun Menon @ 2025-10-28 6:21 ` Arun Menon 1 sibling, 0 replies; 5+ messages in thread From: Arun Menon @ 2025-10-28 6:21 UTC (permalink / raw) To: qemu-devel Cc: Alex Bennée, Akihiko Odaki, Dmitry Osipenko, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Christian Borntraeger, Matthew Rosato, Thomas Huth, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Paolo Bonzini, Fam Zheng, Nicholas Piggin, Harsh Prateek Bora, Peter Maydell, Peter Xu, Fabiano Rosas, qemu-s390x, qemu-ppc, Arun Menon commit 94272d9b45 introduced a memory leak in the postcopy_ram_listen_thread() call. We need to free the local error object in the if clause as well. Signed-off-by: Arun Menon <armenon@redhat.com> --- migration/savevm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/savevm.c b/migration/savevm.c index 7b35ec4dd007c6ed494201be8528a9e4f1c13843..f18c6ee659c1b9a005ff8ccfe51875f81d37694b 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2141,6 +2141,7 @@ static void *postcopy_ram_listen_thread(void *opaque) "bitmaps may be lost, and present migrated dirty " "bitmaps are correctly migrated and valid.", __func__, load_res); + error_free(local_err); load_res = 0; /* prevent further exit() */ } else { error_prepend(&local_err, -- 2.51.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-10-28 14:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-28 6:21 [PATCH v2 0/2] migration: Fix error propagation regression and memory leak Arun Menon 2025-10-28 6:21 ` [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state() Arun Menon 2025-10-28 6:42 ` Akihiko Odaki 2025-10-28 14:26 ` Peter Xu 2025-10-28 6:21 ` [PATCH v2 2/2] migration: Fix memory leak in postcopy_ram_listen_thread() Arun Menon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).