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

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

[Vladimir Sementsov-Ogievskiy]

v3:
01: add r-b by Philippe
02: new fix
03: new, update error messages
04: rebased, simplify commit message

Vladimir Sementsov-Ogievskiy (4):
  migration: vmstate_save_state_v(): fix error path
  tmp_emulator: fix unset errp on error path
  migration/vmstate: stop reporting error number for new _errp APIs
  migration: vmsd errp handlers: return bool

 backends/tpm/tpm_emulator.c   | 11 +++++------
 docs/devel/migration/main.rst |  6 +++---
 include/migration/vmstate.h   |  6 +++---
 migration/vmstate.c           | 26 ++++++++++++--------------
 4 files changed, 23 insertions(+), 26 deletions(-)

-- 
2.48.1

[PATCH v3 1/4] migration: vmstate_save_state_v(): fix error path

[Vladimir Sementsov-Ogievskiy]

In case of pre_save_errp, on error, we continue processing fields,
unlike case of pre_save, where we return immediately. Behavior
for pre_save_errp case is wrong, we must return here, like for
pre_save.

Fixes: 40de712a89
 "migration: Add error-parameterized function variants in VMSD struct"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 migration/vmstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 81eadde553..fd066f910e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -443,6 +443,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         if (ret < 0) {
             error_prepend(errp, "pre-save for %s failed, ret: %d: ",
                           vmsd->name, ret);
+            return ret;
         }
     } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
-- 
2.48.1

[PATCH v3 2/4] tmp_emulator: fix unset errp on error path

[Vladimir Sementsov-Ogievskiy]

Note, that called tpm_emulator_startup_tpm_resume() does error_report()
failure paths, which could be turned into error_setg() to passthough an
error. But, not on all error paths. Not saying about
tpm_emulator_startup_tpm_resume() return -1 on failure and we mix it
with -errno from tpm_emulator_set_state_blobs(). So, it all needs deeper
refactoring, which is out of scope of this small fix.

Fixes: 42e556fa3f7ac
    "backends/tpm: Propagate vTPM error on migration failure"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index dacfca5ab7..aa69eb606f 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -961,6 +961,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
     }
 
     if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
+        error_setg(errp, "Failed to resume tpm");
         return -EIO;
     }
 
-- 
2.48.1

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@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);
     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;
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -438,11 +437,10 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
     trace_vmstate_save_state_top(vmsd->name);
 
     if (vmsd->pre_save_errp) {
-        ret = vmsd->pre_save_errp(opaque, errp);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
-        if (ret < 0) {
+        if (!vmsd->pre_save_errp(opaque, errp)) {
             error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
-            return ret;
+            return -EINVAL;
         }
     } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
-- 
2.48.1

Re: [PATCH v3 2/4] tmp_emulator: fix unset errp on error path

[Markus Armbruster]

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

> Note, that called tpm_emulator_startup_tpm_resume() does error_report()
> failure paths, which could be turned into error_setg() to passthough an
> error. But, not on all error paths. Not saying about
> tpm_emulator_startup_tpm_resume() return -1 on failure and we mix it
> with -errno from tpm_emulator_set_state_blobs(). So, it all needs deeper
> refactoring, which is out of scope of this small fix.
>
> Fixes: 42e556fa3f7ac
>     "backends/tpm: Propagate vTPM error on migration failure"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  backends/tpm/tpm_emulator.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index dacfca5ab7..aa69eb606f 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -961,6 +961,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>      }
>  
>      if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
> +        error_setg(errp, "Failed to resume tpm");
>          return -EIO;
>      }

Anti-pattern: we call error_report() (via
tpm_emulator_startup_tpm_resume(), tpm_emulator_set_buffer_size, ...)
from within an Error-setting function.  You need to convert the entire
nest of functions to Error.

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

[Markus Armbruster]

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.

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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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?

>          }
>      } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
> @@ -438,11 +437,10 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>      trace_vmstate_save_state_top(vmsd->name);
>  
>      if (vmsd->pre_save_errp) {
> -        ret = vmsd->pre_save_errp(opaque, errp);
>          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
> -        if (ret < 0) {
> +        if (!vmsd->pre_save_errp(opaque, errp)) {
>              error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
> -            return ret;
> +            return -EINVAL;
>          }
>      } else if (vmsd->pre_save) {
>          ret = vmsd->pre_save(opaque);
> -- 
> 2.48.1

Re: [PATCH v3 1/4] migration: vmstate_save_state_v(): fix error path

[Stefan Berger]

On 10/25/25 4:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> In case of pre_save_errp, on error, we continue processing fields,
> unlike case of pre_save, where we return immediately. Behavior
> for pre_save_errp case is wrong, we must return here, like for
> pre_save.
> 
> Fixes: 40de712a89
>   "migration: Add error-parameterized function variants in VMSD struct"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   migration/vmstate.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 81eadde553..fd066f910e 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -443,6 +443,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>           if (ret < 0) {
>               error_prepend(errp, "pre-save for %s failed, ret: %d: ",
>                             vmsd->name, ret);
> +            return ret;
>           }
>       } else if (vmsd->pre_save) {
>           ret = vmsd->pre_save(opaque);

Re: [PATCH v3 2/4] tmp_emulator: fix unset errp on error path

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 13:16, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Note, that called tpm_emulator_startup_tpm_resume() does error_report()
>> failure paths, which could be turned into error_setg() to passthough an
>> error. But, not on all error paths. Not saying about
>> tpm_emulator_startup_tpm_resume() return -1 on failure and we mix it
>> with -errno from tpm_emulator_set_state_blobs(). So, it all needs deeper
>> refactoring, which is out of scope of this small fix.
>>
>> Fixes: 42e556fa3f7ac
>>      "backends/tpm: Propagate vTPM error on migration failure"
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   backends/tpm/tpm_emulator.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index dacfca5ab7..aa69eb606f 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -961,6 +961,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>       }
>>   
>>       if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>> +        error_setg(errp, "Failed to resume tpm");
>>           return -EIO;
>>       }
> 
> Anti-pattern: we call error_report() (via
> tpm_emulator_startup_tpm_resume(), tpm_emulator_set_buffer_size, ...)
> from within an Error-setting function.  You need to convert the entire
> nest of functions to Error.
> 

Is it a show-stopper for bug-fix? I'm afraid, I have no time to convert the whole
tpm_emulator.c.


-- 
Best regards,
Vladimir

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

[Stefan Berger]

On 10/25/25 4:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> 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.

You mean in all paths or just the .post_load_errp path? In the 
.post_load_errp cases -EIO is currently always returned if called 
functions failed and return a -1. So their -1 does not propagate.

> 
> 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) {

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

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 17:38, Stefan Berger wrote:
> 
> 
> On 10/25/25 4:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
> 
> You mean in all paths or just the .post_load_errp path? In the .post_load_errp cases -EIO is currently always returned if called functions failed and return a -1. So their -1 does not propagate.


Oops, excuse me, I was wrong.

Now I see that tpm_emulator_post_load return either 0 or -EIO on all 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) {
> 


-- 
Best regards,
Vladimir

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 <vsementsov@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 <vsementsov@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

Re: [PATCH v3 2/4] tmp_emulator: fix unset errp on error path

[Markus Armbruster]

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

> On 27.10.25 13:16, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> Note, that called tpm_emulator_startup_tpm_resume() does error_report()
>>> failure paths, which could be turned into error_setg() to passthough an
>>> error. But, not on all error paths. Not saying about
>>> tpm_emulator_startup_tpm_resume() return -1 on failure and we mix it
>>> with -errno from tpm_emulator_set_state_blobs(). So, it all needs deeper
>>> refactoring, which is out of scope of this small fix.
>>>
>>> Fixes: 42e556fa3f7ac
>>>      "backends/tpm: Propagate vTPM error on migration failure"
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   backends/tpm/tpm_emulator.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>>> index dacfca5ab7..aa69eb606f 100644
>>> --- a/backends/tpm/tpm_emulator.c
>>> +++ b/backends/tpm/tpm_emulator.c
>>> @@ -961,6 +961,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>>      }
>>>          if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>>> +        error_setg(errp, "Failed to resume tpm");
>>>          return -EIO;
>>>      }
>>
>> Anti-pattern: we call error_report() (via
>> tpm_emulator_startup_tpm_resume(), tpm_emulator_set_buffer_size, ...)
>> from within an Error-setting function.  You need to convert the entire
>> nest of functions to Error.
>> 
>
> Is it a show-stopper for bug-fix? I'm afraid, I have no time to convert the whole
> tpm_emulator.c.

Fair.  Throw in a FIXME comment, and I'll give my R-by.

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

[Arun Menon]

Hi Vladimir,

On Mon, Oct 27, 2025 at 05:58:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 27.10.25 13:38, Markus Armbruster wrote:
> > Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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.

I admit I too was not sure whether to use int or bool return type.
A bit of the discussion is here: https://lore.kernel.org/qemu-devel/87v7mbsjb0.fsf@pond.sub.org/
I found few places where a genuine error code was returned from the function
For example:
vmbus_post_load() calls vmbus_init() that returns -ENOMEM on failure.

The intention was to eventually phase out the old implementation and replace them
with the new ones (errp)
We wanted the new errp variants to be structurally as close to the old
ones as possible so that we can perform a bulk change later.

> 
> 
> 
> -- 
> Best regards,
> Vladimir
> 

Regards,
Arun Menon

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

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 19:30, Arun Menon wrote:
> Hi Vladimir,
> 
> On Mon, Oct 27, 2025 at 05:58:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 27.10.25 13:38, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@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 <vsementsov@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.
> 
> I admit I too was not sure whether to use int or bool return type.
> A bit of the discussion is here: https://lore.kernel.org/qemu-devel/87v7mbsjb0.fsf@pond.sub.org/
> I found few places where a genuine error code was returned from the function
> For example:
> vmbus_post_load() calls vmbus_init() that returns -ENOMEM on failure.
> 
> The intention was to eventually phase out the old implementation and replace them
> with the new ones (errp)
> We wanted the new errp variants to be structurally as close to the old
> ones as possible so that we can perform a bulk change later.
> 

Understand.. So we don't know, does any caller use this ENOMEM, or not. And want to save
a chance for bulk conversion.

And blind bulk conversion of all -errno to simple true/false may break something, we
don't know.

Reasonable. Thanks for the explanation.

-- 
Best regards,
Vladimir

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

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

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 27.10.25 19:30, Arun Menon wrote:
> We wanted the new errp variants to be structurally as close to the old
> ones as possible so that we can perform a bulk change later.

Still, could you clarify, are you plan to do this bulk change? Or it was
a theoretical possibility?

-- 
Best regards,
Vladimir

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

[Arun Menon]

Hi Vladimir,

On Mon, Oct 27, 2025 at 11:33:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 27.10.25 19:30, Arun Menon wrote:
> > We wanted the new errp variants to be structurally as close to the old
> > ones as possible so that we can perform a bulk change later.
> 
> Still, could you clarify, are you plan to do this bulk change? Or it was
> a theoretical possibility?

Yes I want to, when time permits. Most of the post_load() calls,
just like post_save, do not fail. That is the reason why post_save does not have
an errp variant. They always return 0.
But there are a few of them that can fail and return an error code. I need to figure out
what would be the appropriate messages to set in errp in those cases so we
can propagate it back to the caller.

> 
> -- 
> Best regards,
> Vladimir
> 

Regards,
Arun Menon

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

[Peter Xu]

On Mon, Oct 27, 2025 at 08:06:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Understand.. So we don't know, does any caller use this ENOMEM, or not. And want to save
> a chance for bulk conversion.
> 
> And blind bulk conversion of all -errno to simple true/false may break something, we
> don't know.
> 
> Reasonable. Thanks for the explanation.

Taking vmstate_load_state() as example: most of its callers should
ultimately route back to the top of vmstate_load_state() that migration
code invokes.

AFAIU, migration itself doesn't care much so far on the retval, but only
succeeded or not, plus the errp as a bonus.  There's one thing exception
that I remember but it was removed in commit fd037a656aca..  So now I
cannot find anything relies on that.

Here's a list of all current vmstate_load_state() callers:

*** hw/display/virtio-gpu.c:
virtio_gpu_load[1358]          ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);

*** hw/pci/pci.c:
pci_device_load[943]           ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,

*** hw/s390x/virtio-ccw.c:
virtio_ccw_load_config[1146]   ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);

*** hw/scsi/spapr_vscsi.c:
vscsi_load_request[657]        rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &local_err);

Until here, it's invoked in a get() callback of VMStateInfo.  Ultimately,
it goes back to migration's vmstate_load_state() invokation.

*** hw/vfio/pci.c:
vfio_pci_load_config[2840]     ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,

This is special as it's part of load_state().  However it's the same, it
routes back to vmstate_load() instead (where vmstate_load_state()
ultimately also routes there).  So looks safe too.

*** hw/virtio/virtio-mmio.c:
virtio_mmio_load_extra_state[630] ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);

*** hw/virtio/virtio-pci.c:
virtio_pci_load_extra_state[205] ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);

*** hw/virtio/virtio.c:
virtio_load[3394]              ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &local_err);
virtio_load[3402]              ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &local_err);

More get() users.  Same.

*** migration/cpr.c:
cpr_state_load[266]            ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);

This is special, ignoring retval but using error_abort.  New one, safe.

*** migration/savevm.c:
vmstate_load[978]              return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
qemu_loadvm_state_header[2885] ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,

This is the migration core invokations.  Safe.

*** migration/vmstate-types.c:
get_tmp[562]                   ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
get_qtailq[670]                ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
get_gtree[833]                 ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
get_gtree[840]                 ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
get_qlist[921]                 ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);

These are special get() for special VMSD fields.  Safe.

*** migration/vmstate.c:
vmstate_load_state[208]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
vmstate_load_state[212]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
vmstate_subsection_load[652]   ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);

Same migration core invokations. Safe.

*** tests/unit/test-vmstate.c:
load_vmstate_one[123]          ret = vmstate_load_state(f, desc, obj, version, &local_err);
test_load_v1[377]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
test_load_v2[408]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
test_load_noskip[512]          ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
test_load_skip[541]            ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
test_load_q[815]               ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
test_gtree_load_domain[1174]   ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
test_gtree_load_iommu[1294]    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
test_load_qlist[1434]          ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,

Test code only.  Safe.

*** ui/vdagent.c:
get_cbinfo[1013]               ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,

Yet another get().  Safe.

So.. even if I'm not sure if a bulk conversion could happen or not (the
get() users above would be very tricky because get() doesn't allow errp so
far.. unless we introduce that too), but other than that, afaict,
vmstate_load_state() callers do not yet care about retvals.

-- 
Peter Xu

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

[Vladimir Sementsov-Ogievskiy]

On 28.10.25 18:12, Peter Xu wrote:
> On Mon, Oct 27, 2025 at 08:06:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Understand.. So we don't know, does any caller use this ENOMEM, or not. And want to save
>> a chance for bulk conversion.
>>
>> And blind bulk conversion of all -errno to simple true/false may break something, we
>> don't know.
>>
>> Reasonable. Thanks for the explanation.
> 
> Taking vmstate_load_state() as example: most of its callers should
> ultimately route back to the top of vmstate_load_state() that migration
> code invokes.
> 
> AFAIU, migration itself doesn't care much so far on the retval, but only
> succeeded or not, plus the errp as a bonus.  There's one thing exception
> that I remember but it was removed in commit fd037a656aca..  So now I
> cannot find anything relies on that.
> 
> Here's a list of all current vmstate_load_state() callers:
> 
> *** hw/display/virtio-gpu.c:
> virtio_gpu_load[1358]          ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> 
> *** hw/pci/pci.c:
> pci_device_load[943]           ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> 
> *** hw/s390x/virtio-ccw.c:
> virtio_ccw_load_config[1146]   ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);
> 
> *** hw/scsi/spapr_vscsi.c:
> vscsi_load_request[657]        rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &local_err);
> 
> Until here, it's invoked in a get() callback of VMStateInfo.  Ultimately,
> it goes back to migration's vmstate_load_state() invokation.
> 
> *** hw/vfio/pci.c:
> vfio_pci_load_config[2840]     ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> 
> This is special as it's part of load_state().  However it's the same, it
> routes back to vmstate_load() instead (where vmstate_load_state()
> ultimately also routes there).  So looks safe too.
> 
> *** hw/virtio/virtio-mmio.c:
> virtio_mmio_load_extra_state[630] ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);
> 
> *** hw/virtio/virtio-pci.c:
> virtio_pci_load_extra_state[205] ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);
> 
> *** hw/virtio/virtio.c:
> virtio_load[3394]              ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &local_err);
> virtio_load[3402]              ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &local_err);
> 
> More get() users.  Same.
> 
> *** migration/cpr.c:
> cpr_state_load[266]            ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> 
> This is special, ignoring retval but using error_abort.  New one, safe.
> 
> *** migration/savevm.c:
> vmstate_load[978]              return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> qemu_loadvm_state_header[2885] ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> 
> This is the migration core invokations.  Safe.
> 
> *** migration/vmstate-types.c:
> get_tmp[562]                   ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> get_qtailq[670]                ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> get_gtree[833]                 ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
> get_gtree[840]                 ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
> get_qlist[921]                 ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> 
> These are special get() for special VMSD fields.  Safe.
> 
> *** migration/vmstate.c:
> vmstate_load_state[208]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> vmstate_load_state[212]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> vmstate_subsection_load[652]   ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> 
> Same migration core invokations. Safe.
> 
> *** tests/unit/test-vmstate.c:
> load_vmstate_one[123]          ret = vmstate_load_state(f, desc, obj, version, &local_err);
> test_load_v1[377]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
> test_load_v2[408]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
> test_load_noskip[512]          ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> test_load_skip[541]            ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> test_load_q[815]               ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
> test_gtree_load_domain[1174]   ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
> test_gtree_load_iommu[1294]    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
> test_load_qlist[1434]          ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,
> 
> Test code only.  Safe.
> 
> *** ui/vdagent.c:
> get_cbinfo[1013]               ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,
> 
> Yet another get().  Safe.
> 
> So.. even if I'm not sure if a bulk conversion could happen or not (the
> get() users above would be very tricky because get() doesn't allow errp so
> far.. unless we introduce that too), but other than that, afaict,
> vmstate_load_state() callers do not yet care about retvals.
> 

Uhh, great analysis! With it, we can proceed with my patch. And may be, just change return value of vmstate_load_state to bool? To avoid analyzing it again in future.

-- 
Best regards,
Vladimir

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

[Peter Xu]

On Tue, Oct 28, 2025 at 07:26:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 28.10.25 18:12, Peter Xu wrote:
> > On Mon, Oct 27, 2025 at 08:06:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Understand.. So we don't know, does any caller use this ENOMEM, or not. And want to save
> > > a chance for bulk conversion.
> > > 
> > > And blind bulk conversion of all -errno to simple true/false may break something, we
> > > don't know.
> > > 
> > > Reasonable. Thanks for the explanation.
> > 
> > Taking vmstate_load_state() as example: most of its callers should
> > ultimately route back to the top of vmstate_load_state() that migration
> > code invokes.
> > 
> > AFAIU, migration itself doesn't care much so far on the retval, but only
> > succeeded or not, plus the errp as a bonus.  There's one thing exception
> > that I remember but it was removed in commit fd037a656aca..  So now I
> > cannot find anything relies on that.
> > 
> > Here's a list of all current vmstate_load_state() callers:
> > 
> > *** hw/display/virtio-gpu.c:
> > virtio_gpu_load[1358]          ret = vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &err);
> > 
> > *** hw/pci/pci.c:
> > pci_device_load[943]           ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> > 
> > *** hw/s390x/virtio-ccw.c:
> > virtio_ccw_load_config[1146]   ret = vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &local_err);
> > 
> > *** hw/scsi/spapr_vscsi.c:
> > vscsi_load_request[657]        rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &local_err);
> > 
> > Until here, it's invoked in a get() callback of VMStateInfo.  Ultimately,
> > it goes back to migration's vmstate_load_state() invokation.
> > 
> > *** hw/vfio/pci.c:
> > vfio_pci_load_config[2840]     ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> > 
> > This is special as it's part of load_state().  However it's the same, it
> > routes back to vmstate_load() instead (where vmstate_load_state()
> > ultimately also routes there).  So looks safe too.
> > 
> > *** hw/virtio/virtio-mmio.c:
> > virtio_mmio_load_extra_state[630] ret = vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &local_err);
> > 
> > *** hw/virtio/virtio-pci.c:
> > virtio_pci_load_extra_state[205] ret = vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &local_err);
> > 
> > *** hw/virtio/virtio.c:
> > virtio_load[3394]              ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &local_err);
> > virtio_load[3402]              ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &local_err);
> > 
> > More get() users.  Same.
> > 
> > *** migration/cpr.c:
> > cpr_state_load[266]            ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> > 
> > This is special, ignoring retval but using error_abort.  New one, safe.
> > 
> > *** migration/savevm.c:
> > vmstate_load[978]              return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > qemu_loadvm_state_header[2885] ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> > 
> > This is the migration core invokations.  Safe.
> > 
> > *** migration/vmstate-types.c:
> > get_tmp[562]                   ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> > get_qtailq[670]                ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> > get_gtree[833]                 ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
> > get_gtree[840]                 ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
> > get_qlist[921]                 ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> > 
> > These are special get() for special VMSD fields.  Safe.
> > 
> > *** migration/vmstate.c:
> > vmstate_load_state[208]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > vmstate_load_state[212]        ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> > vmstate_subsection_load[652]   ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> > 
> > Same migration core invokations. Safe.
> > 
> > *** tests/unit/test-vmstate.c:
> > load_vmstate_one[123]          ret = vmstate_load_state(f, desc, obj, version, &local_err);
> > test_load_v1[377]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
> > test_load_v2[408]              ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
> > test_load_noskip[512]          ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > test_load_skip[541]            ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > test_load_q[815]               ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
> > test_gtree_load_domain[1174]   ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
> > test_gtree_load_iommu[1294]    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
> > test_load_qlist[1434]          ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,
> > 
> > Test code only.  Safe.
> > 
> > *** ui/vdagent.c:
> > get_cbinfo[1013]               ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,
> > 
> > Yet another get().  Safe.
> > 
> > So.. even if I'm not sure if a bulk conversion could happen or not (the
> > get() users above would be very tricky because get() doesn't allow errp so
> > far.. unless we introduce that too), but other than that, afaict,
> > vmstate_load_state() callers do not yet care about retvals.
> > 
> 
> Uhh, great analysis! With it, we can proceed with my patch. And may be, just change return value of vmstate_load_state to bool? To avoid analyzing it again in future.

It'll be some more code churns.. so some more work for downstream
maintenances.  But yeah, I agree that should follow what we suggest to do
in qemu in general.

I also didn't check the save side.  I would expect to be similar, but worth
some double checks.

Thanks,

-- 
Peter Xu