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 2 potential issue(s) to consider:
- [Medium] migration: vmstate_load() still passes error_fatal, so load errors exit QEMU
- [Low] migration: void save callbacks only report vmstate_save_state() failure, stream not failed
--

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

migration: Fix regression of passing error_fatal into vmstate_load_state()

This patch replaces error_fatal in the vmstate_load_state() and
vmstate_save_state() calls in several hw/ files with a local Error
object. 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/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c

[ ... ]

> @@ -1344,9 +1350,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);
> -
> -    return 0;
> +    ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> +    if (ret < 0) {
> +        error_report_err(err);
> +    }
> +    return ret;
>  }

[Severity: Low]
This isn't a bug, but the behaviour here changes. The old code ignored the
result of vmstate_load_state() and returned 0, while the new code returns
the failure to the caller.

The error is also printed here by error_report_err(). Could the outer
vmstate_load_state() caller print it a second time, so that the same
failure is reported twice?

> 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 save failure here go unnoticed by the migration core?

pci_device_save() returns void, so it prints the error and returns
normally. vmstate_save_state_v() only sets the Error object on failure:

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

It does not call qemu_file_set_error(), so the QEMUFile error state stays
clear. virtio_save() then keeps writing the stream, and the migration or
snapshot could be left incomplete with no failure flagged.

The same pattern appears in the other void save callbacks changed by this
patch:

hw/virtio/virtio-pci.c:virtio_pci_save_extra_state()
hw/virtio/virtio-mmio.c:virtio_mmio_save_extra_state()
hw/s390x/virtio-ccw.c:virtio_ccw_save_config()

> 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;
> +    }
>      trace_spapr_vscsi_save_request(req->qtag, req->cur_desc_num,
>                                     req->cur_desc_offset);
>  }

[Severity: Low]
vscsi_save_request() is a void .save_request hook, so the SCSI core cannot
see this failure either. Is it intended that the error is only printed
here and the save carries on?

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

[ ... ]

The patch fixes the hw/ callers, but the central loader still passes
error_fatal:

migration/savevm.c:vmstate_load() {
    ...
    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
                              &error_fatal);
}

[Severity: Medium]
Does this leave the regression described in the commit message in place for
the main load path?

vmstate_load() is called from qemu_loadvm_section_start_full() and
qemu_loadvm_section_part_end(). Any vmsd load failure there, such as a
version_id that is too new or too old, or a pre_load, get or post_load
failure, still calls exit(1) inside vmstate_load_state().

The error handling in both callers is therefore not reached for vmsd
devices:

    ret = vmstate_load(f, se);
    if (ret < 0) {
        error_report("error while loading state for instance 0x%"PRIx32" of"
                     " device '%s'", instance_id, idstr);
        return ret;
    }

and:

    ret = vmstate_load(f, se);
    if (ret < 0) {
        error_report("error while loading state section id %d(%s)",
                     section_id, se->idstr);
        return ret;
    }

A grep of migration/savevm.c at this commit and at the end of the series
shows the same &error_fatal in vmstate_load(). Should vmstate_load() also
use a local Error object and propagate the failure?

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