Re: [PATCH QEMU HCI-8.1 v6 16/34] 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: void vmstate save helpers log failure but do not fail the stream
--

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

migration: Fix regression of passing error_fatal into vmstate_load_state()

This replaces error_fatal in several vmstate_save_state() and
vmstate_load_state() callers with a local Error object. Failures are now
reported with error_report_err() and, where the function returns an int,
propagated to the caller. This is a cherry-pick of upstream commit
986c3292c6b8.

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]
Can a vmstate_save_state() failure here go unnoticed by the migration
core? pci_device_save() is void, so on ret < 0 the error is only printed
and the function carries on to pci_update_irq_status().

Looking at vmstate_save_state_v() in migration/vmstate.c, the failure
paths only call error_setg() and return ret:

    if (ret) {
        error_setg(errp, "Save of field %s/%s failed",
                    vmsd->name, field->name);
        if (vmsd->post_save) {
            vmsd->post_save(opaque);
        }
        return ret;
    }

It never calls qemu_file_set_error() on the QEMUFile. Before this patch,
error_fatal made QEMU exit(1) in this case. Now the save continues and the
stream is not marked failed.

Could the destination then receive truncated or inconsistent device state?
Would it make sense to call qemu_file_set_error(f, ret) in these void
callers?

The same applies to the other void callers changed here:
virtio_ccw_save_config(), virtio_mmio_save_extra_state(), and
virtio_pci_save_extra_state(). In the last one, virtio_save() keeps
writing the stream after the failure because the save_extra_state callback
cannot report one.

[ ... ]

> 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

[ ... ]

>  static void vscsi_save_request(QEMUFile *f, SCSIRequest *sreq)
>  {
>      vscsi_req *req = sreq->hba_private;
> -    assert(req->active);
> +    Error *local_err = NULL;
> +    int rc;
>  
> -    vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &error_fatal);
> +    assert(req->active);
>  
> +    rc = vmstate_save_state(f, &vmstate_spapr_vscsi_req, req, NULL, &local_err);
> +    if (rc < 0) {
> +        error_report_err(local_err);
> +        return;
> +    }
>      trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
>                                     req->cur_desc_offset);
>  }

[Severity: Low]
Similarly, in vscsi_save_request(), the failure is reported and then
swallowed with a bare return. Can the request end up partly or wholly
missing from the stream while the migration is not failed?

The later series patch "migration: vmstate_save_state_v: fix double
error_setg" only changes the error message text. It does not set an error
on the QEMUFile, so this still looks present at the end of the series.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=16