Re: [PATCH QEMU HCI-8.1 v2 09/24] 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 1 potential issue(s) to consider:
- [Low] migration: vmstate save/load callers only report Error when ret < 0
--

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

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

migration: Fix regression of passing error_fatal into vmstate_load_state()

This patch replaces &error_fatal with a local Error object in the callers
of vmstate_save_state() and vmstate_load_state(). It reports the error with
error_report_err() when the call fails, and returns the result where the
caller can use it.

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);
> +    }

[Severity: Low]
This is a speculative concern, but the same pattern appears in all the new
callers, so I am raising it once here.

vmstate_save_state_v() treats any nonzero return as a failure, for example
in the pre_save, post_save and put paths:

migration/vmstate.c:vmstate_save_state_v() {
    ...
        if (ret) {
            error_prepend(errp, "Save of field %s/%s failed: ", ...
            ...
            return ret;
        }
    ...
}

The new callers only report the Error when ret < 0. These callers are
pci_device_save(), virtio_save() (final subsection save), virtio_gpu_save(),
virtio_ccw_save_config(), vscsi_save_request(),
virtio_mmio_save_extra_state() and virtio_pci_save_extra_state().

If a callback ever returned a positive nonzero value, would local_err be
allocated but neither reported nor freed? The void wrappers would also
silently ignore the failure, and virtio_save() and virtio_gpu_save()
would return the positive value unchanged. Before this patch, error_fatal
would have exited on any nonzero return.

The load-side callers have the same exposure through pre_load() (returned
as-is) and post_load().

[ ... ]

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

[ ... ]

>      if (vdc->vmsd) {
> -        int ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
> +        ret = vmstate_save_state(f, vdc->vmsd, vdev, NULL, &local_err);
>          if (ret) {
>              error_report_err(local_err);
>              return ret;
> @@ -3069,7 +3069,11 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      }
>  
>      /* 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]
Within virtio_save(), the vdc->vmsd save a few lines earlier checks
"if (ret)", while this final subsection save checks "if (ret < 0)". Should
the two checks be consistent?

With "ret < 0", a positive return from vmstate_save_state() leaves
local_err unreported and unfreed, and virtio_save() returns the positive
value.

The later series commit "migration: vmstate_save_state_v: fix double
error_setg" does not appear to address this. It sets an Error in the put
branch only when ret < 0, but the following "if (ret)" block still calls
error_prepend(errp, ...). Could a positive return from a put callback then
reach error_prepend() with *errp still NULL when a caller passes
&local_err, as these callers now do? The callers are unchanged at the end of
that series.

In practice callbacks conventionally return negative errnos, so this is
likely a cold path.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=9