* [PATCH 0/3] migration: propagate vTPM errors using Error objects
@ 2025-06-24 12:23 Arun Menon
2025-06-24 12:23 ` [PATCH 1/3] migration: Pass error object to report it to the caller Arun Menon
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Arun Menon @ 2025-06-24 12:23 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic,
Eric Farman, Richard Henderson, David Hildenbrand,
Ilya Leoshkevich, Thomas Huth, Christian Borntraeger,
Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora,
Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater,
Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare,
qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé,
Arun Menon
Currently, when a migration of a VM with an encrypted vTPM
fails on the destination host (e.g., due to a mismatch in secret values),
the error message displayed on the source host is generic and unhelpful.
For example, a typical error looks like this:
"operation failed: job 'migration out' failed: Sibling indicated error 1.
operation failed: job 'migration in' failed: load of migration failed:
Input/output error"
This message does not provide any specific indication of a vTPM failure.
Such generic errors are logged using error_report(), which prints to
the console/monitor but does not make the detailed error accessible via
the QMP query-migrate command.
This series addresses the issue, by ensuring that specific TPM error
messages are propagated via the QEMU Error object.
To make this possible,
- A set of functions in the call stack is changed
to incorporate an Error object as an additional parameter.
- Also, the TPM backend makes use of a new hook called post_load_with_error()
that explicitly passes an Error object.
While this series focuses specifically on TPM error reporting during
live migration, it lays the groundwork for broader improvements.
Most methods in savevm.c that previously returned an integer now capture
errors in the Error object, enabling other modules to adopt the
post_load_with_error hook in the future.
One such change previously attempted:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01727.html
The series does not necessarily have to be applied in 1 go. Each patch
can be compiled and tested separately.
Resolves: https://issues.redhat.com/browse/RHEL-82826
Signed-off-by: Arun Menon <armenon@redhat.com>
---
Arun Menon (3):
migration: Pass error object to report it to the caller
migration: Use error_setg instead of error_report
backends/tpm: Propagate vTPM error on migration failure
backends/tpm/tpm_emulator.c | 39 ++++++++--------
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 | 3 +-
migration/colo.c | 4 +-
migration/cpr.c | 2 +-
migration/migration.c | 10 +++-
migration/savevm.c | 108 +++++++++++++++++++++++++-------------------
migration/savevm.h | 7 +--
migration/vmstate-types.c | 10 ++--
migration/vmstate.c | 40 ++++++++--------
tests/unit/test-vmstate.c | 18 ++++----
18 files changed, 144 insertions(+), 115 deletions(-)
---
base-commit: 43ba160cb4bbb193560eb0d2d7decc4b5fc599fe
change-id: 20250624-propagate_tpm_error-bf4ae6c23d30
Best regards,
--
Arun Menon <armenon@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] migration: Pass error object to report it to the caller 2025-06-24 12:23 [PATCH 0/3] migration: propagate vTPM errors using Error objects Arun Menon @ 2025-06-24 12:23 ` Arun Menon 2025-06-24 13:46 ` Peter Xu 2025-06-24 12:23 ` [PATCH 2/3] migration: Use error_setg instead of error_report Arun Menon 2025-06-24 12:23 ` [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure Arun Menon 2 siblings, 1 reply; 12+ messages in thread From: Arun Menon @ 2025-06-24 12:23 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé, Arun Menon - 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.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] migration: Pass error object to report it to the caller 2025-06-24 12:23 ` [PATCH 1/3] migration: Pass error object to report it to the caller Arun Menon @ 2025-06-24 13:46 ` Peter Xu 2025-06-25 11:54 ` Arun Menon 0 siblings, 1 reply; 12+ messages in thread From: Peter Xu @ 2025-06-24 13:46 UTC (permalink / raw) To: Arun Menon Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé 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. > @@ -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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] migration: Pass error object to report it to the caller 2025-06-24 13:46 ` Peter Xu @ 2025-06-25 11:54 ` Arun Menon 2025-06-25 13:15 ` Peter Xu 0 siblings, 1 reply; 12+ messages in thread From: Arun Menon @ 2025-06-25 11:54 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] migration: Pass error object to report it to the caller 2025-06-25 11:54 ` Arun Menon @ 2025-06-25 13:15 ` Peter Xu 2025-06-27 13:01 ` Arun Menon 0 siblings, 1 reply; 12+ messages in thread From: Peter Xu @ 2025-06-25 13:15 UTC (permalink / raw) To: Arun Menon Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] migration: Pass error object to report it to the caller 2025-06-25 13:15 ` Peter Xu @ 2025-06-27 13:01 ` Arun Menon 0 siblings, 0 replies; 12+ messages in thread From: Arun Menon @ 2025-06-27 13:01 UTC (permalink / raw) To: Peter Xu Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] migration: Use error_setg instead of error_report 2025-06-24 12:23 [PATCH 0/3] migration: propagate vTPM errors using Error objects Arun Menon 2025-06-24 12:23 ` [PATCH 1/3] migration: Pass error object to report it to the caller Arun Menon @ 2025-06-24 12:23 ` Arun Menon 2025-06-24 13:51 ` Peter Xu 2025-06-24 12:23 ` [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure Arun Menon 2 siblings, 1 reply; 12+ messages in thread From: Arun Menon @ 2025-06-24 12:23 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé, Arun Menon - This is an incremental step in converting vmstate loading code to report error via Error object. - error_report() has been replaced with error_setg(); and in places where error has been already set, error_prepend() is used to not lose information. Signed-off-by: Arun Menon <armenon@redhat.com> --- migration/migration.c | 8 +++++++- migration/savevm.c | 56 ++++++++++++++++++++++++++++++++------------------- migration/vmstate.c | 20 +++++++++--------- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index 5cabb4e7307323159241ff35781db7f1c665a75b..86e16a284286928aedd47e65e7756d27e82e811c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -903,7 +903,13 @@ process_incoming_migration_co(void *opaque) } if (ret < 0) { - error_setg(&local_err, "load of migration failed: %s", strerror(-ret)); + if (local_err) { + error_prepend(&local_err, "load of migration failed: %s: ", + strerror(-ret)); + } else { + error_setg(&local_err, "load of migration failed: %s", + strerror(-ret)); + } goto fail; } diff --git a/migration/savevm.c b/migration/savevm.c index 9bcc0935781b73e209dc57945f9dbb381283cad5..dd7b722f51143eacdc621c343a1334e6056bb268 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2689,8 +2689,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) /* Read section start */ section_id = qemu_get_be32(f); if (!qemu_get_counted_string(f, idstr)) { - error_report("Unable to read ID string for section %u", - section_id); + error_setg(errp, "Unable to read ID string for section %u", + section_id); return -EINVAL; } instance_id = qemu_get_be32(f); @@ -2698,8 +2698,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) ret = qemu_file_get_error(f); if (ret) { - error_report("%s: Failed to read instance/version ID: %d", - __func__, ret); + error_setg(errp, "%s: Failed to read instance/version ID: %d", + __func__, ret); return ret; } @@ -2708,17 +2708,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) /* Find savevm section */ se = find_se(idstr, instance_id); if (se == NULL) { - error_report("Unknown savevm section or instance '%s' %"PRIu32". " - "Make sure that your current VM setup matches your " - "saved VM setup, including any hotplugged devices", - idstr, instance_id); + error_setg(errp, "Unknown savevm section or instance '%s' %"PRIu32". " + "Make sure that your current VM setup matches your " + "saved VM setup, including any hotplugged devices", + idstr, instance_id); return -EINVAL; } /* Validate version */ if (version_id > se->version_id) { - error_report("savevm: unsupported version %d for '%s' v%d", - version_id, idstr, se->version_id); + error_setg(errp, "savevm: unsupported version %d for '%s' v%d", + version_id, idstr, se->version_id); return -EINVAL; } se->load_version_id = version_id; @@ -2726,7 +2726,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) /* Validate if it is a device's state */ if (xen_enabled() && se->is_ram) { - error_report("loadvm: %s RAM loading not allowed on Xen", idstr); + error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr); return -EINVAL; } @@ -2767,8 +2767,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp) ret = qemu_file_get_error(f); if (ret) { - error_report("%s: Failed to read section ID: %d", - __func__, ret); + error_setg(errp, "%s: Failed to read section ID: %d", + __func__, ret); return ret; } @@ -2779,7 +2779,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp) } } if (se == NULL) { - error_report("Unknown savevm section %d", section_id); + error_setg(errp, "Unknown savevm section %d", section_id); return -EINVAL; } @@ -2814,23 +2814,27 @@ static int qemu_loadvm_state_header(QEMUFile *f, Error **errp) v = qemu_get_be32(f); if (v != QEMU_VM_FILE_MAGIC) { - error_report("Not a migration stream"); + error_setg(errp, "Not a migration stream magic %x != %x", + v, QEMU_VM_FILE_MAGIC); return -EINVAL; } v = qemu_get_be32(f); if (v == QEMU_VM_FILE_VERSION_COMPAT) { - error_report("SaveVM v2 format is obsolete and don't work anymore"); + error_setg(errp, "SaveVM v2 format is obsolete and don't work anymore"); return -ENOTSUP; } if (v != QEMU_VM_FILE_VERSION) { - error_report("Unsupported migration stream version"); + error_setg(errp, "Unsupported migration stream version %x != %x", + v, QEMU_VM_FILE_VERSION); return -ENOTSUP; } if (migrate_get_current()->send_configuration) { - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { - error_report("Configuration section missing"); + v = qemu_get_byte(f); + if (v != QEMU_VM_CONFIGURATION) { + error_setg(errp, "Configuration section missing, %x != %x", + v, QEMU_VM_CONFIGURATION); return -EINVAL; } ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0, @@ -3434,7 +3438,12 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) ret = qemu_loadvm_state(f, errp); qemu_fclose(f); if (ret < 0) { - error_setg(errp, "loading Xen device state failed"); + if (*errp) { + ERRP_GUARD(); + error_prepend(errp, "loading Xen device state failed: "); + } else { + error_setg(errp, "loading Xen device state failed"); + } } migration_incoming_state_destroy(); } @@ -3511,7 +3520,12 @@ bool load_snapshot(const char *name, const char *vmstate, bdrv_drain_all_end(); if (ret < 0) { - error_setg(errp, "Error %d while loading VM state", ret); + if (*errp) { + ERRP_GUARD(); + error_prepend(errp, "Error %d while loading VM state: ", ret); + } else { + error_setg(errp, "Error %d while loading VM state", ret); + } return false; } diff --git a/migration/vmstate.c b/migration/vmstate.c index 177c563ff103ada2e494c14173fa773d52adb800..3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -139,16 +139,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, trace_vmstate_load_state(vmsd->name, version_id); if (version_id > vmsd->version_id) { - error_report("%s: incoming version_id %d is too new " - "for local version_id %d", - vmsd->name, version_id, vmsd->version_id); + error_setg(errp, "%s: incoming version_id %d is too new " + "for local version_id %d", + vmsd->name, version_id, vmsd->version_id); trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); return -EINVAL; } if (version_id < vmsd->minimum_version_id) { - error_report("%s: incoming version_id %d is too old " - "for local minimum version_id %d", - vmsd->name, version_id, vmsd->minimum_version_id); + error_setg(errp, "%s: incoming version_id %d is too old " + "for local minimum version_id %d", + vmsd->name, version_id, vmsd->minimum_version_id); trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); return -EINVAL; } @@ -213,15 +213,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, } if (ret < 0) { qemu_file_set_error(f, ret); - error_report("Failed to load %s:%s", vmsd->name, - field->name); + error_setg(errp, "Failed to load %s:%s", vmsd->name, + field->name); trace_vmstate_load_field_error(field->name, ret); return ret; } } } else if (field->flags & VMS_MUST_EXIST) { - error_report("Input validation failed: %s/%s", - vmsd->name, field->name); + error_setg(errp, "Input validation failed: %s/%s", + vmsd->name, field->name); return -1; } field++; -- 2.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] migration: Use error_setg instead of error_report 2025-06-24 12:23 ` [PATCH 2/3] migration: Use error_setg instead of error_report Arun Menon @ 2025-06-24 13:51 ` Peter Xu 0 siblings, 0 replies; 12+ messages in thread From: Peter Xu @ 2025-06-24 13:51 UTC (permalink / raw) To: Arun Menon Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé On Tue, Jun 24, 2025 at 05:53:05PM +0530, Arun Menon wrote: > - This is an incremental step in converting vmstate > loading code to report error via Error object. > - error_report() has been replaced with error_setg(); > and in places where error has been already set, > error_prepend() is used to not lose information. > Please feel free to squash this into the previous one. If you want to split the patches into smaller ones (which is optional, but will be better indeed), you can do it by function, rather than by (1) add errp, then (2) replace error_report() -> error_setg(). > Signed-off-by: Arun Menon <armenon@redhat.com> > --- > migration/migration.c | 8 +++++++- > migration/savevm.c | 56 ++++++++++++++++++++++++++++++++------------------- > migration/vmstate.c | 20 +++++++++--------- > 3 files changed, 52 insertions(+), 32 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 5cabb4e7307323159241ff35781db7f1c665a75b..86e16a284286928aedd47e65e7756d27e82e811c 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -903,7 +903,13 @@ process_incoming_migration_co(void *opaque) > } > > if (ret < 0) { > - error_setg(&local_err, "load of migration failed: %s", strerror(-ret)); > + if (local_err) { > + error_prepend(&local_err, "load of migration failed: %s: ", > + strerror(-ret)); > + } else { > + error_setg(&local_err, "load of migration failed: %s", > + strerror(-ret)); We shouldn't need this line, by making sure local_err must be set if ret<0 in the function invoked. > + } > goto fail; > } > > diff --git a/migration/savevm.c b/migration/savevm.c > index 9bcc0935781b73e209dc57945f9dbb381283cad5..dd7b722f51143eacdc621c343a1334e6056bb268 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2689,8 +2689,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) > /* Read section start */ > section_id = qemu_get_be32(f); > if (!qemu_get_counted_string(f, idstr)) { > - error_report("Unable to read ID string for section %u", > - section_id); > + error_setg(errp, "Unable to read ID string for section %u", > + section_id); > return -EINVAL; > } > instance_id = qemu_get_be32(f); > @@ -2698,8 +2698,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) > > ret = qemu_file_get_error(f); > if (ret) { > - error_report("%s: Failed to read instance/version ID: %d", > - __func__, ret); > + error_setg(errp, "%s: Failed to read instance/version ID: %d", > + __func__, ret); > return ret; > } > > @@ -2708,17 +2708,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) > /* Find savevm section */ > se = find_se(idstr, instance_id); > if (se == NULL) { > - error_report("Unknown savevm section or instance '%s' %"PRIu32". " > - "Make sure that your current VM setup matches your " > - "saved VM setup, including any hotplugged devices", > - idstr, instance_id); > + error_setg(errp, "Unknown savevm section or instance '%s' %"PRIu32". " > + "Make sure that your current VM setup matches your " > + "saved VM setup, including any hotplugged devices", > + idstr, instance_id); > return -EINVAL; > } > > /* Validate version */ > if (version_id > se->version_id) { > - error_report("savevm: unsupported version %d for '%s' v%d", > - version_id, idstr, se->version_id); > + error_setg(errp, "savevm: unsupported version %d for '%s' v%d", > + version_id, idstr, se->version_id); > return -EINVAL; > } > se->load_version_id = version_id; > @@ -2726,7 +2726,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp) > > /* Validate if it is a device's state */ > if (xen_enabled() && se->is_ram) { > - error_report("loadvm: %s RAM loading not allowed on Xen", idstr); > + error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr); > return -EINVAL; > } > > @@ -2767,8 +2767,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp) > > ret = qemu_file_get_error(f); > if (ret) { > - error_report("%s: Failed to read section ID: %d", > - __func__, ret); > + error_setg(errp, "%s: Failed to read section ID: %d", > + __func__, ret); > return ret; > } > > @@ -2779,7 +2779,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp) > } > } > if (se == NULL) { > - error_report("Unknown savevm section %d", section_id); > + error_setg(errp, "Unknown savevm section %d", section_id); > return -EINVAL; > } > > @@ -2814,23 +2814,27 @@ static int qemu_loadvm_state_header(QEMUFile *f, Error **errp) > > v = qemu_get_be32(f); > if (v != QEMU_VM_FILE_MAGIC) { > - error_report("Not a migration stream"); > + error_setg(errp, "Not a migration stream magic %x != %x", > + v, QEMU_VM_FILE_MAGIC); > return -EINVAL; > } > > v = qemu_get_be32(f); > if (v == QEMU_VM_FILE_VERSION_COMPAT) { > - error_report("SaveVM v2 format is obsolete and don't work anymore"); > + error_setg(errp, "SaveVM v2 format is obsolete and don't work anymore"); > return -ENOTSUP; > } > if (v != QEMU_VM_FILE_VERSION) { > - error_report("Unsupported migration stream version"); > + error_setg(errp, "Unsupported migration stream version %x != %x", > + v, QEMU_VM_FILE_VERSION); > return -ENOTSUP; > } > > if (migrate_get_current()->send_configuration) { > - if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > - error_report("Configuration section missing"); > + v = qemu_get_byte(f); > + if (v != QEMU_VM_CONFIGURATION) { > + error_setg(errp, "Configuration section missing, %x != %x", > + v, QEMU_VM_CONFIGURATION); > return -EINVAL; > } > ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0, > @@ -3434,7 +3438,12 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) > ret = qemu_loadvm_state(f, errp); > qemu_fclose(f); > if (ret < 0) { > - error_setg(errp, "loading Xen device state failed"); > + if (*errp) { > + ERRP_GUARD(); This is either not needed here, or needed at the top of function. > + error_prepend(errp, "loading Xen device state failed: "); > + } else { > + error_setg(errp, "loading Xen device state failed"); Similar here, can drop it by making sure *errp!=NULL for ret<0. > + } > } > migration_incoming_state_destroy(); > } > @@ -3511,7 +3520,12 @@ bool load_snapshot(const char *name, const char *vmstate, > bdrv_drain_all_end(); > > if (ret < 0) { > - error_setg(errp, "Error %d while loading VM state", ret); > + if (*errp) { > + ERRP_GUARD(); Same. > + error_prepend(errp, "Error %d while loading VM state: ", ret); > + } else { > + error_setg(errp, "Error %d while loading VM state", ret); Same. > + } > return false; > } > > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 177c563ff103ada2e494c14173fa773d52adb800..3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -139,16 +139,16 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > > trace_vmstate_load_state(vmsd->name, version_id); > if (version_id > vmsd->version_id) { > - error_report("%s: incoming version_id %d is too new " > - "for local version_id %d", > - vmsd->name, version_id, vmsd->version_id); > + error_setg(errp, "%s: incoming version_id %d is too new " > + "for local version_id %d", > + vmsd->name, version_id, vmsd->version_id); > trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL); > return -EINVAL; > } > if (version_id < vmsd->minimum_version_id) { > - error_report("%s: incoming version_id %d is too old " > - "for local minimum version_id %d", > - vmsd->name, version_id, vmsd->minimum_version_id); > + error_setg(errp, "%s: incoming version_id %d is too old " > + "for local minimum version_id %d", > + vmsd->name, version_id, vmsd->minimum_version_id); > trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); > return -EINVAL; > } > @@ -213,15 +213,15 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > } > if (ret < 0) { > qemu_file_set_error(f, ret); > - error_report("Failed to load %s:%s", vmsd->name, > - field->name); > + error_setg(errp, "Failed to load %s:%s", vmsd->name, > + field->name); > trace_vmstate_load_field_error(field->name, ret); > return ret; > } > } > } else if (field->flags & VMS_MUST_EXIST) { > - error_report("Input validation failed: %s/%s", > - vmsd->name, field->name); > + error_setg(errp, "Input validation failed: %s/%s", > + vmsd->name, field->name); > return -1; > } > field++; > > -- > 2.49.0 > -- Peter Xu ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure 2025-06-24 12:23 [PATCH 0/3] migration: propagate vTPM errors using Error objects Arun Menon 2025-06-24 12:23 ` [PATCH 1/3] migration: Pass error object to report it to the caller Arun Menon 2025-06-24 12:23 ` [PATCH 2/3] migration: Use error_setg instead of error_report Arun Menon @ 2025-06-24 12:23 ` Arun Menon 2025-06-24 17:30 ` Stefan Berger 2025-07-01 12:21 ` Marc-André Lureau 2 siblings, 2 replies; 12+ messages in thread From: Arun Menon @ 2025-06-24 12:23 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé, Arun Menon - Introduce a new post_load_with_error() hook that will take in the Error object as a parameter. - This error object is set if the loading of state fails. - The error can then be retrieved using QMP command {"execute" : "query-migrate"} Buglink: https://issues.redhat.com/browse/RHEL-82826 Signed-off-by: Arun Menon <armenon@redhat.com> --- backends/tpm/tpm_emulator.c | 39 ++++++++++++++++++++------------------- include/migration/vmstate.h | 1 + migration/vmstate.c | 4 +++- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c index 4a234ab2c0b19b2604bf0dd8cb5f4540c72a9438..816134d7b4de00a75a3d0b928d160595b17be810 100644 --- a/backends/tpm/tpm_emulator.c +++ b/backends/tpm/tpm_emulator.c @@ -819,7 +819,8 @@ static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu) static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, uint32_t type, TPMSizedBuffer *tsb, - uint32_t flags) + uint32_t flags, + Error **errp) { ssize_t n; ptm_setstate pss; @@ -838,17 +839,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, /* write the header only */ if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss, offsetof(ptm_setstate, u.req.data), 0, 0) < 0) { - error_report("tpm-emulator: could not set state blob type %d : %s", - type, strerror(errno)); + error_setg(errp, "tpm-emulator: could not set state blob type %d : %s", + type, strerror(errno)); return -1; } /* now the body */ n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr, tsb->buffer, tsb->size); if (n != tsb->size) { - error_report("tpm-emulator: Writing the stateblob (type %d) " - "failed; could not write %u bytes, but only %zd", - type, tsb->size, n); + error_setg(errp, "tpm-emulator: Writing the stateblob (type %d) " + "failed; could not write %u bytes, but only %zd", + type, tsb->size, n); return -1; } @@ -856,17 +857,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, (uint8_t *)&pss, sizeof(pss.u.resp)); if (n != sizeof(pss.u.resp)) { - error_report("tpm-emulator: Reading response from writing stateblob " - "(type %d) failed; expected %zu bytes, got %zd", type, - sizeof(pss.u.resp), n); + error_setg(errp, "tpm-emulator: Reading response from writing " + "stateblob (type %d) failed; expected %zu bytes, " + "got %zd", type, sizeof(pss.u.resp), n); return -1; } tpm_result = be32_to_cpu(pss.u.resp.tpm_result); if (tpm_result != 0) { - error_report("tpm-emulator: Setting the stateblob (type %d) failed " - "with a TPM error 0x%x %s", type, tpm_result, - tpm_emulator_strerror(tpm_result)); + error_setg(errp, "tpm-emulator: Setting the stateblob (type %d) " + "failed with a TPM error 0x%x %s", type, tpm_result, + tpm_emulator_strerror(tpm_result)); return -1; } @@ -880,7 +881,7 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, * * Returns a negative errno code in case of error. */ -static int tpm_emulator_set_state_blobs(TPMBackend *tb) +static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp) { TPMEmulator *tpm_emu = TPM_EMULATOR(tb); TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs; @@ -894,13 +895,13 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb) if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT, &state_blobs->permanent, - state_blobs->permanent_flags) < 0 || + state_blobs->permanent_flags, errp) < 0 || tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE, &state_blobs->volatil, - state_blobs->volatil_flags) < 0 || + state_blobs->volatil_flags, errp) < 0 || tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE, &state_blobs->savestate, - state_blobs->savestate_flags) < 0) { + state_blobs->savestate_flags, errp) < 0) { return -EIO; } @@ -948,12 +949,12 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running, * * Returns negative errno codes in case of error. */ -static int tpm_emulator_post_load(void *opaque, int version_id) +static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp) { TPMBackend *tb = opaque; int ret; - ret = tpm_emulator_set_state_blobs(tb); + ret = tpm_emulator_set_state_blobs(tb, errp); if (ret < 0) { return ret; } @@ -969,7 +970,7 @@ static const VMStateDescription vmstate_tpm_emulator = { .name = "tpm-emulator", .version_id = 0, .pre_save = tpm_emulator_pre_save, - .post_load = tpm_emulator_post_load, + .post_load_with_error = tpm_emulator_post_load, .fields = (const VMStateField[]) { VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator), VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator), diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 056781b1c21e737583f081594d9f88b32adfd674..1c6e89c3b08a3914cde6dce3be5955978b6b7d0b 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -207,6 +207,7 @@ struct VMStateDescription { MigrationPriority priority; int (*pre_load)(void *opaque); int (*post_load)(void *opaque, int version_id); + int (*post_load_with_error)(void *opaque, int version_id, Error **errp); int (*pre_save)(void *opaque); int (*post_save)(void *opaque); bool (*needed)(void *opaque); diff --git a/migration/vmstate.c b/migration/vmstate.c index 3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385..c5dfffd9bad7285e819d4769e055d47157caab34 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -232,7 +232,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, qemu_file_set_error(f, ret); return ret; } - if (vmsd->post_load) { + if (vmsd->post_load_with_error) { + ret = vmsd->post_load_with_error(opaque, version_id, errp); + } else if (vmsd->post_load) { ret = vmsd->post_load(opaque, version_id); } trace_vmstate_load_state_end(vmsd->name, "end", ret); -- 2.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure 2025-06-24 12:23 ` [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure Arun Menon @ 2025-06-24 17:30 ` Stefan Berger 2025-07-01 12:21 ` Marc-André Lureau 1 sibling, 0 replies; 12+ messages in thread From: Stefan Berger @ 2025-06-24 17:30 UTC (permalink / raw) To: Arun Menon, qemu-devel Cc: Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé On 6/24/25 8:23 AM, Arun Menon wrote: > - Introduce a new post_load_with_error() hook that will > take in the Error object as a parameter. > - This error object is set if the loading of state fails. > - The error can then be retrieved using QMP command > {"execute" : "query-migrate"} > > Buglink: https://issues.redhat.com/browse/RHEL-82826 > > Signed-off-by: Arun Menon <armenon@redhat.com> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > backends/tpm/tpm_emulator.c | 39 ++++++++++++++++++++------------------- > include/migration/vmstate.h | 1 + > migration/vmstate.c | 4 +++- > 3 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c > index 4a234ab2c0b19b2604bf0dd8cb5f4540c72a9438..816134d7b4de00a75a3d0b928d160595b17be810 100644 > --- a/backends/tpm/tpm_emulator.c > +++ b/backends/tpm/tpm_emulator.c > @@ -819,7 +819,8 @@ static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu) > static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > uint32_t type, > TPMSizedBuffer *tsb, > - uint32_t flags) > + uint32_t flags, > + Error **errp) > { > ssize_t n; > ptm_setstate pss; > @@ -838,17 +839,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > /* write the header only */ > if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss, > offsetof(ptm_setstate, u.req.data), 0, 0) < 0) { > - error_report("tpm-emulator: could not set state blob type %d : %s", > - type, strerror(errno)); > + error_setg(errp, "tpm-emulator: could not set state blob type %d : %s", > + type, strerror(errno)); > return -1; > } > > /* now the body */ > n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr, tsb->buffer, tsb->size); > if (n != tsb->size) { > - error_report("tpm-emulator: Writing the stateblob (type %d) " > - "failed; could not write %u bytes, but only %zd", > - type, tsb->size, n); > + error_setg(errp, "tpm-emulator: Writing the stateblob (type %d) " > + "failed; could not write %u bytes, but only %zd", > + type, tsb->size, n); > return -1; > } > > @@ -856,17 +857,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, > (uint8_t *)&pss, sizeof(pss.u.resp)); > if (n != sizeof(pss.u.resp)) { > - error_report("tpm-emulator: Reading response from writing stateblob " > - "(type %d) failed; expected %zu bytes, got %zd", type, > - sizeof(pss.u.resp), n); > + error_setg(errp, "tpm-emulator: Reading response from writing " > + "stateblob (type %d) failed; expected %zu bytes, " > + "got %zd", type, sizeof(pss.u.resp), n); > return -1; > } > > tpm_result = be32_to_cpu(pss.u.resp.tpm_result); > if (tpm_result != 0) { > - error_report("tpm-emulator: Setting the stateblob (type %d) failed " > - "with a TPM error 0x%x %s", type, tpm_result, > - tpm_emulator_strerror(tpm_result)); > + error_setg(errp, "tpm-emulator: Setting the stateblob (type %d) " > + "failed with a TPM error 0x%x %s", type, tpm_result, > + tpm_emulator_strerror(tpm_result)); > return -1; > } > > @@ -880,7 +881,7 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > * > * Returns a negative errno code in case of error. > */ > -static int tpm_emulator_set_state_blobs(TPMBackend *tb) > +static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp) > { > TPMEmulator *tpm_emu = TPM_EMULATOR(tb); > TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs; > @@ -894,13 +895,13 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb) > > if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT, > &state_blobs->permanent, > - state_blobs->permanent_flags) < 0 || > + state_blobs->permanent_flags, errp) < 0 || > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE, > &state_blobs->volatil, > - state_blobs->volatil_flags) < 0 || > + state_blobs->volatil_flags, errp) < 0 || > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE, > &state_blobs->savestate, > - state_blobs->savestate_flags) < 0) { > + state_blobs->savestate_flags, errp) < 0) { > return -EIO; > } > > @@ -948,12 +949,12 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running, > * > * Returns negative errno codes in case of error. > */ > -static int tpm_emulator_post_load(void *opaque, int version_id) > +static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp) > { > TPMBackend *tb = opaque; > int ret; > > - ret = tpm_emulator_set_state_blobs(tb); > + ret = tpm_emulator_set_state_blobs(tb, errp); > if (ret < 0) { > return ret; > } > @@ -969,7 +970,7 @@ static const VMStateDescription vmstate_tpm_emulator = { > .name = "tpm-emulator", > .version_id = 0, > .pre_save = tpm_emulator_pre_save, > - .post_load = tpm_emulator_post_load, > + .post_load_with_error = tpm_emulator_post_load, > .fields = (const VMStateField[]) { > VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator), > VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator), > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 056781b1c21e737583f081594d9f88b32adfd674..1c6e89c3b08a3914cde6dce3be5955978b6b7d0b 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -207,6 +207,7 @@ struct VMStateDescription { > MigrationPriority priority; > int (*pre_load)(void *opaque); > int (*post_load)(void *opaque, int version_id); > + int (*post_load_with_error)(void *opaque, int version_id, Error **errp); > int (*pre_save)(void *opaque); > int (*post_save)(void *opaque); > bool (*needed)(void *opaque); > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385..c5dfffd9bad7285e819d4769e055d47157caab34 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -232,7 +232,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > qemu_file_set_error(f, ret); > return ret; > } > - if (vmsd->post_load) { > + if (vmsd->post_load_with_error) { > + ret = vmsd->post_load_with_error(opaque, version_id, errp); > + } else if (vmsd->post_load) { > ret = vmsd->post_load(opaque, version_id); > } > trace_vmstate_load_state_end(vmsd->name, "end", ret); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure 2025-06-24 12:23 ` [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure Arun Menon 2025-06-24 17:30 ` Stefan Berger @ 2025-07-01 12:21 ` Marc-André Lureau 2025-07-02 11:39 ` Arun Menon 1 sibling, 1 reply; 12+ messages in thread From: Marc-André Lureau @ 2025-07-01 12:21 UTC (permalink / raw) To: Arun Menon Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé Hi On Tue, Jun 24, 2025 at 4:26 PM Arun Menon <armenon@redhat.com> wrote: > > - Introduce a new post_load_with_error() hook that will > take in the Error object as a parameter. You should make this a different patch. I wonder if adding another callback is the right approach, as only one of the two is called. I would rather change the existing callback. > - This error object is set if the loading of state fails. > - The error can then be retrieved using QMP command > {"execute" : "query-migrate"} > > Buglink: https://issues.redhat.com/browse/RHEL-82826 > > Signed-off-by: Arun Menon <armenon@redhat.com> > --- > backends/tpm/tpm_emulator.c | 39 ++++++++++++++++++++------------------- > include/migration/vmstate.h | 1 + > migration/vmstate.c | 4 +++- > 3 files changed, 24 insertions(+), 20 deletions(-) > > diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c > index 4a234ab2c0b19b2604bf0dd8cb5f4540c72a9438..816134d7b4de00a75a3d0b928d160595b17be810 100644 > --- a/backends/tpm/tpm_emulator.c > +++ b/backends/tpm/tpm_emulator.c > @@ -819,7 +819,8 @@ static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu) > static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > uint32_t type, > TPMSizedBuffer *tsb, > - uint32_t flags) > + uint32_t flags, > + Error **errp) > { > ssize_t n; > ptm_setstate pss; > @@ -838,17 +839,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > /* write the header only */ > if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss, > offsetof(ptm_setstate, u.req.data), 0, 0) < 0) { > - error_report("tpm-emulator: could not set state blob type %d : %s", > - type, strerror(errno)); > + error_setg(errp, "tpm-emulator: could not set state blob type %d : %s", > + type, strerror(errno)); > return -1; > } > > /* now the body */ > n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr, tsb->buffer, tsb->size); > if (n != tsb->size) { > - error_report("tpm-emulator: Writing the stateblob (type %d) " > - "failed; could not write %u bytes, but only %zd", > - type, tsb->size, n); > + error_setg(errp, "tpm-emulator: Writing the stateblob (type %d) " > + "failed; could not write %u bytes, but only %zd", > + type, tsb->size, n); > return -1; > } > > @@ -856,17 +857,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, > (uint8_t *)&pss, sizeof(pss.u.resp)); > if (n != sizeof(pss.u.resp)) { > - error_report("tpm-emulator: Reading response from writing stateblob " > - "(type %d) failed; expected %zu bytes, got %zd", type, > - sizeof(pss.u.resp), n); > + error_setg(errp, "tpm-emulator: Reading response from writing " > + "stateblob (type %d) failed; expected %zu bytes, " > + "got %zd", type, sizeof(pss.u.resp), n); > return -1; > } > > tpm_result = be32_to_cpu(pss.u.resp.tpm_result); > if (tpm_result != 0) { > - error_report("tpm-emulator: Setting the stateblob (type %d) failed " > - "with a TPM error 0x%x %s", type, tpm_result, > - tpm_emulator_strerror(tpm_result)); > + error_setg(errp, "tpm-emulator: Setting the stateblob (type %d) " > + "failed with a TPM error 0x%x %s", type, tpm_result, > + tpm_emulator_strerror(tpm_result)); > return -1; > } > > @@ -880,7 +881,7 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > * > * Returns a negative errno code in case of error. > */ > -static int tpm_emulator_set_state_blobs(TPMBackend *tb) > +static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp) > { > TPMEmulator *tpm_emu = TPM_EMULATOR(tb); > TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs; > @@ -894,13 +895,13 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb) > > if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT, > &state_blobs->permanent, > - state_blobs->permanent_flags) < 0 || > + state_blobs->permanent_flags, errp) < 0 || > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE, > &state_blobs->volatil, > - state_blobs->volatil_flags) < 0 || > + state_blobs->volatil_flags, errp) < 0 || > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE, > &state_blobs->savestate, > - state_blobs->savestate_flags) < 0) { > + state_blobs->savestate_flags, errp) < 0) { > return -EIO; > } > > @@ -948,12 +949,12 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running, > * > * Returns negative errno codes in case of error. > */ > -static int tpm_emulator_post_load(void *opaque, int version_id) > +static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp) > { > TPMBackend *tb = opaque; > int ret; > > - ret = tpm_emulator_set_state_blobs(tb); > + ret = tpm_emulator_set_state_blobs(tb, errp); > if (ret < 0) { > return ret; > } > @@ -969,7 +970,7 @@ static const VMStateDescription vmstate_tpm_emulator = { > .name = "tpm-emulator", > .version_id = 0, > .pre_save = tpm_emulator_pre_save, > - .post_load = tpm_emulator_post_load, > + .post_load_with_error = tpm_emulator_post_load, > .fields = (const VMStateField[]) { > VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator), > VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator), > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > index 056781b1c21e737583f081594d9f88b32adfd674..1c6e89c3b08a3914cde6dce3be5955978b6b7d0b 100644 > --- a/include/migration/vmstate.h > +++ b/include/migration/vmstate.h > @@ -207,6 +207,7 @@ struct VMStateDescription { > MigrationPriority priority; > int (*pre_load)(void *opaque); > int (*post_load)(void *opaque, int version_id); > + int (*post_load_with_error)(void *opaque, int version_id, Error **errp); > int (*pre_save)(void *opaque); > int (*post_save)(void *opaque); > bool (*needed)(void *opaque); > diff --git a/migration/vmstate.c b/migration/vmstate.c > index 3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385..c5dfffd9bad7285e819d4769e055d47157caab34 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -232,7 +232,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > qemu_file_set_error(f, ret); > return ret; > } > - if (vmsd->post_load) { > + if (vmsd->post_load_with_error) { > + ret = vmsd->post_load_with_error(opaque, version_id, errp); > + } else if (vmsd->post_load) { > ret = vmsd->post_load(opaque, version_id); > } > trace_vmstate_load_state_end(vmsd->name, "end", ret); > > -- > 2.49.0 > > -- Marc-André Lureau ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure 2025-07-01 12:21 ` Marc-André Lureau @ 2025-07-02 11:39 ` Arun Menon 0 siblings, 0 replies; 12+ messages in thread From: Arun Menon @ 2025-07-02 11:39 UTC (permalink / raw) To: Marc-André Lureau Cc: qemu-devel, Michael S. Tsirkin, Marcel Apfelbaum, Cornelia Huck, Halil Pasic, Eric Farman, Richard Henderson, David Hildenbrand, Ilya Leoshkevich, Thomas Huth, Christian Borntraeger, Nicholas Piggin, Daniel Henrique Barboza, Harsh Prateek Bora, Paolo Bonzini, Fam Zheng, Alex Williamson, Cédric Le Goater, Peter Xu, Fabiano Rosas, Hailiang Zhang, Steve Sistare, qemu-s390x, qemu-ppc, Stefan Berger, Daniel P. Berrangé Hi Marc-André, Thank you for the review. On Tue, Jul 01, 2025 at 04:21:23PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Jun 24, 2025 at 4:26 PM Arun Menon <armenon@redhat.com> wrote: > > > > - Introduce a new post_load_with_error() hook that will > > take in the Error object as a parameter. > > You should make this a different patch. Agreed. Updated in version 3. > I wonder if adding another > callback is the right approach, as only one of the two is called. I > would rather change the existing callback. This was briefly discussed in the ticket comment section. https://issues.redhat.com/browse/RHEL-82826 The idea is to gradually introduce convert the codebase to using post_load_with_error() hook. Therefore, the call is kept in mutual exclusion to post_load(). > > > - This error object is set if the loading of state fails. > > - The error can then be retrieved using QMP command > > {"execute" : "query-migrate"} > > > > Buglink: https://issues.redhat.com/browse/RHEL-82826 > > > > Signed-off-by: Arun Menon <armenon@redhat.com> > > --- > > backends/tpm/tpm_emulator.c | 39 ++++++++++++++++++++------------------- > > include/migration/vmstate.h | 1 + > > migration/vmstate.c | 4 +++- > > 3 files changed, 24 insertions(+), 20 deletions(-) > > > > diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c > > index 4a234ab2c0b19b2604bf0dd8cb5f4540c72a9438..816134d7b4de00a75a3d0b928d160595b17be810 100644 > > --- a/backends/tpm/tpm_emulator.c > > +++ b/backends/tpm/tpm_emulator.c > > @@ -819,7 +819,8 @@ static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu) > > static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > > uint32_t type, > > TPMSizedBuffer *tsb, > > - uint32_t flags) > > + uint32_t flags, > > + Error **errp) > > { > > ssize_t n; > > ptm_setstate pss; > > @@ -838,17 +839,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > > /* write the header only */ > > if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss, > > offsetof(ptm_setstate, u.req.data), 0, 0) < 0) { > > - error_report("tpm-emulator: could not set state blob type %d : %s", > > - type, strerror(errno)); > > + error_setg(errp, "tpm-emulator: could not set state blob type %d : %s", > > + type, strerror(errno)); > > return -1; > > } > > > > /* now the body */ > > n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr, tsb->buffer, tsb->size); > > if (n != tsb->size) { > > - error_report("tpm-emulator: Writing the stateblob (type %d) " > > - "failed; could not write %u bytes, but only %zd", > > - type, tsb->size, n); > > + error_setg(errp, "tpm-emulator: Writing the stateblob (type %d) " > > + "failed; could not write %u bytes, but only %zd", > > + type, tsb->size, n); > > return -1; > > } > > > > @@ -856,17 +857,17 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > > n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, > > (uint8_t *)&pss, sizeof(pss.u.resp)); > > if (n != sizeof(pss.u.resp)) { > > - error_report("tpm-emulator: Reading response from writing stateblob " > > - "(type %d) failed; expected %zu bytes, got %zd", type, > > - sizeof(pss.u.resp), n); > > + error_setg(errp, "tpm-emulator: Reading response from writing " > > + "stateblob (type %d) failed; expected %zu bytes, " > > + "got %zd", type, sizeof(pss.u.resp), n); > > return -1; > > } > > > > tpm_result = be32_to_cpu(pss.u.resp.tpm_result); > > if (tpm_result != 0) { > > - error_report("tpm-emulator: Setting the stateblob (type %d) failed " > > - "with a TPM error 0x%x %s", type, tpm_result, > > - tpm_emulator_strerror(tpm_result)); > > + error_setg(errp, "tpm-emulator: Setting the stateblob (type %d) " > > + "failed with a TPM error 0x%x %s", type, tpm_result, > > + tpm_emulator_strerror(tpm_result)); > > return -1; > > } > > > > @@ -880,7 +881,7 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu, > > * > > * Returns a negative errno code in case of error. > > */ > > -static int tpm_emulator_set_state_blobs(TPMBackend *tb) > > +static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp) > > { > > TPMEmulator *tpm_emu = TPM_EMULATOR(tb); > > TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs; > > @@ -894,13 +895,13 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb) > > > > if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT, > > &state_blobs->permanent, > > - state_blobs->permanent_flags) < 0 || > > + state_blobs->permanent_flags, errp) < 0 || > > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE, > > &state_blobs->volatil, > > - state_blobs->volatil_flags) < 0 || > > + state_blobs->volatil_flags, errp) < 0 || > > tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE, > > &state_blobs->savestate, > > - state_blobs->savestate_flags) < 0) { > > + state_blobs->savestate_flags, errp) < 0) { > > return -EIO; > > } > > > > @@ -948,12 +949,12 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running, > > * > > * Returns negative errno codes in case of error. > > */ > > -static int tpm_emulator_post_load(void *opaque, int version_id) > > +static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp) > > { > > TPMBackend *tb = opaque; > > int ret; > > > > - ret = tpm_emulator_set_state_blobs(tb); > > + ret = tpm_emulator_set_state_blobs(tb, errp); > > if (ret < 0) { > > return ret; > > } > > @@ -969,7 +970,7 @@ static const VMStateDescription vmstate_tpm_emulator = { > > .name = "tpm-emulator", > > .version_id = 0, > > .pre_save = tpm_emulator_pre_save, > > - .post_load = tpm_emulator_post_load, > > + .post_load_with_error = tpm_emulator_post_load, > > .fields = (const VMStateField[]) { > > VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator), > > VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator), > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h > > index 056781b1c21e737583f081594d9f88b32adfd674..1c6e89c3b08a3914cde6dce3be5955978b6b7d0b 100644 > > --- a/include/migration/vmstate.h > > +++ b/include/migration/vmstate.h > > @@ -207,6 +207,7 @@ struct VMStateDescription { > > MigrationPriority priority; > > int (*pre_load)(void *opaque); > > int (*post_load)(void *opaque, int version_id); > > + int (*post_load_with_error)(void *opaque, int version_id, Error **errp); > > int (*pre_save)(void *opaque); > > int (*post_save)(void *opaque); > > bool (*needed)(void *opaque); > > diff --git a/migration/vmstate.c b/migration/vmstate.c > > index 3f8c3d3c1dcfe14d70bab1f43b827244eb4bb385..c5dfffd9bad7285e819d4769e055d47157caab34 100644 > > --- a/migration/vmstate.c > > +++ b/migration/vmstate.c > > @@ -232,7 +232,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, > > qemu_file_set_error(f, ret); > > return ret; > > } > > - if (vmsd->post_load) { > > + if (vmsd->post_load_with_error) { > > + ret = vmsd->post_load_with_error(opaque, version_id, errp); > > + } else if (vmsd->post_load) { > > ret = vmsd->post_load(opaque, version_id); > > } > > trace_vmstate_load_state_end(vmsd->name, "end", ret); > > > > -- > > 2.49.0 > > > > > > > -- > Marc-André Lureau > Regards, Arun Menon ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-07-02 11:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-06-24 12:23 [PATCH 0/3] migration: propagate vTPM errors using Error objects Arun Menon 2025-06-24 12:23 ` [PATCH 1/3] migration: Pass error object to report it to the caller Arun Menon 2025-06-24 13:46 ` Peter Xu 2025-06-25 11:54 ` Arun Menon 2025-06-25 13:15 ` Peter Xu 2025-06-27 13:01 ` Arun Menon 2025-06-24 12:23 ` [PATCH 2/3] migration: Use error_setg instead of error_report Arun Menon 2025-06-24 13:51 ` Peter Xu 2025-06-24 12:23 ` [PATCH 3/3] backends/tpm: Propagate vTPM error on migration failure Arun Menon 2025-06-24 17:30 ` Stefan Berger 2025-07-01 12:21 ` Marc-André Lureau 2025-07-02 11:39 ` Arun Menon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).