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 <[email protected]>
> > ---
> > 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
