First, the handlers should put all needed information into errp,
we should not append error number here.

Second, the only realization of new _errp API is
tpm_emulator_post_load(), which on some failure paths returns
-errno, but on the others simply -1. So printing this additional
number may be misleading. tpm_emulator.c needs a lot more work
to report good error message on all error paths.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/vmstate.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index fd066f910e..677e56c84a 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_load_errp(opaque, errp);
         if (ret < 0) {
             error_prepend(errp, "pre load hook failed for: '%s', "
-                          "version_id: %d, minimum version_id: %d, "
-                          "ret: %d: ", vmsd->name, vmsd->version_id,
-                          vmsd->minimum_version_id, ret);
+                          "version_id: %d, minimum version_id: %d: ",
+                          vmsd->name, vmsd->version_id,
+                          vmsd->minimum_version_id);
             return ret;
         }
     } else if (vmsd->pre_load) {
@@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->post_load_errp(opaque, version_id, errp);
         if (ret < 0) {
             error_prepend(errp, "post load hook failed for: %s, version_id: "
-                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
-                          vmsd->version_id, vmsd->minimum_version_id, ret);
+                          "%d, minimum_version: %d: ", vmsd->name,
+                          vmsd->version_id, vmsd->minimum_version_id);
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_save_errp(opaque, errp);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret < 0) {
-            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
-                          vmsd->name, ret);
+            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
             return ret;
         }
     } else if (vmsd->pre_save) {
-- 
2.48.1

What do you mean by "new _errp APIs"?  Is it the pre_load_errp(),
post_load_errp(), pre_save_errp() callbacks?

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> First, the handlers should put all needed information into errp,
> we should not append error number here.
>
> Second, the only realization of new _errp API is
> tpm_emulator_post_load(), which on some failure paths returns
> -errno, but on the others simply -1. So printing this additional
> number may be misleading. tpm_emulator.c needs a lot more work
> to report good error message on all error paths.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/vmstate.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fd066f910e..677e56c84a 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->pre_load_errp(opaque, errp);
>          if (ret < 0) {
>              error_prepend(errp, "pre load hook failed for: '%s', "
> -                          "version_id: %d, minimum version_id: %d, "
> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
> -                          vmsd->minimum_version_id, ret);
> +                          "version_id: %d, minimum version_id: %d: ",
> +                          vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id);
>              return ret;
>          }
>      } else if (vmsd->pre_load) {
> @@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->post_load_errp(opaque, version_id, errp);
>          if (ret < 0) {
>              error_prepend(errp, "post load hook failed for: %s, version_id: "
> -                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
> -                          vmsd->version_id, vmsd->minimum_version_id, ret);
> +                          "%d, minimum_version: %d: ", vmsd->name,
> +                          vmsd->version_id, vmsd->minimum_version_id);
>          }
>      } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
> @@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->pre_save_errp(opaque, errp);
>          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>          if (ret < 0) {
> -            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
> -                          vmsd->name, ret);
> +            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>              return ret;
>          }
>      } else if (vmsd->pre_save) {

All good.  However, there are more error messages with numeric error
codes in this file.  I figure you're leaving them for another day.
That's okay, but I'd suggest to mention this in your commit message.