Re: [PATCH v3 4/4] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 13:38, Markus Armbruster wrote:

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

Switch the new API to simple bool-returning interface, as return value
is not used otherwise than check is function failed or not. No logic
depend on concrete errno values.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
---
  backends/tpm/tpm_emulator.c   | 10 ++++------
  docs/devel/migration/main.rst |  6 +++---
  include/migration/vmstate.h   |  6 +++---
  migration/vmstate.c           | 14 ++++++--------
  4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index aa69eb606f..6cc9aa199c 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, 
bool running,

/*

   * Load the TPM state blobs into the TPM.
- *
- * Returns negative errno codes in case of error.
   */
-static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
+static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
  {
      TPMBackend *tb = opaque;
      int ret;

ret = tpm_emulator_set_state_blobs(tb, errp);

Note for later: this returns 0 or -EIO.

      if (ret < 0) {
-        return ret;
+        return false;
      }

if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {

          error_setg(errp, "Failed to resume tpm");
-        return -EIO;
+        return false;
      }

- return 0;

+    return true;
  }

static const VMStateDescription vmstate_tpm_emulator = {

diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 1afe7b9689..234d280249 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, 
and are called:

Following are the errp variants of these functions. -- ``int (*pre_load_errp)(void *opaque, Error **errp);``

+- ``bool (*pre_load_errp)(void *opaque, Error **errp);``

This function is called before we load the state of one device. -- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``

+- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``

This function is called after we load the state of one device. -- ``int (*pre_save_errp)(void *opaque, Error **errp);``

+- ``bool (*pre_save_errp)(void *opaque, Error **errp);``

This function is called before we save the state of one device. diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h

index 63ccaee07a..dbe330dd5f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -218,11 +218,11 @@ struct VMStateDescription {
      int minimum_version_id;
      MigrationPriority priority;
      int (*pre_load)(void *opaque);
-    int (*pre_load_errp)(void *opaque, Error **errp);
+    bool (*pre_load_errp)(void *opaque, Error **errp);
      int (*post_load)(void *opaque, int version_id);
-    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
+    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
      int (*pre_save)(void *opaque);
-    int (*pre_save_errp)(void *opaque, Error **errp);
+    bool (*pre_save_errp)(void *opaque, Error **errp);
      int (*post_save)(void *opaque);
      bool (*needed)(void *opaque);
      bool (*dev_unplug_pending)(void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 677e56c84a..adaaf91b3f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
          return -EINVAL;
      }
      if (vmsd->pre_load_errp) {
-        ret = vmsd->pre_load_errp(opaque, errp);
-        if (ret < 0) {
+        if (!vmsd->pre_load_errp(opaque, errp)) {
              error_prepend(errp, "pre load hook failed for: '%s', "
                            "version_id: %d, minimum version_id: %d: ",
                            vmsd->name, vmsd->version_id,
                            vmsd->minimum_version_id);
-            return ret;
+            return -EINVAL;
          }
      } else if (vmsd->pre_load) {
          ret = vmsd->pre_load(opaque);
@@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
          return ret;
      }
      if (vmsd->post_load_errp) {
-        ret = vmsd->post_load_errp(opaque, version_id, errp);
-        if (ret < 0) {
+        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
              error_prepend(errp, "post load hook failed for: %s, version_id: "
                            "%d, minimum_version: %d: ", vmsd->name,
                            vmsd->version_id, vmsd->minimum_version_id);
+            ret = -EINVAL;

With ->post_load_errp is tpm_emulator_post_load(), the value returned on
error changes from -EIO to -EINVAL.

Do callers of vmstate_load_state() care?

Fast check.. let see somewhere, OK: get_qlist() in vmstate-types.c.. That's a 
.get
in VMStateInfo structure.

Oh, and we do print this ret the same way:


int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                       void *opaque, int version_id, Error **errp)

 ...

                    ret = inner_field->info->get(f, curr_elem, size,
                                                 inner_field);
                    if (ret < 0) {
                        error_setg(errp,
                                   "Failed to load element of type %s for %s: "
                                   "%d", inner_field->info->name,
                                   inner_field->name, ret);
                    }


Not saying about a lot  of vmstate_load_state() other calls in the code, and 
callers do
passthrough the error to their callers, and so on. It's a huge work to analyze 
all of
them.


Hmm now I see that tpm_emulator_post_load() returns only -EIO code on all error 
paths.

Would it be correct just use -EIO here in my patch instead of -EINVAL? It will 
save
current behavior as is.



--
Best regards,
Vladimir