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

[Arun Menon <armenon@redhat.com>]

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