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 <arme...@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
