Re: [PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 13:23, Markus Armbruster wrote:

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

Yes

Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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.

Yes.. Will do, if we continue going this way.

--
Best regards,
Vladimir