Re: [PATCH QEMU HCI-8.1 09/21] 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:
- [Medium] migration: vmstate_save_state_v() calls error_setg() on an errp already set by the nested call
- [Low] migration: void vmstate save helpers log failure but never flag the QEMUFile
--

Commit e12869dadb039e65c940e64b56a6161151d7d971
Author: Arun Menon <armenon@redhat.com>

migration: Fix regression of passing error_fatal into vmstate_load_state()

This replaces &error_fatal in the vmstate_save_state() and
vmstate_load_state() callers with a local Error object. Errors are now
reported with error_report_err() and the return value is propagated where
the caller has one.

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: Medium]
Can this abort QEMU through the assertion in error_setv()?

In vmstate_save_state_v(), a nested VMS_STRUCT, VMS_VSTRUCT or subsection
save is called with the same errp:

    if (inner_field->flags & VMS_STRUCT) {
        ret = vmstate_save_state(f, inner_field->vmsd, curr_elem,
                                 vmdesc_loop, errp);
    }
    ...
    if (ret) {
        error_setg(errp, "Save of field %s/%s failed",
                   vmsd->name, field->name);

If the nested call fails, it has already set *errp. The outer loop then
calls error_setg() on the same errp, which reaches:

    assert(*errp == NULL);

With &error_fatal, QEMU exited before this point. With a real Error * as
passed here, the second error_setg() looks like it would hit the assertion
instead of propagating the error.

The same applies to the other converted save callers in this patch, such
as virtio_pci_save_extra_state() and virtio_save(). vmstate_save_state_v()
does not appear to use ERRP_GUARD or check for an already set error.

Could the outer error_setg() be skipped when *errp is already set, or could
error_prepend() be used there instead?

[Severity: Low]
Is logging the error here enough?

pci_device_save() returns void and does not call qemu_file_set_error().
vmstate_save_state_v() only does error_setg() and returns ret on failure,
unlike the load path, which calls qemu_file_set_error(). So the QEMUFile
error state stays clean after a failed save.

The source then continues writing the stream, and the failure would show
up later as a load error on the destination. Before this patch, QEMU
exited at the point of failure.

> 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]
Does vscsi_save_request() leave its caller unaware of the failed save?

This void function reports the error and returns. Nothing calls
qemu_file_set_error(f, rc), so the caller continues as if the save
succeeded.

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

[ ... ]

>  static void virtio_pci_save_extra_state(DeviceState *d, QEMUFile *f)
>  {
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> +    Error *local_err = NULL;
> +    int ret;
>  
> -    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);
> +    }
>  }

[Severity: Low]
Does this void save callback have the same gap as pci_device_save()?

The failure is only logged and the QEMUFile is not flagged with
qemu_file_set_error(). The same pattern appears in
virtio_mmio_save_extra_state() and virtio_ccw_save_config().

Would it make sense for vmstate_save_state_v() or these helpers to set the
QEMUFile error on failure?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=9