- This is an incremental step in converting vmstate loading
code to report errors.
- Minimal changes to the signature and body of the following
functions are done,
- vmstate_load()
- vmstate_load_state()
- vmstate_subsection_load()
- qemu_load_device_state()
- qemu_loadvm_state()
- qemu_loadvm_section_start_full()
- qemu_loadvm_section_part_end()
- qemu_loadvm_state_header()
- qemu_loadvm_state_main()
Signed-off-by: Arun Menon <armenon@redhat.com>
---
hw/display/virtio-gpu.c | 2 +-
hw/pci/pci.c | 2 +-
hw/s390x/virtio-ccw.c | 2 +-
hw/scsi/spapr_vscsi.c | 2 +-
hw/vfio/pci.c | 2 +-
hw/virtio/virtio-mmio.c | 2 +-
hw/virtio/virtio-pci.c | 2 +-
hw/virtio/virtio.c | 4 ++--
include/migration/vmstate.h | 2 +-
migration/colo.c | 4 ++--
migration/cpr.c | 2 +-
migration/migration.c | 2 +-
migration/savevm.c | 52 +++++++++++++++++++++++----------------------
migration/savevm.h | 7 +++---
migration/vmstate-types.c | 10 ++++-----
migration/vmstate.c | 16 ++++++++------
tests/unit/test-vmstate.c | 18 ++++++++--------
17 files changed, 68 insertions(+), 63 deletions(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5d2ca8d8b864350133a674802d7316abd379591c 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1343,7 +1343,7 @@ 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);
+ vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, NULL);
return 0;
}
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..2ab5d30bb3c319ac1c7bfc9a2acf6a2b38082066 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -934,7 +934,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
int pci_device_load(PCIDevice *s, QEMUFile *f)
{
int ret;
- ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
+ ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id, NULL);
/* Restore the interrupt status bit. */
pci_update_irq_status(s);
return ret;
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..2f6feff2b0a22d7d7f6aecfd7e7870d8362f1a73 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
+ return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, NULL);
}
static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 20f70fb2729de78b9636a6b8c869695dab4f8902..573fdea668536b464bca11f001e9e0288e781493 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
assert(!req->active);
memset(req, 0, sizeof(*req));
- rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
+ rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, NULL);
if (rc) {
fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
return NULL;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index fa25bded25c51f8efb6c5ad31bd90506cd69745c..87aee0a5701087f9a68ea435bb96e9d6b07b0c24 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2715,7 +2715,7 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
old_addr[bar] = pdev->io_regions[bar].addr;
}
- ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
+ ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1, NULL);
if (ret) {
return ret;
}
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..9058b1563462d4464dcba799643a583c93fb5683 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -619,7 +619,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
{
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
- return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
+ return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, NULL);
}
static bool virtio_mmio_has_extra_state(DeviceState *opaque)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index fba2372c93bfd648736b07e4bc83e7097baa58cb..50a1f5701754b88e8a1ee062d6eeedfd848cb4f5 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -160,7 +160,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
{
VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
- return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
+ return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, NULL);
}
static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 82a285a31d1c0427d55f7cb73398adfc94e678fe..66d5941f68a4b9e1e5390bb0aa45fc6cd34e2a1e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3317,14 +3317,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
}
if (vdc->vmsd) {
- ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
+ ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, NULL);
if (ret) {
return ret;
}
}
/* Subsections */
- ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
+ ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, NULL);
if (ret) {
return ret;
}
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
}
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, int version_id);
+ void *opaque, int version_id, Error **errp);
int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc);
int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
diff --git a/migration/colo.c b/migration/colo.c
index e0f713c837f5da25d67afbd02ceb6c54024ca3af..c7779683f0aad33cd071030ac553da69d6a5e60d 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -686,7 +686,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
bql_lock();
cpu_synchronize_all_states();
- ret = qemu_loadvm_state_main(mis->from_src_file, mis);
+ ret = qemu_loadvm_state_main(mis->from_src_file, mis, &local_err);
bql_unlock();
if (ret < 0) {
@@ -729,7 +729,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
bql_lock();
vmstate_loading = true;
colo_flush_ram_cache();
- ret = qemu_load_device_state(fb);
+ ret = qemu_load_device_state(fb, &local_err);
if (ret < 0) {
error_setg(errp, "COLO: load device state failed");
vmstate_loading = false;
diff --git a/migration/cpr.c b/migration/cpr.c
index a50a57edca754b50e68fa9c294b3c89791e62ba8..0fb9fadac905c83689eed2b1193b282da679b6ef 100644
--- a/migration/cpr.c
+++ b/migration/cpr.c
@@ -235,7 +235,7 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
return -ENOTSUP;
}
- ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
+ ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
if (ret) {
error_setg(errp, "vmstate_load_state error %d", ret);
qemu_fclose(f);
diff --git a/migration/migration.c b/migration/migration.c
index 4098870bce33ffdc57b5972fc5b106d88abb237e..5cabb4e7307323159241ff35781db7f1c665a75b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -876,7 +876,7 @@ process_incoming_migration_co(void *opaque)
MIGRATION_STATUS_ACTIVE);
mis->loadvm_co = qemu_coroutine_self();
- ret = qemu_loadvm_state(mis->from_src_file);
+ ret = qemu_loadvm_state(mis->from_src_file, &local_err);
mis->loadvm_co = NULL;
trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
diff --git a/migration/savevm.c b/migration/savevm.c
index bb04a4520df9a443d90cf6cb52a383a5f053aaff..9bcc0935781b73e209dc57945f9dbb381283cad5 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -963,13 +963,14 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
}
}
-static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
+static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
{
trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
if (!se->vmsd) { /* Old style */
return se->ops->load_state(f, se->opaque, se->load_version_id);
}
- return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
+ return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
+ errp);
}
static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
@@ -2071,6 +2072,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
{
MigrationIncomingState *mis = migration_incoming_get_current();
QEMUFile *f = mis->from_src_file;
+ Error *local_err = NULL;
int load_res;
MigrationState *migr = migrate_get_current();
@@ -2089,7 +2091,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
qemu_file_set_blocking(f, true);
/* TODO: sanity check that only postcopiable data will be loaded here */
- load_res = qemu_loadvm_state_main(f, mis);
+ load_res = qemu_loadvm_state_main(f, mis, &local_err);
/*
* This is tricky, but, mis->from_src_file can change after it
@@ -2394,6 +2396,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
int ret;
size_t length;
QIOChannelBuffer *bioc;
+ Error *local_error;
length = qemu_get_be32(mis->from_src_file);
trace_loadvm_handle_cmd_packaged(length);
@@ -2440,7 +2443,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
qemu_coroutine_yield();
} while (1);
- ret = qemu_loadvm_state_main(packf, mis);
+ ret = qemu_loadvm_state_main(packf, mis, &local_error);
trace_loadvm_handle_cmd_packaged_main(ret);
qemu_fclose(packf);
object_unref(OBJECT(bioc));
@@ -2674,7 +2677,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
}
static int
-qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
+qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
{
bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
uint32_t instance_id, version_id, section_id;
@@ -2731,7 +2734,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
}
- ret = vmstate_load(f, se);
+ ret = vmstate_load(f, se, errp);
if (ret < 0) {
error_report("error while loading state for instance 0x%"PRIx32" of"
" device '%s'", instance_id, idstr);
@@ -2752,7 +2755,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
}
static int
-qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
+qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
{
bool trace_downtime = (type == QEMU_VM_SECTION_END);
int64_t start_ts, end_ts;
@@ -2784,7 +2787,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
}
- ret = vmstate_load(f, se);
+ ret = vmstate_load(f, se, errp);
if (ret < 0) {
error_report("error while loading state section id %d(%s)",
section_id, se->idstr);
@@ -2804,7 +2807,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
return 0;
}
-static int qemu_loadvm_state_header(QEMUFile *f)
+static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
{
unsigned int v;
int ret;
@@ -2830,7 +2833,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
error_report("Configuration section missing");
return -EINVAL;
}
- ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
+ ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
+ errp);
if (ret) {
return ret;
@@ -3019,7 +3023,8 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
return true;
}
-int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
+int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
+ Error **errp)
{
uint8_t section_type;
int ret = 0;
@@ -3037,14 +3042,14 @@ retry:
switch (section_type) {
case QEMU_VM_SECTION_START:
case QEMU_VM_SECTION_FULL:
- ret = qemu_loadvm_section_start_full(f, section_type);
+ ret = qemu_loadvm_section_start_full(f, section_type, errp);
if (ret < 0) {
goto out;
}
break;
case QEMU_VM_SECTION_PART:
case QEMU_VM_SECTION_END:
- ret = qemu_loadvm_section_part_end(f, section_type);
+ ret = qemu_loadvm_section_part_end(f, section_type, errp);
if (ret < 0) {
goto out;
}
@@ -3094,27 +3099,24 @@ out:
return ret;
}
-int qemu_loadvm_state(QEMUFile *f)
+int qemu_loadvm_state(QEMUFile *f, Error **errp)
{
MigrationState *s = migrate_get_current();
MigrationIncomingState *mis = migration_incoming_get_current();
- Error *local_err = NULL;
int ret;
- if (qemu_savevm_state_blocked(&local_err)) {
- error_report_err(local_err);
+ if (qemu_savevm_state_blocked(errp)) {
return -EINVAL;
}
qemu_loadvm_thread_pool_create(mis);
- ret = qemu_loadvm_state_header(f);
+ ret = qemu_loadvm_state_header(f, errp);
if (ret) {
return ret;
}
- if (qemu_loadvm_state_setup(f, &local_err) != 0) {
- error_report_err(local_err);
+ if (qemu_loadvm_state_setup(f, errp) != 0) {
return -EINVAL;
}
@@ -3124,7 +3126,7 @@ int qemu_loadvm_state(QEMUFile *f)
cpu_synchronize_all_pre_loadvm();
- ret = qemu_loadvm_state_main(f, mis);
+ ret = qemu_loadvm_state_main(f, mis, errp);
qemu_event_set(&mis->main_thread_load_event);
trace_qemu_loadvm_state_post_main(ret);
@@ -3192,13 +3194,13 @@ int qemu_loadvm_state(QEMUFile *f)
return ret;
}
-int qemu_load_device_state(QEMUFile *f)
+int qemu_load_device_state(QEMUFile *f, Error **errp)
{
MigrationIncomingState *mis = migration_incoming_get_current();
int ret;
/* Load QEMU_VM_SECTION_FULL section */
- ret = qemu_loadvm_state_main(f, mis);
+ ret = qemu_loadvm_state_main(f, mis, errp);
if (ret < 0) {
error_report("Failed to load device state: %d", ret);
return ret;
@@ -3429,7 +3431,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
f = qemu_file_new_input(QIO_CHANNEL(ioc));
object_unref(OBJECT(ioc));
- ret = qemu_loadvm_state(f);
+ ret = qemu_loadvm_state(f, errp);
qemu_fclose(f);
if (ret < 0) {
error_setg(errp, "loading Xen device state failed");
@@ -3503,7 +3505,7 @@ bool load_snapshot(const char *name, const char *vmstate,
ret = -EINVAL;
goto err_drain;
}
- ret = qemu_loadvm_state(f);
+ ret = qemu_loadvm_state(f, errp);
migration_incoming_state_destroy();
bdrv_drain_all_end();
diff --git a/migration/savevm.h b/migration/savevm.h
index 2d5e9c716686f06720325e82fe90c75335ced1de..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
--- a/migration/savevm.h
+++ b/migration/savevm.h
@@ -64,10 +64,11 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
void qemu_savevm_live_state(QEMUFile *f);
int qemu_save_device_state(QEMUFile *f);
-int qemu_loadvm_state(QEMUFile *f);
+int qemu_loadvm_state(QEMUFile *f, Error **errp);
void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
-int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
-int qemu_load_device_state(QEMUFile *f);
+int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
+ Error **errp);
+int qemu_load_device_state(QEMUFile *f, Error **errp);
int qemu_loadvm_approve_switchover(void);
int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
bool in_postcopy);
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..1c5b76e1dd198030847971bc35637867c9d54fc0 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -549,7 +549,7 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
/* Writes the parent field which is at the start of the tmp */
*(void **)tmp = pv;
- ret = vmstate_load_state(f, vmsd, tmp, version_id);
+ ret = vmstate_load_state(f, vmsd, tmp, version_id, NULL);
g_free(tmp);
return ret;
}
@@ -649,7 +649,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
while (qemu_get_byte(f)) {
elm = g_malloc(size);
- ret = vmstate_load_state(f, vmsd, elm, version_id);
+ ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
if (ret) {
return ret;
}
@@ -803,7 +803,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
key = (void *)(uintptr_t)qemu_get_be64(f);
} else {
key = g_malloc0(key_size);
- ret = vmstate_load_state(f, key_vmsd, key, version_id);
+ ret = vmstate_load_state(f, key_vmsd, key, version_id, NULL);
if (ret) {
error_report("%s : failed to load %s (%d)",
field->name, key_vmsd->name, ret);
@@ -811,7 +811,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
}
}
val = g_malloc0(val_size);
- ret = vmstate_load_state(f, val_vmsd, val, version_id);
+ ret = vmstate_load_state(f, val_vmsd, val, version_id, NULL);
if (ret) {
error_report("%s : failed to load %s (%d)",
field->name, val_vmsd->name, ret);
@@ -892,7 +892,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
while (qemu_get_byte(f)) {
elm = g_malloc(size);
- ret = vmstate_load_state(f, vmsd, elm, version_id);
+ ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
if (ret) {
error_report("%s: failed to load %s (%d)", field->name,
vmsd->name, ret);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 5feaa3244d259874f03048326b2497e7db32e47c..177c563ff103ada2e494c14173fa773d52adb800 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
void *opaque, JSONWriter *vmdesc,
Error **errp);
static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque);
+ void *opaque, Error **errp);
/* Whether this field should exist for either save or load the VM? */
static bool
@@ -132,7 +132,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
}
int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque, int version_id)
+ void *opaque, int version_id, Error **errp)
{
const VMStateField *field = vmsd->fields;
int ret = 0;
@@ -192,10 +192,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
if (inner_field->flags & VMS_STRUCT) {
ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
- inner_field->vmsd->version_id);
+ inner_field->vmsd->version_id,
+ errp);
} else if (inner_field->flags & VMS_VSTRUCT) {
ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
- inner_field->struct_version_id);
+ inner_field->struct_version_id,
+ errp);
} else {
ret = inner_field->info->get(f, curr_elem, size,
inner_field);
@@ -225,7 +227,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
field++;
}
assert(field->flags == VMS_END);
- ret = vmstate_subsection_load(f, vmsd, opaque);
+ ret = vmstate_subsection_load(f, vmsd, opaque, errp);
if (ret != 0) {
qemu_file_set_error(f, ret);
return ret;
@@ -566,7 +568,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
}
static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
- void *opaque)
+ void *opaque, Error **errp)
{
trace_vmstate_subsection_load(vmsd->name);
@@ -605,7 +607,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
qemu_file_skip(f, len); /* idstr */
version_id = qemu_get_be32(f);
- ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
+ ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
if (ret) {
trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
return ret;
diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index 63f28f26f45691a70936d33e7341d16477a3471f..ca5e0ba1e3e5e2bb0a1ce39143a292f2c6f9420a 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -114,7 +114,7 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
qemu_fclose(f);
f = open_test_file(false);
- ret = vmstate_load_state(f, desc, obj, version);
+ ret = vmstate_load_state(f, desc, obj, version, NULL);
if (ret) {
g_assert(qemu_file_get_error(f));
} else{
@@ -365,7 +365,7 @@ static void test_load_v1(void)
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .b = 200, .e = 500, .f = 600 };
- vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
+ vmstate_load_state(loading, &vmstate_versioned, &obj, 1, NULL);
g_assert(!qemu_file_get_error(loading));
g_assert_cmpint(obj.a, ==, 10);
g_assert_cmpint(obj.b, ==, 200);
@@ -391,7 +391,7 @@ static void test_load_v2(void)
QEMUFile *loading = open_test_file(false);
TestStruct obj;
- vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
+ vmstate_load_state(loading, &vmstate_versioned, &obj, 2, NULL);
g_assert_cmpint(obj.a, ==, 10);
g_assert_cmpint(obj.b, ==, 20);
g_assert_cmpint(obj.c, ==, 30);
@@ -480,7 +480,7 @@ static void test_load_noskip(void)
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .skip_c_e = false };
- vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
+ vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
g_assert(!qemu_file_get_error(loading));
g_assert_cmpint(obj.a, ==, 10);
g_assert_cmpint(obj.b, ==, 20);
@@ -504,7 +504,7 @@ static void test_load_skip(void)
QEMUFile *loading = open_test_file(false);
TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
- vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
+ vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
g_assert(!qemu_file_get_error(loading));
g_assert_cmpint(obj.a, ==, 10);
g_assert_cmpint(obj.b, ==, 20);
@@ -773,7 +773,7 @@ static void test_load_q(void)
TestQtailq tgt;
QTAILQ_INIT(&tgt.q);
- vmstate_load_state(fload, &vmstate_q, &tgt, 1);
+ vmstate_load_state(fload, &vmstate_q, &tgt, 1, NULL);
char eof = qemu_get_byte(fload);
g_assert(!qemu_file_get_error(fload));
g_assert_cmpint(tgt.i16, ==, obj_q.i16);
@@ -1127,7 +1127,7 @@ static void test_gtree_load_domain(void)
fload = open_test_file(false);
- vmstate_load_state(fload, &vmstate_domain, dest_domain, 1);
+ vmstate_load_state(fload, &vmstate_domain, dest_domain, 1, NULL);
eof = qemu_get_byte(fload);
g_assert(!qemu_file_get_error(fload));
g_assert_cmpint(orig_domain->id, ==, dest_domain->id);
@@ -1241,7 +1241,7 @@ static void test_gtree_load_iommu(void)
qemu_fclose(fsave);
fload = open_test_file(false);
- vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1);
+ vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, NULL);
eof = qemu_get_byte(fload);
g_assert(!qemu_file_get_error(fload));
g_assert_cmpint(orig_iommu->id, ==, dest_iommu->id);
@@ -1376,7 +1376,7 @@ static void test_load_qlist(void)
qemu_fclose(fsave);
fload = open_test_file(false);
- vmstate_load_state(fload, &vmstate_container, dest_container, 1);
+ vmstate_load_state(fload, &vmstate_container, dest_container, 1, NULL);
eof = qemu_get_byte(fload);
g_assert(!qemu_file_get_error(fload));
g_assert_cmpint(eof, ==, QEMU_VM_EOF);
--
2.49.0Hi, Arun,
On Tue, Jun 24, 2025 at 05:53:04PM +0530, Arun Menon wrote:
> - This is an incremental step in converting vmstate loading
> code to report errors.
> - Minimal changes to the signature and body of the following
> functions are done,
> - vmstate_load()
> - vmstate_load_state()
> - vmstate_subsection_load()
> - qemu_load_device_state()
> - qemu_loadvm_state()
> - qemu_loadvm_section_start_full()
> - qemu_loadvm_section_part_end()
> - qemu_loadvm_state_header()
> - qemu_loadvm_state_main()
>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
> hw/display/virtio-gpu.c | 2 +-
> hw/pci/pci.c | 2 +-
> hw/s390x/virtio-ccw.c | 2 +-
> hw/scsi/spapr_vscsi.c | 2 +-
> hw/vfio/pci.c | 2 +-
> hw/virtio/virtio-mmio.c | 2 +-
> hw/virtio/virtio-pci.c | 2 +-
> hw/virtio/virtio.c | 4 ++--
> include/migration/vmstate.h | 2 +-
> migration/colo.c | 4 ++--
> migration/cpr.c | 2 +-
> migration/migration.c | 2 +-
> migration/savevm.c | 52 +++++++++++++++++++++++----------------------
> migration/savevm.h | 7 +++---
> migration/vmstate-types.c | 10 ++++-----
> migration/vmstate.c | 16 ++++++++------
> tests/unit/test-vmstate.c | 18 ++++++++--------
> 17 files changed, 68 insertions(+), 63 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5d2ca8d8b864350133a674802d7316abd379591c 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1343,7 +1343,7 @@ 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);
> + vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, NULL);
>
> return 0;
> }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..2ab5d30bb3c319ac1c7bfc9a2acf6a2b38082066 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -934,7 +934,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> int pci_device_load(PCIDevice *s, QEMUFile *f)
> {
> int ret;
> - ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> + ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id, NULL);
> /* Restore the interrupt status bit. */
> pci_update_irq_status(s);
> return ret;
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..2f6feff2b0a22d7d7f6aecfd7e7870d8362f1a73 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> - return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, NULL);
> }
>
> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index 20f70fb2729de78b9636a6b8c869695dab4f8902..573fdea668536b464bca11f001e9e0288e781493 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
> assert(!req->active);
>
> memset(req, 0, sizeof(*req));
> - rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
> + rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, NULL);
> if (rc) {
> fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
> return NULL;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index fa25bded25c51f8efb6c5ad31bd90506cd69745c..87aee0a5701087f9a68ea435bb96e9d6b07b0c24 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2715,7 +2715,7 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> old_addr[bar] = pdev->io_regions[bar].addr;
> }
>
> - ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
> + ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1, NULL);
> if (ret) {
> return ret;
> }
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..9058b1563462d4464dcba799643a583c93fb5683 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -619,7 +619,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
> {
> VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>
> - return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
> + return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, NULL);
> }
>
> static bool virtio_mmio_has_extra_state(DeviceState *opaque)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index fba2372c93bfd648736b07e4bc83e7097baa58cb..50a1f5701754b88e8a1ee062d6eeedfd848cb4f5 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -160,7 +160,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
> {
> VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>
> - return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
> + return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, NULL);
> }
>
> static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 82a285a31d1c0427d55f7cb73398adfc94e678fe..66d5941f68a4b9e1e5390bb0aa45fc6cd34e2a1e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3317,14 +3317,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> }
>
> if (vdc->vmsd) {
> - ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
> + ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, NULL);
> if (ret) {
> return ret;
> }
> }
>
> /* Subsections */
> - ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
> + ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, NULL);
> if (ret) {
> return ret;
> }
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
> }
>
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, int version_id);
> + void *opaque, int version_id, Error **errp);
> int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc);
> int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> diff --git a/migration/colo.c b/migration/colo.c
> index e0f713c837f5da25d67afbd02ceb6c54024ca3af..c7779683f0aad33cd071030ac553da69d6a5e60d 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -686,7 +686,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>
> bql_lock();
> cpu_synchronize_all_states();
> - ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> + ret = qemu_loadvm_state_main(mis->from_src_file, mis, &local_err);
> bql_unlock();
>
> if (ret < 0) {
Here the diff didn't show, but it's:
if (ret < 0) {
error_setg(errp, "Load VM's live state (ram) error");
return;
}
Note that error_setg() asserts *errp==NULL. I think this will crash qemu
when load fails for COLO. AFAIU we need to use error_prepend() for such
cases.
> @@ -729,7 +729,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> bql_lock();
> vmstate_loading = true;
> colo_flush_ram_cache();
> - ret = qemu_load_device_state(fb);
> + ret = qemu_load_device_state(fb, &local_err);
> if (ret < 0) {
> error_setg(errp, "COLO: load device state failed");
Same here.
> vmstate_loading = false;
> diff --git a/migration/cpr.c b/migration/cpr.c
> index a50a57edca754b50e68fa9c294b3c89791e62ba8..0fb9fadac905c83689eed2b1193b282da679b6ef 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -235,7 +235,7 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
> return -ENOTSUP;
> }
>
> - ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> + ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> if (ret) {
> error_setg(errp, "vmstate_load_state error %d", ret);
Same here.
> qemu_fclose(f);
> diff --git a/migration/migration.c b/migration/migration.c
> index 4098870bce33ffdc57b5972fc5b106d88abb237e..5cabb4e7307323159241ff35781db7f1c665a75b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -876,7 +876,7 @@ process_incoming_migration_co(void *opaque)
> MIGRATION_STATUS_ACTIVE);
>
> mis->loadvm_co = qemu_coroutine_self();
> - ret = qemu_loadvm_state(mis->from_src_file);
> + ret = qemu_loadvm_state(mis->from_src_file, &local_err);
Same. This one you need to scroll a bit until:
if (ret < 0) {
error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
goto fail;
}
> mis->loadvm_co = NULL;
>
> trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> diff --git a/migration/savevm.c b/migration/savevm.c
> index bb04a4520df9a443d90cf6cb52a383a5f053aaff..9bcc0935781b73e209dc57945f9dbb381283cad5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -963,13 +963,14 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
> }
> }
>
> -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
> {
> trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
> if (!se->vmsd) { /* Old style */
> return se->ops->load_state(f, se->opaque, se->load_version_id);
> }
> - return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
> + return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> + errp);
> }
>
> static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> @@ -2071,6 +2072,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> {
> MigrationIncomingState *mis = migration_incoming_get_current();
> QEMUFile *f = mis->from_src_file;
> + Error *local_err = NULL;
> int load_res;
> MigrationState *migr = migrate_get_current();
>
> @@ -2089,7 +2091,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> qemu_file_set_blocking(f, true);
>
> /* TODO: sanity check that only postcopiable data will be loaded here */
> - load_res = qemu_loadvm_state_main(f, mis);
> + load_res = qemu_loadvm_state_main(f, mis, &local_err);
Here we captured the error but ignored it. AFAIU it'll be the same as
NULL..
Not sure if you tried to trigger such vTPM migration failure with postcopy
yet. AFAIU this path will be for that. To achieve your goal and make sure
the error appears for postcopy too, you may want to make use of this
local_err, probably by converting below (outside the diff context):
qemu_file_set_error(f, load_res);
...
} else {
error_report("%s: loadvm failed: %d", __func__, load_res);
migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
MIGRATION_STATUS_FAILED);
}
Into:
error_prepend(...);
migrate_set_error(s, local_err);
Some test will be needed to make sure it works.
Side note: we really should have some migration failure tests on mismatched
devices or device states / configs. We have a bunch of tests under
migration-test.c. Feel free to have a look if you like to add precopy /
postcopy unit tests for such case. It's also ok to leave that for later -
I don't want to keep piling up work for you, and I already appreciate your
help. :)
>
> /*
> * This is tricky, but, mis->from_src_file can change after it
> @@ -2394,6 +2396,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> int ret;
> size_t length;
> QIOChannelBuffer *bioc;
> + Error *local_error;
>
> length = qemu_get_be32(mis->from_src_file);
> trace_loadvm_handle_cmd_packaged(length);
> @@ -2440,7 +2443,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> qemu_coroutine_yield();
> } while (1);
>
> - ret = qemu_loadvm_state_main(packf, mis);
> + ret = qemu_loadvm_state_main(packf, mis, &local_error);
This is another piece of error report that will need to be done to the
upper layer for postcopy. So if you want to try postcopy error reporting
this needs to be propagated to caller too.
> trace_loadvm_handle_cmd_packaged_main(ret);
> qemu_fclose(packf);
> object_unref(OBJECT(bioc));
> @@ -2674,7 +2677,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
> }
>
> static int
> -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
> {
> bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
> uint32_t instance_id, version_id, section_id;
> @@ -2731,7 +2734,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> }
>
> - ret = vmstate_load(f, se);
> + ret = vmstate_load(f, se, errp);
> if (ret < 0) {
> error_report("error while loading state for instance 0x%"PRIx32" of"
> " device '%s'", instance_id, idstr);
We should try our best to keep this line. As I mentioned in the bz this
line is the most important. We could also use error_prepend() here
instead of error_report() when we have an Error**.
> @@ -2752,7 +2755,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> }
>
> static int
> -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
> {
> bool trace_downtime = (type == QEMU_VM_SECTION_END);
> int64_t start_ts, end_ts;
> @@ -2784,7 +2787,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> }
>
> - ret = vmstate_load(f, se);
> + ret = vmstate_load(f, se, errp);
> if (ret < 0) {
> error_report("error while loading state section id %d(%s)",
> section_id, se->idstr);
Same here to use error_prepend().
> @@ -2804,7 +2807,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> return 0;
> }
>
> -static int qemu_loadvm_state_header(QEMUFile *f)
> +static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
> {
> unsigned int v;
> int ret;
> @@ -2830,7 +2833,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
> error_report("Configuration section missing");
> return -EINVAL;
> }
> - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> + errp);
Ideally when we allow one function to use Error**, we'd better convert all
the error_report()s into error_setg() or other error_*() APIs. I forgot to
check that part in previous calls, but here qemu_loadvm_state_header() is
one such case. Similar comment may apply elsewhere.
>
> if (ret) {
> return ret;
> @@ -3019,7 +3023,8 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> return true;
> }
>
> -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> + Error **errp)
> {
> uint8_t section_type;
> int ret = 0;
> @@ -3037,14 +3042,14 @@ retry:
> switch (section_type) {
> case QEMU_VM_SECTION_START:
> case QEMU_VM_SECTION_FULL:
> - ret = qemu_loadvm_section_start_full(f, section_type);
> + ret = qemu_loadvm_section_start_full(f, section_type, errp);
> if (ret < 0) {
> goto out;
> }
> break;
> case QEMU_VM_SECTION_PART:
> case QEMU_VM_SECTION_END:
> - ret = qemu_loadvm_section_part_end(f, section_type);
> + ret = qemu_loadvm_section_part_end(f, section_type, errp);
> if (ret < 0) {
> goto out;
> }
Similar here, we'll need to convert current error_report() into
error_setg() for "default".
> @@ -3094,27 +3099,24 @@ out:
> return ret;
> }
>
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> {
> MigrationState *s = migrate_get_current();
> MigrationIncomingState *mis = migration_incoming_get_current();
> - Error *local_err = NULL;
> int ret;
>
> - if (qemu_savevm_state_blocked(&local_err)) {
> - error_report_err(local_err);
> + if (qemu_savevm_state_blocked(errp)) {
Another thing to be careful here: I didn't check whether errp can be NULL
here, likely it can.
In that case we'd better keep local_err, because error_setg() (inside of
qemu_savevm_state_blocked) will ignore the error otherwise..
static void error_setv(Error **errp,
const char *src, int line, const char *func,
ErrorClass err_class, const char *fmt, va_list ap,
const char *suffix)
{
Error *err;
int saved_errno = errno;
if (errp == NULL) {
return;
}
assert(*errp == NULL);
...
}
So here we can keep local_err but use error_propagate().
> return -EINVAL;
> }
>
> qemu_loadvm_thread_pool_create(mis);
>
> - ret = qemu_loadvm_state_header(f);
> + ret = qemu_loadvm_state_header(f, errp);
> if (ret) {
> return ret;
> }
>
> - if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> - error_report_err(local_err);
> + if (qemu_loadvm_state_setup(f, errp) != 0) {
> return -EINVAL;
> }
>
> @@ -3124,7 +3126,7 @@ int qemu_loadvm_state(QEMUFile *f)
>
> cpu_synchronize_all_pre_loadvm();
>
> - ret = qemu_loadvm_state_main(f, mis);
> + ret = qemu_loadvm_state_main(f, mis, errp);
> qemu_event_set(&mis->main_thread_load_event);
>
> trace_qemu_loadvm_state_post_main(ret);
> @@ -3192,13 +3194,13 @@ int qemu_loadvm_state(QEMUFile *f)
> return ret;
> }
>
> -int qemu_load_device_state(QEMUFile *f)
> +int qemu_load_device_state(QEMUFile *f, Error **errp)
> {
> MigrationIncomingState *mis = migration_incoming_get_current();
> int ret;
>
> /* Load QEMU_VM_SECTION_FULL section */
> - ret = qemu_loadvm_state_main(f, mis);
> + ret = qemu_loadvm_state_main(f, mis, errp);
> if (ret < 0) {
> error_report("Failed to load device state: %d", ret);
Prone to merge errors using error_prepend().
> return ret;
> @@ -3429,7 +3431,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
> f = qemu_file_new_input(QIO_CHANNEL(ioc));
> object_unref(OBJECT(ioc));
>
> - ret = qemu_loadvm_state(f);
> + ret = qemu_loadvm_state(f, errp);
> qemu_fclose(f);
> if (ret < 0) {
> error_setg(errp, "loading Xen device state failed");
Prone to crash when errp set.
> @@ -3503,7 +3505,7 @@ bool load_snapshot(const char *name, const char *vmstate,
> ret = -EINVAL;
> goto err_drain;
> }
> - ret = qemu_loadvm_state(f);
> + ret = qemu_loadvm_state(f, errp);
Prone to crash, see lines below.
> migration_incoming_state_destroy();
>
> bdrv_drain_all_end();
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 2d5e9c716686f06720325e82fe90c75335ced1de..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -64,10 +64,11 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
> void qemu_savevm_live_state(QEMUFile *f);
> int qemu_save_device_state(QEMUFile *f);
>
> -int qemu_loadvm_state(QEMUFile *f);
> +int qemu_loadvm_state(QEMUFile *f, Error **errp);
> void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> -int qemu_load_device_state(QEMUFile *f);
> +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> + Error **errp);
> +int qemu_load_device_state(QEMUFile *f, Error **errp);
> int qemu_loadvm_approve_switchover(void);
> int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> bool in_postcopy);
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..1c5b76e1dd198030847971bc35637867c9d54fc0 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -549,7 +549,7 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
>
> /* Writes the parent field which is at the start of the tmp */
> *(void **)tmp = pv;
> - ret = vmstate_load_state(f, vmsd, tmp, version_id);
> + ret = vmstate_load_state(f, vmsd, tmp, version_id, NULL);
> g_free(tmp);
> return ret;
> }
> @@ -649,7 +649,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>
> while (qemu_get_byte(f)) {
> elm = g_malloc(size);
> - ret = vmstate_load_state(f, vmsd, elm, version_id);
> + ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
> if (ret) {
> return ret;
> }
> @@ -803,7 +803,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> key = (void *)(uintptr_t)qemu_get_be64(f);
> } else {
> key = g_malloc0(key_size);
> - ret = vmstate_load_state(f, key_vmsd, key, version_id);
> + ret = vmstate_load_state(f, key_vmsd, key, version_id, NULL);
> if (ret) {
> error_report("%s : failed to load %s (%d)",
> field->name, key_vmsd->name, ret);
> @@ -811,7 +811,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> }
> }
> val = g_malloc0(val_size);
> - ret = vmstate_load_state(f, val_vmsd, val, version_id);
> + ret = vmstate_load_state(f, val_vmsd, val, version_id, NULL);
> if (ret) {
> error_report("%s : failed to load %s (%d)",
> field->name, val_vmsd->name, ret);
> @@ -892,7 +892,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
>
> while (qemu_get_byte(f)) {
> elm = g_malloc(size);
> - ret = vmstate_load_state(f, vmsd, elm, version_id);
> + ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
> if (ret) {
> error_report("%s: failed to load %s (%d)", field->name,
> vmsd->name, ret);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5feaa3244d259874f03048326b2497e7db32e47c..177c563ff103ada2e494c14173fa773d52adb800 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> void *opaque, JSONWriter *vmdesc,
> Error **errp);
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque);
> + void *opaque, Error **errp);
>
> /* Whether this field should exist for either save or load the VM? */
> static bool
> @@ -132,7 +132,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> }
>
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque, int version_id)
> + void *opaque, int version_id, Error **errp)
> {
> const VMStateField *field = vmsd->fields;
> int ret = 0;
> @@ -192,10 +192,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>
> if (inner_field->flags & VMS_STRUCT) {
> ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> - inner_field->vmsd->version_id);
> + inner_field->vmsd->version_id,
> + errp);
> } else if (inner_field->flags & VMS_VSTRUCT) {
> ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> - inner_field->struct_version_id);
> + inner_field->struct_version_id,
> + errp);
> } else {
> ret = inner_field->info->get(f, curr_elem, size,
> inner_field);
> @@ -225,7 +227,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> field++;
> }
> assert(field->flags == VMS_END);
> - ret = vmstate_subsection_load(f, vmsd, opaque);
> + ret = vmstate_subsection_load(f, vmsd, opaque, errp);
> if (ret != 0) {
> qemu_file_set_error(f, ret);
> return ret;
Need to convert all error_reports() in this function.
> @@ -566,7 +568,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> }
>
> static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> - void *opaque)
> + void *opaque, Error **errp)
> {
> trace_vmstate_subsection_load(vmsd->name);
>
> @@ -605,7 +607,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> qemu_file_skip(f, len); /* idstr */
> version_id = qemu_get_be32(f);
>
> - ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> + ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> if (ret) {
> trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> return ret;
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index 63f28f26f45691a70936d33e7341d16477a3471f..ca5e0ba1e3e5e2bb0a1ce39143a292f2c6f9420a 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -114,7 +114,7 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> qemu_fclose(f);
>
> f = open_test_file(false);
> - ret = vmstate_load_state(f, desc, obj, version);
> + ret = vmstate_load_state(f, desc, obj, version, NULL);
> if (ret) {
> g_assert(qemu_file_get_error(f));
> } else{
> @@ -365,7 +365,7 @@ static void test_load_v1(void)
>
> QEMUFile *loading = open_test_file(false);
> TestStruct obj = { .b = 200, .e = 500, .f = 600 };
> - vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
> + vmstate_load_state(loading, &vmstate_versioned, &obj, 1, NULL);
> g_assert(!qemu_file_get_error(loading));
> g_assert_cmpint(obj.a, ==, 10);
> g_assert_cmpint(obj.b, ==, 200);
> @@ -391,7 +391,7 @@ static void test_load_v2(void)
>
> QEMUFile *loading = open_test_file(false);
> TestStruct obj;
> - vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
> + vmstate_load_state(loading, &vmstate_versioned, &obj, 2, NULL);
> g_assert_cmpint(obj.a, ==, 10);
> g_assert_cmpint(obj.b, ==, 20);
> g_assert_cmpint(obj.c, ==, 30);
> @@ -480,7 +480,7 @@ static void test_load_noskip(void)
>
> QEMUFile *loading = open_test_file(false);
> TestStruct obj = { .skip_c_e = false };
> - vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> + vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
> g_assert(!qemu_file_get_error(loading));
> g_assert_cmpint(obj.a, ==, 10);
> g_assert_cmpint(obj.b, ==, 20);
> @@ -504,7 +504,7 @@ static void test_load_skip(void)
>
> QEMUFile *loading = open_test_file(false);
> TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
> - vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> + vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
> g_assert(!qemu_file_get_error(loading));
> g_assert_cmpint(obj.a, ==, 10);
> g_assert_cmpint(obj.b, ==, 20);
> @@ -773,7 +773,7 @@ static void test_load_q(void)
> TestQtailq tgt;
>
> QTAILQ_INIT(&tgt.q);
> - vmstate_load_state(fload, &vmstate_q, &tgt, 1);
> + vmstate_load_state(fload, &vmstate_q, &tgt, 1, NULL);
> char eof = qemu_get_byte(fload);
> g_assert(!qemu_file_get_error(fload));
> g_assert_cmpint(tgt.i16, ==, obj_q.i16);
> @@ -1127,7 +1127,7 @@ static void test_gtree_load_domain(void)
>
> fload = open_test_file(false);
>
> - vmstate_load_state(fload, &vmstate_domain, dest_domain, 1);
> + vmstate_load_state(fload, &vmstate_domain, dest_domain, 1, NULL);
> eof = qemu_get_byte(fload);
> g_assert(!qemu_file_get_error(fload));
> g_assert_cmpint(orig_domain->id, ==, dest_domain->id);
> @@ -1241,7 +1241,7 @@ static void test_gtree_load_iommu(void)
> qemu_fclose(fsave);
>
> fload = open_test_file(false);
> - vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1);
> + vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, NULL);
> eof = qemu_get_byte(fload);
> g_assert(!qemu_file_get_error(fload));
> g_assert_cmpint(orig_iommu->id, ==, dest_iommu->id);
> @@ -1376,7 +1376,7 @@ static void test_load_qlist(void)
> qemu_fclose(fsave);
>
> fload = open_test_file(false);
> - vmstate_load_state(fload, &vmstate_container, dest_container, 1);
> + vmstate_load_state(fload, &vmstate_container, dest_container, 1, NULL);
> eof = qemu_get_byte(fload);
> g_assert(!qemu_file_get_error(fload));
> g_assert_cmpint(eof, ==, QEMU_VM_EOF);
>
> --
> 2.49.0
>
--
Peter Xu
Hi Peter,
Thank you so much for the review.
I shall squash patches 1 and 2 and add error_prepend()
where I missed them; also adding a null check.
On Tue, Jun 24, 2025 at 09:46:48AM -0400, Peter Xu wrote:
> Hi, Arun,
>
> On Tue, Jun 24, 2025 at 05:53:04PM +0530, Arun Menon wrote:
> > - This is an incremental step in converting vmstate loading
> > code to report errors.
> > - Minimal changes to the signature and body of the following
> > functions are done,
> > - vmstate_load()
> > - vmstate_load_state()
> > - vmstate_subsection_load()
> > - qemu_load_device_state()
> > - qemu_loadvm_state()
> > - qemu_loadvm_section_start_full()
> > - qemu_loadvm_section_part_end()
> > - qemu_loadvm_state_header()
> > - qemu_loadvm_state_main()
> >
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> > hw/display/virtio-gpu.c | 2 +-
> > hw/pci/pci.c | 2 +-
> > hw/s390x/virtio-ccw.c | 2 +-
> > hw/scsi/spapr_vscsi.c | 2 +-
> > hw/vfio/pci.c | 2 +-
> > hw/virtio/virtio-mmio.c | 2 +-
> > hw/virtio/virtio-pci.c | 2 +-
> > hw/virtio/virtio.c | 4 ++--
> > include/migration/vmstate.h | 2 +-
> > migration/colo.c | 4 ++--
> > migration/cpr.c | 2 +-
> > migration/migration.c | 2 +-
> > migration/savevm.c | 52 +++++++++++++++++++++++----------------------
> > migration/savevm.h | 7 +++---
> > migration/vmstate-types.c | 10 ++++-----
> > migration/vmstate.c | 16 ++++++++------
> > tests/unit/test-vmstate.c | 18 ++++++++--------
> > 17 files changed, 68 insertions(+), 63 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5d2ca8d8b864350133a674802d7316abd379591c 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1343,7 +1343,7 @@ 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);
> > + vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, NULL);
> >
> > return 0;
> > }
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..2ab5d30bb3c319ac1c7bfc9a2acf6a2b38082066 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -934,7 +934,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> > int pci_device_load(PCIDevice *s, QEMUFile *f)
> > {
> > int ret;
> > - ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> > + ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id, NULL);
> > /* Restore the interrupt status bit. */
> > pci_update_irq_status(s);
> > return ret;
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..2f6feff2b0a22d7d7f6aecfd7e7870d8362f1a73 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> > static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> > {
> > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> > - return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> > + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, NULL);
> > }
> >
> > static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> > index 20f70fb2729de78b9636a6b8c869695dab4f8902..573fdea668536b464bca11f001e9e0288e781493 100644
> > --- a/hw/scsi/spapr_vscsi.c
> > +++ b/hw/scsi/spapr_vscsi.c
> > @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
> > assert(!req->active);
> >
> > memset(req, 0, sizeof(*req));
> > - rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
> > + rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, NULL);
> > if (rc) {
> > fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
> > return NULL;
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index fa25bded25c51f8efb6c5ad31bd90506cd69745c..87aee0a5701087f9a68ea435bb96e9d6b07b0c24 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2715,7 +2715,7 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> > old_addr[bar] = pdev->io_regions[bar].addr;
> > }
> >
> > - ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
> > + ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1, NULL);
> > if (ret) {
> > return ret;
> > }
> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..9058b1563462d4464dcba799643a583c93fb5683 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -619,7 +619,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
> > {
> > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
> >
> > - return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
> > + return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, NULL);
> > }
> >
> > static bool virtio_mmio_has_extra_state(DeviceState *opaque)
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index fba2372c93bfd648736b07e4bc83e7097baa58cb..50a1f5701754b88e8a1ee062d6eeedfd848cb4f5 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -160,7 +160,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
> > {
> > VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >
> > - return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
> > + return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, NULL);
> > }
> >
> > static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 82a285a31d1c0427d55f7cb73398adfc94e678fe..66d5941f68a4b9e1e5390bb0aa45fc6cd34e2a1e 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3317,14 +3317,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> > }
> >
> > if (vdc->vmsd) {
> > - ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
> > + ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, NULL);
> > if (ret) {
> > return ret;
> > }
> > }
> >
> > /* Subsections */
> > - ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
> > + ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, NULL);
> > if (ret) {
> > return ret;
> > }
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
> > }
> >
> > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > - void *opaque, int version_id);
> > + void *opaque, int version_id, Error **errp);
> > int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> > void *opaque, JSONWriter *vmdesc);
> > int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> > diff --git a/migration/colo.c b/migration/colo.c
> > index e0f713c837f5da25d67afbd02ceb6c54024ca3af..c7779683f0aad33cd071030ac553da69d6a5e60d 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -686,7 +686,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >
> > bql_lock();
> > cpu_synchronize_all_states();
> > - ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > + ret = qemu_loadvm_state_main(mis->from_src_file, mis, &local_err);
> > bql_unlock();
> >
> > if (ret < 0) {
>
> Here the diff didn't show, but it's:
>
> if (ret < 0) {
> error_setg(errp, "Load VM's live state (ram) error");
> return;
> }
>
> Note that error_setg() asserts *errp==NULL. I think this will crash qemu
> when load fails for COLO. AFAIU we need to use error_prepend() for such
> cases.
Agreed.
>
> > @@ -729,7 +729,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> > bql_lock();
> > vmstate_loading = true;
> > colo_flush_ram_cache();
> > - ret = qemu_load_device_state(fb);
> > + ret = qemu_load_device_state(fb, &local_err);
> > if (ret < 0) {
> > error_setg(errp, "COLO: load device state failed");
>
> Same here.
Agreed.
>
> >
vmstate_loading = false;
> > diff --git a/migration/cpr.c b/migration/cpr.c
> > index a50a57edca754b50e68fa9c294b3c89791e62ba8..0fb9fadac905c83689eed2b1193b282da679b6ef 100644
> > --- a/migration/cpr.c
> > +++ b/migration/cpr.c
> > @@ -235,7 +235,7 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
> > return -ENOTSUP;
> > }
> >
> > - ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> > + ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> > if (ret) {
> > error_setg(errp, "vmstate_load_state error %d", ret);
>
> Same here.
Agreed.
>
> > qemu_fclose(f);
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 4098870bce33ffdc57b5972fc5b106d88abb237e..5cabb4e7307323159241ff35781db7f1c665a75b 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -876,7 +876,7 @@ process_incoming_migration_co(void *opaque)
> > MIGRATION_STATUS_ACTIVE);
> >
> > mis->loadvm_co = qemu_coroutine_self();
> > - ret = qemu_loadvm_state(mis->from_src_file);
> > + ret = qemu_loadvm_state(mis->from_src_file, &local_err);
>
> Same. This one you need to scroll a bit until:
>
> if (ret < 0) {
> error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
> goto fail;
> }
Yes, agreed.
>
> > mis->loadvm_co = NULL;
> >
> > trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index bb04a4520df9a443d90cf6cb52a383a5f053aaff..9bcc0935781b73e209dc57945f9dbb381283cad5 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -963,13 +963,14 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
> > }
> > }
> >
> > -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> > +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
> > {
> > trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
> > if (!se->vmsd) { /* Old style */
> > return se->ops->load_state(f, se->opaque, se->load_version_id);
> > }
> > - return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
> > + return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > + errp);
> > }
> >
> > static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > @@ -2071,6 +2072,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > {
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > QEMUFile *f = mis->from_src_file;
> > + Error *local_err = NULL;
> > int load_res;
> > MigrationState *migr = migrate_get_current();
> >
> > @@ -2089,7 +2091,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > qemu_file_set_blocking(f, true);
> >
> > /* TODO: sanity check that only postcopiable data will be loaded here */
> > - load_res = qemu_loadvm_state_main(f, mis);
> > + load_res = qemu_loadvm_state_main(f, mis, &local_err);
>
> Here we captured the error but ignored it. AFAIU it'll be the same as
> NULL..
>
> Not sure if you tried to trigger such vTPM migration failure with postcopy
> yet. AFAIU this path will be for that. To achieve your goal and make sure
> the error appears for postcopy too, you may want to make use of this
> local_err, probably by converting below (outside the diff context):
>
> qemu_file_set_error(f, load_res);
> ...
>
> } else {
> error_report("%s: loadvm failed: %d", __func__, load_res);
> migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> MIGRATION_STATUS_FAILED);
> }
>
> Into:
>
> error_prepend(...);
> migrate_set_error(s, local_err);
>
> Some test will be needed to make sure it works.
>
> Side note: we really should have some migration failure tests on mismatched
> devices or device states / configs. We have a bunch of tests under
> migration-test.c. Feel free to have a look if you like to add precopy /
> postcopy unit tests for such case. It's also ok to leave that for later -
> I don't want to keep piling up work for you, and I already appreciate your
> help. :)
>
Yes, the postcopy errors also need to be propagated.
I still need to figure out to build a test case around post-copy.
Maybe we can do that in another commit/ticket.
> >
> > /*
> > * This is tricky, but, mis->from_src_file can change after it
> > @@ -2394,6 +2396,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> > int ret;
> > size_t length;
> > QIOChannelBuffer *bioc;
> > + Error *local_error;
> >
> > length = qemu_get_be32(mis->from_src_file);
> > trace_loadvm_handle_cmd_packaged(length);
> > @@ -2440,7 +2443,7 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> > qemu_coroutine_yield();
> > } while (1);
> >
> > - ret = qemu_loadvm_state_main(packf, mis);
> > + ret = qemu_loadvm_state_main(packf, mis, &local_error);
>
> This is another piece of error report that will need to be done to the
> upper layer for postcopy. So if you want to try postcopy error reporting
> this needs to be propagated to caller too.
>
Yes, the postcopy errors need to be propagated.
> > trace_loadvm_handle_cmd_packaged_main(ret);
> > qemu_fclose(packf);
> > object_unref(OBJECT(bioc));
> > @@ -2674,7 +2677,7 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
> > }
> >
> > static int
> > -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
> > {
> > bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
> > uint32_t instance_id, version_id, section_id;
> > @@ -2731,7 +2734,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> > start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> > }
> >
> > - ret = vmstate_load(f, se);
> > + ret = vmstate_load(f, se, errp);
> > if (ret < 0) {
> > error_report("error while loading state for instance 0x%"PRIx32" of"
> > " device '%s'", instance_id, idstr);
>
> We should try our best to keep this line. As I mentioned in the bz this
> line is the most important. We could also use error_prepend() here
> instead of error_report() when we have an Error**.
Yes.
>
> > @@ -2752,7 +2755,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> > }
> >
> > static int
> > -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
> > {
> > bool trace_downtime = (type == QEMU_VM_SECTION_END);
> > int64_t start_ts, end_ts;
> > @@ -2784,7 +2787,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> > start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> > }
> >
> > - ret = vmstate_load(f, se);
> > + ret = vmstate_load(f, se, errp);
> > if (ret < 0) {
> > error_report("error while loading state section id %d(%s)",
> > section_id, se->idstr);
>
> Same here to use error_prepend().
Agreed.
>
> > @@ -2804,7 +2807,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> > return 0;
> > }
> >
> > -static int qemu_loadvm_state_header(QEMUFile *f)
> > +static int qemu_loadvm_state_header(QEMUFile *f, Error **errp)
> > {
> > unsigned int v;
> > int ret;
> > @@ -2830,7 +2833,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
> > error_report("Configuration section missing");
> > return -EINVAL;
> > }
> > - ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> > + errp);
>
> Ideally when we allow one function to use Error**, we'd better convert all
> the error_report()s into error_setg() or other error_*() APIs. I forgot to
> check that part in previous calls, but here qemu_loadvm_state_header() is
> one such case. Similar comment may apply elsewhere.
I added error_setg() or error_prepend() in functions
where errp was passed in the savevm.c file, more or less everywhere.
I shall recheck.
>
> >
> > if (ret) {
> > return ret;
> > @@ -3019,7 +3023,8 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > return true;
> > }
> >
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > + Error **errp)
> > {
> > uint8_t section_type;
> > int ret = 0;
> > @@ -3037,14 +3042,14 @@ retry:
> > switch (section_type) {
> > case QEMU_VM_SECTION_START:
> > case QEMU_VM_SECTION_FULL:
> > - ret = qemu_loadvm_section_start_full(f, section_type);
> > + ret = qemu_loadvm_section_start_full(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
> > break;
> > case QEMU_VM_SECTION_PART:
> > case QEMU_VM_SECTION_END:
> > - ret = qemu_loadvm_section_part_end(f, section_type);
> > + ret = qemu_loadvm_section_part_end(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
>
> Similar here, we'll need to convert current error_report() into
> error_setg() for "default".
Agreed. I missed this in patch 2
>
> > @@ -3094,27 +3099,24 @@ out:
> > return ret;
> > }
> >
> > -int qemu_loadvm_state(QEMUFile *f)
> > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> > {
> > MigrationState *s = migrate_get_current();
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > - Error *local_err = NULL;
> > int ret;
> >
> > - if (qemu_savevm_state_blocked(&local_err)) {
> > - error_report_err(local_err);
> > + if (qemu_savevm_state_blocked(errp)) {
>
> Another thing to be careful here: I didn't check whether errp can be NULL
> here, likely it can.
>
> In that case we'd better keep local_err, because error_setg() (inside of
> qemu_savevm_state_blocked) will ignore the error otherwise..
>
> static void error_setv(Error **errp,
> const char *src, int line, const char *func,
> ErrorClass err_class, const char *fmt, va_list ap,
> const char *suffix)
> {
> Error *err;
> int saved_errno = errno;
>
> if (errp == NULL) {
> return;
> }
> assert(*errp == NULL);
> ...
> }
>
> So here we can keep local_err but use error_propagate().
Please correct me if I am wrong, as far as I have checked,
qemu_loadvm_state() is called at 3 places
- load_snapshot
- qmp_xen_load_devices_state
- process_incoming_migration_co
and in all these functions, Error **errp is passed.
I did not find a function that passes NULL.
Is it still required to declare and pass a local_err object?
>
> > return -EINVAL;
> > }
> >
> > qemu_loadvm_thread_pool_create(mis);
> >
> > - ret = qemu_loadvm_state_header(f);
> > + ret = qemu_loadvm_state_header(f, errp);
> > if (ret) {
> > return ret;
> > }
> >
> > - if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> > - error_report_err(local_err);
> > + if (qemu_loadvm_state_setup(f, errp) != 0) {
> > return -EINVAL;
> > }
> >
> > @@ -3124,7 +3126,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >
> > cpu_synchronize_all_pre_loadvm();
> >
> > - ret = qemu_loadvm_state_main(f, mis);
> > + ret = qemu_loadvm_state_main(f, mis, errp);
> > qemu_event_set(&mis->main_thread_load_event);
> >
> > trace_qemu_loadvm_state_post_main(ret);
> > @@ -3192,13 +3194,13 @@ int qemu_loadvm_state(QEMUFile *f)
> > return ret;
> > }
> >
> > -int qemu_load_device_state(QEMUFile *f)
> > +int qemu_load_device_state(QEMUFile *f, Error **errp)
> > {
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > int ret;
> >
> > /* Load QEMU_VM_SECTION_FULL section */
> > - ret = qemu_loadvm_state_main(f, mis);
> > + ret = qemu_loadvm_state_main(f, mis, errp);
> > if (ret < 0) {
> > error_report("Failed to load device state: %d", ret);
>
> Prone to merge errors using error_prepend().
Agreed.
>
> > return ret;
> > @@ -3429,7 +3431,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
> > f = qemu_file_new_input(QIO_CHANNEL(ioc));
> > object_unref(OBJECT(ioc));
> >
> > - ret = qemu_loadvm_state(f);
> > + ret = qemu_loadvm_state(f, errp);
> > qemu_fclose(f);
> > if (ret < 0) {
> > error_setg(errp, "loading Xen device state failed");
>
> Prone to crash when errp set.
Agreed.
>
> > @@ -3503,7 +3505,7 @@ bool load_snapshot(const char *name, const char *vmstate,
> > ret = -EINVAL;
> > goto err_drain;
> > }
> > - ret = qemu_loadvm_state(f);
> > + ret = qemu_loadvm_state(f, errp);
>
> Prone to crash, see lines below.
Agreed.
>
> > migration_incoming_state_destroy();
> >
> > bdrv_drain_all_end();
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index 2d5e9c716686f06720325e82fe90c75335ced1de..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -64,10 +64,11 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
> > void qemu_savevm_live_state(QEMUFile *f);
> > int qemu_save_device_state(QEMUFile *f);
> >
> > -int qemu_loadvm_state(QEMUFile *f);
> > +int qemu_loadvm_state(QEMUFile *f, Error **errp);
> > void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > -int qemu_load_device_state(QEMUFile *f);
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > + Error **errp);
> > +int qemu_load_device_state(QEMUFile *f, Error **errp);
> > int qemu_loadvm_approve_switchover(void);
> > int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> > bool in_postcopy);
> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> > index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..1c5b76e1dd198030847971bc35637867c9d54fc0 100644
> > --- a/migration/vmstate-types.c
> > +++ b/migration/vmstate-types.c
> > @@ -549,7 +549,7 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
> >
> > /* Writes the parent field which is at the start of the tmp */
> > *(void **)tmp = pv;
> > - ret = vmstate_load_state(f, vmsd, tmp, version_id);
> > + ret = vmstate_load_state(f, vmsd, tmp, version_id, NULL);
> > g_free(tmp);
> > return ret;
> > }
> > @@ -649,7 +649,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >
> > while (qemu_get_byte(f)) {
> > elm = g_malloc(size);
> > - ret = vmstate_load_state(f, vmsd, elm, version_id);
> > + ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
> > if (ret) {
> > return ret;
> > }
> > @@ -803,7 +803,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> > key = (void *)(uintptr_t)qemu_get_be64(f);
> > } else {
> > key = g_malloc0(key_size);
> > - ret = vmstate_load_state(f, key_vmsd, key, version_id);
> > + ret = vmstate_load_state(f, key_vmsd, key, version_id, NULL);
> > if (ret) {
> > error_report("%s : failed to load %s (%d)",
> > field->name, key_vmsd->name, ret);
> > @@ -811,7 +811,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> > }
> > }
> > val = g_malloc0(val_size);
> > - ret = vmstate_load_state(f, val_vmsd, val, version_id);
> > + ret = vmstate_load_state(f, val_vmsd, val, version_id, NULL);
> > if (ret) {
> > error_report("%s : failed to load %s (%d)",
> > field->name, val_vmsd->name, ret);
> > @@ -892,7 +892,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
> >
> > while (qemu_get_byte(f)) {
> > elm = g_malloc(size);
> > - ret = vmstate_load_state(f, vmsd, elm, version_id);
> > + ret = vmstate_load_state(f, vmsd, elm, version_id, NULL);
> > if (ret) {
> > error_report("%s: failed to load %s (%d)", field->name,
> > vmsd->name, ret);
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 5feaa3244d259874f03048326b2497e7db32e47c..177c563ff103ada2e494c14173fa773d52adb800 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> > void *opaque, JSONWriter *vmdesc,
> > Error **errp);
> > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> > - void *opaque);
> > + void *opaque, Error **errp);
> >
> > /* Whether this field should exist for either save or load the VM? */
> > static bool
> > @@ -132,7 +132,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> > }
> >
> > int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > - void *opaque, int version_id)
> > + void *opaque, int version_id, Error **errp)
> > {
> > const VMStateField *field = vmsd->fields;
> > int ret = 0;
> > @@ -192,10 +192,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >
> > if (inner_field->flags & VMS_STRUCT) {
> > ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > - inner_field->vmsd->version_id);
> > + inner_field->vmsd->version_id,
> > + errp);
> > } else if (inner_field->flags & VMS_VSTRUCT) {
> > ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > - inner_field->struct_version_id);
> > + inner_field->struct_version_id,
> > + errp);
> > } else {
> > ret = inner_field->info->get(f, curr_elem, size,
> > inner_field);
> > @@ -225,7 +227,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > field++;
> > }
> > assert(field->flags == VMS_END);
> > - ret = vmstate_subsection_load(f, vmsd, opaque);
> > + ret = vmstate_subsection_load(f, vmsd, opaque, errp);
> > if (ret != 0) {
> > qemu_file_set_error(f, ret);
> > return ret;
>
> Need to convert all error_reports() in this function.
Yes, should be done after squash.
>
> > @@ -566,7 +568,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> > }
> >
> > static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> > - void *opaque)
> > + void *opaque, Error **errp)
> > {
> > trace_vmstate_subsection_load(vmsd->name);
> >
> > @@ -605,7 +607,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> > qemu_file_skip(f, len); /* idstr */
> > version_id = qemu_get_be32(f);
> >
> > - ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> > + ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> > if (ret) {
> > trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> > return ret;
> > diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> > index 63f28f26f45691a70936d33e7341d16477a3471f..ca5e0ba1e3e5e2bb0a1ce39143a292f2c6f9420a 100644
> > --- a/tests/unit/test-vmstate.c
> > +++ b/tests/unit/test-vmstate.c
> > @@ -114,7 +114,7 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> > qemu_fclose(f);
> >
> > f = open_test_file(false);
> > - ret = vmstate_load_state(f, desc, obj, version);
> > + ret = vmstate_load_state(f, desc, obj, version, NULL);
> > if (ret) {
> > g_assert(qemu_file_get_error(f));
> > } else{
> > @@ -365,7 +365,7 @@ static void test_load_v1(void)
> >
> > QEMUFile *loading = open_test_file(false);
> > TestStruct obj = { .b = 200, .e = 500, .f = 600 };
> > - vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
> > + vmstate_load_state(loading, &vmstate_versioned, &obj, 1, NULL);
> > g_assert(!qemu_file_get_error(loading));
> > g_assert_cmpint(obj.a, ==, 10);
> > g_assert_cmpint(obj.b, ==, 200);
> > @@ -391,7 +391,7 @@ static void test_load_v2(void)
> >
> > QEMUFile *loading = open_test_file(false);
> > TestStruct obj;
> > - vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
> > + vmstate_load_state(loading, &vmstate_versioned, &obj, 2, NULL);
> > g_assert_cmpint(obj.a, ==, 10);
> > g_assert_cmpint(obj.b, ==, 20);
> > g_assert_cmpint(obj.c, ==, 30);
> > @@ -480,7 +480,7 @@ static void test_load_noskip(void)
> >
> > QEMUFile *loading = open_test_file(false);
> > TestStruct obj = { .skip_c_e = false };
> > - vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> > + vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
> > g_assert(!qemu_file_get_error(loading));
> > g_assert_cmpint(obj.a, ==, 10);
> > g_assert_cmpint(obj.b, ==, 20);
> > @@ -504,7 +504,7 @@ static void test_load_skip(void)
> >
> > QEMUFile *loading = open_test_file(false);
> > TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
> > - vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> > + vmstate_load_state(loading, &vmstate_skipping, &obj, 2, NULL);
> > g_assert(!qemu_file_get_error(loading));
> > g_assert_cmpint(obj.a, ==, 10);
> > g_assert_cmpint(obj.b, ==, 20);
> > @@ -773,7 +773,7 @@ static void test_load_q(void)
> > TestQtailq tgt;
> >
> > QTAILQ_INIT(&tgt.q);
> > - vmstate_load_state(fload, &vmstate_q, &tgt, 1);
> > + vmstate_load_state(fload, &vmstate_q, &tgt, 1, NULL);
> > char eof = qemu_get_byte(fload);
> > g_assert(!qemu_file_get_error(fload));
> > g_assert_cmpint(tgt.i16, ==, obj_q.i16);
> > @@ -1127,7 +1127,7 @@ static void test_gtree_load_domain(void)
> >
> > fload = open_test_file(false);
> >
> > - vmstate_load_state(fload, &vmstate_domain, dest_domain, 1);
> > + vmstate_load_state(fload, &vmstate_domain, dest_domain, 1, NULL);
> > eof = qemu_get_byte(fload);
> > g_assert(!qemu_file_get_error(fload));
> > g_assert_cmpint(orig_domain->id, ==, dest_domain->id);
> > @@ -1241,7 +1241,7 @@ static void test_gtree_load_iommu(void)
> > qemu_fclose(fsave);
> >
> > fload = open_test_file(false);
> > - vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1);
> > + vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, NULL);
> > eof = qemu_get_byte(fload);
> > g_assert(!qemu_file_get_error(fload));
> > g_assert_cmpint(orig_iommu->id, ==, dest_iommu->id);
> > @@ -1376,7 +1376,7 @@ static void test_load_qlist(void)
> > qemu_fclose(fsave);
> >
> > fload = open_test_file(false);
> > - vmstate_load_state(fload, &vmstate_container, dest_container, 1);
> > + vmstate_load_state(fload, &vmstate_container, dest_container, 1, NULL);
> > eof = qemu_get_byte(fload);
> > g_assert(!qemu_file_get_error(fload));
> > g_assert_cmpint(eof, ==, QEMU_VM_EOF);
> >
> > --
> > 2.49.0
> >
>
> --
> Peter Xu
>
Regards,
Arun
On Wed, Jun 25, 2025 at 05:24:10PM +0530, Arun Menon wrote:
> Hi Peter,
Hi, Arun,
[...]
> > > static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > > @@ -2071,6 +2072,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > {
> > > MigrationIncomingState *mis = migration_incoming_get_current();
> > > QEMUFile *f = mis->from_src_file;
> > > + Error *local_err = NULL;
> > > int load_res;
> > > MigrationState *migr = migrate_get_current();
> > >
> > > @@ -2089,7 +2091,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > qemu_file_set_blocking(f, true);
> > >
> > > /* TODO: sanity check that only postcopiable data will be loaded here */
> > > - load_res = qemu_loadvm_state_main(f, mis);
> > > + load_res = qemu_loadvm_state_main(f, mis, &local_err);
> >
> > Here we captured the error but ignored it. AFAIU it'll be the same as
> > NULL..
> >
> > Not sure if you tried to trigger such vTPM migration failure with postcopy
> > yet. AFAIU this path will be for that. To achieve your goal and make sure
> > the error appears for postcopy too, you may want to make use of this
> > local_err, probably by converting below (outside the diff context):
> >
> > qemu_file_set_error(f, load_res);
> > ...
> >
> > } else {
> > error_report("%s: loadvm failed: %d", __func__, load_res);
> > migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > MIGRATION_STATUS_FAILED);
> > }
> >
> > Into:
> >
> > error_prepend(...);
> > migrate_set_error(s, local_err);
> >
> > Some test will be needed to make sure it works.
> >
> > Side note: we really should have some migration failure tests on mismatched
> > devices or device states / configs. We have a bunch of tests under
> > migration-test.c. Feel free to have a look if you like to add precopy /
> > postcopy unit tests for such case. It's also ok to leave that for later -
> > I don't want to keep piling up work for you, and I already appreciate your
> > help. :)
> >
> Yes, the postcopy errors also need to be propagated.
> I still need to figure out to build a test case around post-copy.
> Maybe we can do that in another commit/ticket.
Yes, for now it'll be good enough if the fix can cover postcopy for the
same issue. Manual test should be OK to switch postcopy rightaway in the
wrong vTPM setup, and one should observe failure already, checking the
query-migrate on dest on errors.
It's possible that postcopy may still have an issue that might hang src
QEMU if it fails exactly at device transfer stage; I may need to have a
closer look later and see whether it's true and if it's fixable - I think
it is. The ideal way is if device state migration failed when
postcopy_start(), then we should fail the migration and continue on src
QEMU (like a precopy failure, as postcopy hasn't yet really started).
For this specific issue, as long as the error will pop up correctly on dest
after postcopy failed, it should be good.
The auto test case can be for later.
[...]
> > > @@ -3094,27 +3099,24 @@ out:
> > > return ret;
> > > }
> > >
> > > -int qemu_loadvm_state(QEMUFile *f)
> > > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> > > {
> > > MigrationState *s = migrate_get_current();
> > > MigrationIncomingState *mis = migration_incoming_get_current();
> > > - Error *local_err = NULL;
> > > int ret;
> > >
> > > - if (qemu_savevm_state_blocked(&local_err)) {
> > > - error_report_err(local_err);
> > > + if (qemu_savevm_state_blocked(errp)) {
> >
> > Another thing to be careful here: I didn't check whether errp can be NULL
> > here, likely it can.
> >
> > In that case we'd better keep local_err, because error_setg() (inside of
> > qemu_savevm_state_blocked) will ignore the error otherwise..
> >
> > static void error_setv(Error **errp,
> > const char *src, int line, const char *func,
> > ErrorClass err_class, const char *fmt, va_list ap,
> > const char *suffix)
> > {
> > Error *err;
> > int saved_errno = errno;
> >
> > if (errp == NULL) {
> > return;
> > }
> > assert(*errp == NULL);
> > ...
> > }
> >
> > So here we can keep local_err but use error_propagate().
>
> Please correct me if I am wrong, as far as I have checked,
> qemu_loadvm_state() is called at 3 places
> - load_snapshot
> - qmp_xen_load_devices_state
> - process_incoming_migration_co
> and in all these functions, Error **errp is passed.
> I did not find a function that passes NULL.
> Is it still required to declare and pass a local_err object?
In that case (this should include all the callers of the three call sites
never passing in NULL) we're almost asserting errp!=NULL, then it should be
OK.
Thanks,
--
Peter Xu
Hi Peter,
Thanks for the comments.
Updated in v2.
Note: I have included a null check in the fail statement of
migration coroutine as well.
This is so that there are no dereferencing
issue in case local_err is not set in any of the paths.
(error_report_err call dereferences the err->msg)
On Wed, Jun 25, 2025 at 09:15:53AM -0400, Peter Xu wrote:
> On Wed, Jun 25, 2025 at 05:24:10PM +0530, Arun Menon wrote:
> > Hi Peter,
>
> Hi, Arun,
>
> [...]
>
> > > > static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > > > @@ -2071,6 +2072,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > > {
> > > > MigrationIncomingState *mis = migration_incoming_get_current();
> > > > QEMUFile *f = mis->from_src_file;
> > > > + Error *local_err = NULL;
> > > > int load_res;
> > > > MigrationState *migr = migrate_get_current();
> > > >
> > > > @@ -2089,7 +2091,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > > > qemu_file_set_blocking(f, true);
> > > >
> > > > /* TODO: sanity check that only postcopiable data will be loaded here */
> > > > - load_res = qemu_loadvm_state_main(f, mis);
> > > > + load_res = qemu_loadvm_state_main(f, mis, &local_err);
> > >
> > > Here we captured the error but ignored it. AFAIU it'll be the same as
> > > NULL..
> > >
> > > Not sure if you tried to trigger such vTPM migration failure with postcopy
> > > yet. AFAIU this path will be for that. To achieve your goal and make sure
> > > the error appears for postcopy too, you may want to make use of this
> > > local_err, probably by converting below (outside the diff context):
> > >
> > > qemu_file_set_error(f, load_res);
> > > ...
> > >
> > > } else {
> > > error_report("%s: loadvm failed: %d", __func__, load_res);
> > > migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > > MIGRATION_STATUS_FAILED);
> > > }
> > >
> > > Into:
> > >
> > > error_prepend(...);
> > > migrate_set_error(s, local_err);
> > >
> > > Some test will be needed to make sure it works.
> > >
> > > Side note: we really should have some migration failure tests on mismatched
> > > devices or device states / configs. We have a bunch of tests under
> > > migration-test.c. Feel free to have a look if you like to add precopy /
> > > postcopy unit tests for such case. It's also ok to leave that for later -
> > > I don't want to keep piling up work for you, and I already appreciate your
> > > help. :)
> > >
> > Yes, the postcopy errors also need to be propagated.
> > I still need to figure out to build a test case around post-copy.
> > Maybe we can do that in another commit/ticket.
>
> Yes, for now it'll be good enough if the fix can cover postcopy for the
> same issue. Manual test should be OK to switch postcopy rightaway in the
> wrong vTPM setup, and one should observe failure already, checking the
> query-migrate on dest on errors.
I need to package this version of qemu and try it on beaker machine,
in order to have 2 bare metal instances for migration.
I tried locally, running 2 QEMU instances, but it fails because of
a file descriptor error, probably because I am using the same
qcow2 image to spin the vm.
If there is any better way to do it, please let me know.
Up until now I was saving the state in to a file and restoring from it.
I suppose postcopy requires the migration to be "live", and 2 running
qemu instances are necessary.
>
> It's possible that postcopy may still have an issue that might hang src
> QEMU if it fails exactly at device transfer stage; I may need to have a
> closer look later and see whether it's true and if it's fixable - I think
> it is. The ideal way is if device state migration failed when
> postcopy_start(), then we should fail the migration and continue on src
> QEMU (like a precopy failure, as postcopy hasn't yet really started).
Yes the source QEMU hangs while trying with 2 qemu instances on the same machine,
attached to 2 separate vTPM devices (encrypted with differen secrets).
I suppose exit-on-error does not apply to the postcopy-ram capability because
the destination QEMU exited.
>
> For this specific issue, as long as the error will pop up correctly on dest
> after postcopy failed, it should be good.
>
> The auto test case can be for later.
>
> [...]
>
> > > > @@ -3094,27 +3099,24 @@ out:
> > > > return ret;
> > > > }
> > > >
> > > > -int qemu_loadvm_state(QEMUFile *f)
> > > > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> > > > {
> > > > MigrationState *s = migrate_get_current();
> > > > MigrationIncomingState *mis = migration_incoming_get_current();
> > > > - Error *local_err = NULL;
> > > > int ret;
> > > >
> > > > - if (qemu_savevm_state_blocked(&local_err)) {
> > > > - error_report_err(local_err);
> > > > + if (qemu_savevm_state_blocked(errp)) {
> > >
> > > Another thing to be careful here: I didn't check whether errp can be NULL
> > > here, likely it can.
> > >
> > > In that case we'd better keep local_err, because error_setg() (inside of
> > > qemu_savevm_state_blocked) will ignore the error otherwise..
> > >
> > > static void error_setv(Error **errp,
> > > const char *src, int line, const char *func,
> > > ErrorClass err_class, const char *fmt, va_list ap,
> > > const char *suffix)
> > > {
> > > Error *err;
> > > int saved_errno = errno;
> > >
> > > if (errp == NULL) {
> > > return;
> > > }
> > > assert(*errp == NULL);
> > > ...
> > > }
> > >
> > > So here we can keep local_err but use error_propagate().
> >
> > Please correct me if I am wrong, as far as I have checked,
> > qemu_loadvm_state() is called at 3 places
> > - load_snapshot
> > - qmp_xen_load_devices_state
> > - process_incoming_migration_co
> > and in all these functions, Error **errp is passed.
> > I did not find a function that passes NULL.
> > Is it still required to declare and pass a local_err object?
>
> In that case (this should include all the callers of the three call sites
> never passing in NULL) we're almost asserting errp!=NULL, then it should be
> OK.
>
> Thanks,
>
> --
> Peter Xu
>
Thanks,
Arun
© 2016 - 2025 Red Hat, Inc.