Re: [PATCH v2 1/2] migration: Fix regression of passing error_fatal into vmstate_load_state()

[Peter Xu <peterx@redhat.com>]

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