Re: [PATCH v3 4/4] migration: vmsd errp handlers: return bool

[Peter Xu <peterx@redhat.com>]

On Tue, Oct 28, 2025 at 07:26:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 28.10.25 18:12, Peter Xu wrote:
> > On Mon, Oct 27, 2025 at 08:06:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Understand.. So we don't know, does any caller use this ENOMEM, or not. And want to save
> > > a chance for bulk conversion.
> > > 
> > > And blind bulk conversion of all -errno to simple true/false may break something, we
> > > don't know.
> > > 
> > > Reasonable. Thanks for the explanation.
> > 
> > Taking vmstate_load_state() as example: most of its callers should
> > ultimately route back to the top of vmstate_load_state() that migration
> > code invokes.
> > 
> > AFAIU, migration itself doesn't care much so far on the retval, but only
> > succeeded or not, plus the errp as a bonus.  There's one thing exception
> > that I remember but it was removed in commit fd037a656aca..  So now I
> > cannot find anything relies on that.
> > 
> > Here's a list of all current vmstate_load_state() callers:
> > 
> > *** hw/display/virtio-gpu.c:
> > virtio_gpu_load[1358]          ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> > 
> > *** hw/pci/pci.c:
> > pci_device_load[943]           ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> > 
> > *** hw/s390x/virtio-ccw.c:
> > virtio_ccw_load_config[1146]   ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);
> > 
> > *** hw/scsi/spapr_vscsi.c:
> > vscsi_load_request[657]        rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &local_err);
> > 
> > Until here, it's invoked in a get() callback of VMStateInfo.  Ultimately,
> > it goes back to migration's vmstate_load_state() invokation.
> > 
> > *** hw/vfio/pci.c:
> > vfio_pci_load_config[2840]     ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> > 
> > This is special as it's part of load_state().  However it's the same, it
> > routes back to vmstate_load() instead (where vmstate_load_state()
> > ultimately also routes there).  So looks safe too.
> > 
> > *** hw/virtio/virtio-mmio.c:
> > virtio_mmio_load_extra_state[630] ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);
> > 
> > *** hw/virtio/virtio-pci.c:
> > virtio_pci_load_extra_state[205] ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);
> > 
> > *** hw/virtio/virtio.c:
> > virtio_load[3394]              ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &local_err);
> > virtio_load[3402]              ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &local_err);
> > 
> > More get() users.  Same.
> > 
> > *** migration/cpr.c:
> > cpr_state_load[266]            ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> > 
> > This is special, ignoring retval but using error_abort.  New one, safe.
> > 
> > *** migration/savevm.c:
> > vmstate_load[978]              return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > qemu_loadvm_state_header[2885] ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> > 
> > This is the migration core invokations.  Safe.
> > 
> > *** migration/vmstate-types.c:
> > get_tmp[562]                   ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> > get_qtailq[670]                ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> > get_gtree[833]                 ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
> > get_gtree[840]                 ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
> > get_qlist[921]                 ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> > 
> > These are special get() for special VMSD fields.  Safe.
> > 
> > *** migration/vmstate.c:
> > vmstate_load_state[208]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > vmstate_load_state[212]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > vmstate_subsection_load[652]   ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> > 
> > Same migration core invokations. Safe.
> > 
> > *** tests/unit/test-vmstate.c:
> > load_vmstate_one[123]          ret = vmstate_load_state(f, desc, obj, version, &local_err);
> > test_load_v1[377]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
> > test_load_v2[408]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
> > test_load_noskip[512]          ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > test_load_skip[541]            ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > test_load_q[815]               ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
> > test_gtree_load_domain[1174]   ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
> > test_gtree_load_iommu[1294]    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
> > test_load_qlist[1434]          ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,
> > 
> > Test code only.  Safe.
> > 
> > *** ui/vdagent.c:
> > get_cbinfo[1013]               ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,
> > 
> > Yet another get().  Safe.
> > 
> > So.. even if I'm not sure if a bulk conversion could happen or not (the
> > get() users above would be very tricky because get() doesn't allow errp so
> > far.. unless we introduce that too), but other than that, afaict,
> > vmstate_load_state() callers do not yet care about retvals.
> > 
> 
> Uhh, great analysis! With it, we can proceed with my patch. And may be, just change return value of vmstate_load_state to bool? To avoid analyzing it again in future.

It'll be some more code churns.. so some more work for downstream
maintenances.  But yeah, I agree that should follow what we suggest to do
in qemu in general.

I also didn't check the save side.  I would expect to be similar, but worth
some double checks.

Thanks,

-- 
Peter Xu