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
