[PATCH 2/3] migration: Use error_setg instead of error_report

Arun Menon posted 3 patches 4 months, 3 weeks ago
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Hailiang Zhang <zhanghailiang@xfusion.com>, Steve Sistare <steven.sistare@oracle.com>
There is a newer version of this series
[PATCH 2/3] migration: Use error_setg instead of error_report
Posted by Arun Menon 4 months, 3 weeks ago
- 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
Re: [PATCH 2/3] migration: Use error_setg instead of error_report
Posted by Peter Xu 4 months, 3 weeks ago
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