Re: [PATCH QEMU HCI-8.1 v3 12/28] migration: Fix regression of passing error_fatal into vmstate_load_state()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] migration: Error leaked when vmstate hooks return positive ret (ret < 0 check)
- [Low] migration: void vmstate save wrappers only report errors and leave QEMUFile unmarked
--

commit e28996c9e3a0b9aff33ccbdf09f181f304ec549c
Author: Arun Menon <armenon@redhat.com>

migration: Fix regression of passing error_fatal into vmstate_load_state()

This commit replaces error_fatal with a local Error object in the callers of
vmstate_load_state() and vmstate_save_state(). Failures are now reported with
error_report_err() and, where the caller can return a value, propagated.

Link: https://lore.kernel.org/r/20251028-solve_error_fatal_regression-v2-1-dab24c808a28@redhat.com

> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c

[ ... ]

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

[Severity: Low]
pci_device_save() is a void function. It only prints the error and carries
on to pci_update_irq_status().

vmstate_save_state_v() does not call qemu_file_set_error() on failure, so
the QEMUFile is not marked as failed here.

Can a failed nested save (a nonzero pre_save, put or post_save return) in
pci_device_save() leave a truncated device section in the stream while the
save still looks successful?

The same pattern is present in virtio_ccw_save_config(),
virtio_mmio_save_extra_state() and virtio_pci_save_extra_state().

[ ... ]

> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c

[ ... ]

> +    rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
> +    if (rc < 0) {
> +        error_report_err(local_err);
> +        return;
> +    }

[Severity: Low]
In vscsi_save_request(), the error is printed and then dropped. The
QEMUFile is not marked failed, and the caller cannot tell that the request
was not saved.

Should the failure be recorded on the stream, for example with
qemu_file_set_error(), so that this and the other void save helpers do not
hide it?

The commit message says the change is meant to keep "either propagating the
error to the caller or reporting it". That holds for the load paths,
virtio_gpu_save() and virtio_save(), but not for these void helpers.

[ ... ]

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c

[ ... ]

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

[Severity: Low]
The earlier vdc->vmsd call in virtio_save() tests "if (ret)". This new call
tests "ret < 0".

vmstate_save_state_v() calls error_setg(errp, ...) for any nonzero return
from pre_save, and vmstate_load_state() does the same for any nonzero
pre_load return. The load side also returns early on a nonzero
vmstate_subsection_load() result.

If one of those hooks returns a positive value, local_err is set but never
passed to error_report_err(). Can that leak the Error and leave the failure
unreported?

The same "ret < 0" check is used in pci_device_save(), pci_device_load(),
virtio_ccw_save_config(), virtio_ccw_load_config(),
virtio_mmio_save_extra_state(), virtio_mmio_load_extra_state(),
virtio_pci_save_extra_state(), virtio_pci_load_extra_state(),
virtio_gpu_save(), virtio_gpu_load() and vscsi_save_request().

I did not find an in-tree hook that returns a positive value. This looks
like a conditional problem, because hooks conventionally return negative
errno. Would testing "if (ret)" here, as the vdc->vmsd call does, be more
consistent?

The follow-up commit e68e95a3b426 ("migration: vmstate_save_state_v: fix
double error_setg") does not change this. The nonzero-return error_setg()
calls in vmstate.c are unchanged, and the call sites still use "ret < 0".

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=12