[PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

[PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

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

Signed-off-by: Arun Menon <armenon@redhat.com>
---
 hw/display/virtio-gpu.c | 20 ++++++++++++++------
 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(+), 19 deletions(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 3a555125be60aa4c243cfb870caa517995de8183..c0c88c283e2ef7fa70b3fecc1e935cca76423276 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1225,7 +1225,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
 {
     VirtIOGPU *g = opaque;
     struct virtio_gpu_simple_resource *res;
-    int i;
+    Error *err = NULL;
+    int i, ret;
 
     /* in 2d mode we should never find unprocessed commands here */
     assert(QTAILQ_EMPTY(&g->cmdq));
@@ -1248,8 +1249,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,
@@ -1288,7 +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 i, ret;
 
     g->hostmem = 0;
 
@@ -1348,8 +1353,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);
-
+    ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
+    if (ret < 0) {
+        error_report_err(err);
+        return ret;
+    }
     return 0;
 }
 
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 */

---
base-commit: 88b1716a407459c8189473e4667653cb8e4c3df7
change-id: 20251024-solve_error_fatal_regression-301763debd8f

Best regards,
-- 
Arun Menon <armenon@redhat.com>

Re: [PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Akihiko Odaki]

On 2025/10/25 1:32, 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.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>   hw/display/virtio-gpu.c | 20 ++++++++++++++------
>   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(+), 19 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 3a555125be60aa4c243cfb870caa517995de8183..c0c88c283e2ef7fa70b3fecc1e935cca76423276 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1225,7 +1225,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
>   {
>       VirtIOGPU *g = opaque;
>       struct virtio_gpu_simple_resource *res;
> -    int i;
> +    Error *err = NULL;
> +    int i, ret;
>   
>       /* in 2d mode we should never find unprocessed commands here */
>       assert(QTAILQ_EMPTY(&g->cmdq));
> @@ -1248,8 +1249,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,
> @@ -1288,7 +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 i, ret;
>   
>       g->hostmem = 0;
>   
> @@ -1348,8 +1353,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);
> -
> +    ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> +    if (ret < 0) {
> +        error_report_err(err);
> +        return ret;
> +    }
>       return 0;

virtio_gpu_save() always "return ret" instead of having "return ret" and 
"return 0"; this function can do the same.

>   }
>   
> 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 */
> 
> ---
> base-commit: 88b1716a407459c8189473e4667653cb8e4c3df7
> change-id: 20251024-solve_error_fatal_regression-301763debd8f
> 
> Best regards,

Re: [PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Cornelia Huck]

On Fri, Oct 24 2025, Arun Menon <armenon@redhat.com> 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.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  hw/display/virtio-gpu.c | 20 ++++++++++++++------
>  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(+), 19 deletions(-)
>

(...)

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

I was wondering whether it would make sense to pass the error object in
VirtioBusClass's ->{load,save}_config instead, but it seems virtio-ccw
is the only one using that pattern there (virtio-mmio and virtio-pci are
doing that dance in their ->{load,save}_extra_state callbacks); so let's
just do it this way and think about more error object passing later.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Arun Menon]

Hi Cornelia,

On Mon, Oct 27, 2025 at 12:18:58PM +0100, Cornelia Huck wrote:
> On Fri, Oct 24 2025, Arun Menon <armenon@redhat.com> 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.
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  hw/display/virtio-gpu.c | 20 ++++++++++++++------
> >  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(+), 19 deletions(-)
> >
> 
> (...)
> 
> > 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)
> 
> I was wondering whether it would make sense to pass the error object in
> VirtioBusClass's ->{load,save}_config instead, but it seems virtio-ccw
> is the only one using that pattern there (virtio-mmio and virtio-pci are
> doing that dance in their ->{load,save}_extra_state callbacks); so let's
> just do it this way and think about more error object passing later.
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thank you for the review.

> 

Regards,
Arun

Re: [PATCH] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Arun Menon]

Hi Akihiko,
Thanks for the review.

On Mon, Oct 27, 2025 at 09:37:52AM +0900, Akihiko Odaki wrote:
> On 2025/10/25 1:32, 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.
> > 
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >   hw/display/virtio-gpu.c | 20 ++++++++++++++------
> >   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(+), 19 deletions(-)
> > 
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 3a555125be60aa4c243cfb870caa517995de8183..c0c88c283e2ef7fa70b3fecc1e935cca76423276 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1225,7 +1225,8 @@ static int virtio_gpu_save(QEMUFile *f, void *opaque, size_t size,
> >   {
> >       VirtIOGPU *g = opaque;
> >       struct virtio_gpu_simple_resource *res;
> > -    int i;
> > +    Error *err = NULL;
> > +    int i, ret;
> >       /* in 2d mode we should never find unprocessed commands here */
> >       assert(QTAILQ_EMPTY(&g->cmdq));
> > @@ -1248,8 +1249,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,
> > @@ -1288,7 +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 i, ret;
> >       g->hostmem = 0;
> > @@ -1348,8 +1353,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);
> > -
> > +    ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> > +    if (ret < 0) {
> > +        error_report_err(err);
> > +        return ret;
> > +    }
> >       return 0;
> 
> virtio_gpu_save() always "return ret" instead of having "return ret" and
> "return 0"; this function can do the same.

Sure. Fixed in v2. Thanks.

> 
> >   }
> > 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. */

Regards,
Arun