Recently we moved to returning errp. Why to keep int return value?
Generally it doesn't help: you can't use in a logic of handling
an error, as you are never sure, that in future the logic in
the stack will not change: it may start to return another error
code in the same case, or return same error code in another case.

Actually, we can only rely on concrete errno code when get it
_directly_ from documented library function or syscall. This way we
handle for example EINTR. But later in a stack, we can only add
this errno to the textual error by strerror().

Let this new, recently added API be simpler and clearer, let it
return simple boolean value, so we shouldn't think:

  - should we handle different error codes differently
    (if yes - how exactly, if no - why do we need this information?)

  - should we add it to errp, or it was already added earlier in
    the stack

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           | 24 +++++++++++-------------
 4 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index dacfca5ab7..3937ac5572 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -947,24 +947,22 @@ 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) {
-        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 fd066f910e..921e09c38e 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, "
-                          "ret: %d: ", vmsd->name, vmsd->version_id,
-                          vmsd->minimum_version_id, ret);
-            return ret;
+                          "version_id: %d, minimum version_id: %d: "
+                          vmsd->name, vmsd->version_id,
+                          vmsd->minimum_version_id);
+            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, 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);
+            ret = -EINVAL;
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -438,12 +437,11 @@ 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, ret: %d: ",
                           vmsd->name, ret);
-            return ret;
+            return -EINVAL;
         }
     } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
-- 
2.48.1

On 20.10.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
> Recently we moved to returning errp. Why to keep int return value?
> Generally it doesn't help: you can't use in a logic of handling
> an error, as you are never sure, that in future the logic in
> the stack will not change: it may start to return another error
> code in the same case, or return same error code in another case.
> 
> Actually, we can only rely on concrete errno code when get it
> _directly_ from documented library function or syscall. This way we
> handle for example EINTR. But later in a stack, we can only add
> this errno to the textual error by strerror().
> 
> Let this new, recently added API be simpler and clearer, let it
> return simple boolean value, so we shouldn't think:
> 
>    - should we handle different error codes differently
>      (if yes - how exactly, if no - why do we need this information?)
> 
>    - should we add it to errp, or it was already added earlier in
>      the stack
> 
> 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           | 24 +++++++++++-------------
>   4 files changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index dacfca5ab7..3937ac5572 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -947,24 +947,22 @@ 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) {

O_o, we've missed an elephant (errp is not set here).

> -        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 fd066f910e..921e09c38e 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, "
> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
> -                          vmsd->minimum_version_id, ret);
> -            return ret;
> +                          "version_id: %d, minimum version_id: %d: "
> +                          vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id);
> +            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, 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);
> +            ret = -EINVAL;
>           }
>       } else if (vmsd->post_load) {
>           ret = vmsd->post_load(opaque, version_id);
> @@ -438,12 +437,11 @@ 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, ret: %d: ",
>                             vmsd->name, ret);
> -            return ret;
> +            return -EINVAL;
>           }
>       } else if (vmsd->pre_save) {
>           ret = vmsd->pre_save(opaque);


-- 
Best regards,
Vladimir

On 20.10.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
> Recently we moved to returning errp. Why to keep int return value?
> Generally it doesn't help: you can't use in a logic of handling
> an error, as you are never sure, that in future the logic in
> the stack will not change: it may start to return another error
> code in the same case, or return same error code in another case.
> 
> Actually, we can only rely on concrete errno code when get it
> _directly_ from documented library function or syscall. This way we
> handle for example EINTR. But later in a stack, we can only add
> this errno to the textual error by strerror().
> 
> Let this new, recently added API be simpler and clearer, let it
> return simple boolean value, so we shouldn't think:
> 
>    - should we handle different error codes differently
>      (if yes - how exactly, if no - why do we need this information?)
> 
>    - should we add it to errp, or it was already added earlier in
>      the stack


Less aggressive commit message may be:

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           | 24 +++++++++++-------------
>   4 files changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index dacfca5ab7..3937ac5572 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -947,24 +947,22 @@ 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) {
> -        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 fd066f910e..921e09c38e 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, "
> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
> -                          vmsd->minimum_version_id, ret);
> -            return ret;
> +                          "version_id: %d, minimum version_id: %d: "
> +                          vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id);
> +            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, 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);
> +            ret = -EINVAL;
>           }
>       } else if (vmsd->post_load) {
>           ret = vmsd->post_load(opaque, version_id);
> @@ -438,12 +437,11 @@ 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, ret: %d: ",
>                             vmsd->name, ret);
> -            return ret;
> +            return -EINVAL;
>           }
>       } else if (vmsd->pre_save) {
>           ret = vmsd->pre_save(opaque);


-- 
Best regards,
Vladimir

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

> On 20.10.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
>> Recently we moved to returning errp. Why to keep int return value?
>> Generally it doesn't help: you can't use in a logic of handling
>> an error, as you are never sure, that in future the logic in
>> the stack will not change: it may start to return another error
>> code in the same case, or return same error code in another case.
>> Actually, we can only rely on concrete errno code when get it
>> _directly_ from documented library function or syscall. This way we
>> handle for example EINTR. But later in a stack, we can only add
>> this errno to the textual error by strerror().
>> Let this new, recently added API be simpler and clearer, let it
>> return simple boolean value, so we shouldn't think:
>>    - should we handle different error codes differently
>>      (if yes - how exactly, if no - why do we need this information?)
>>    - should we add it to errp, or it was already added earlier in
>>      the stack
>
>
> Less aggressive commit message may be:
>
> 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.

Good, except it isn't quite accurate: callers include the return value
in error messages.

They format it as a number, which is inappropriate.

If they formatted it appropriately, with strerror(), would that be
helpful?  I *think* the answer is no.  The functions set an Error, and
their callers prepend to that Error's message.  The original Error
should suffice to tell what went wrong.  The prepended text's purpose is
to add context.

I think the easiest way to make this argument is to split this patch
into a part changing the error messages, and a part changing the return
type.

On 22.10.25 09:14, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 20.10.25 19:03, Vladimir Sementsov-Ogievskiy wrote:
>>> Recently we moved to returning errp. Why to keep int return value?
>>> Generally it doesn't help: you can't use in a logic of handling
>>> an error, as you are never sure, that in future the logic in
>>> the stack will not change: it may start to return another error
>>> code in the same case, or return same error code in another case.
>>> Actually, we can only rely on concrete errno code when get it
>>> _directly_ from documented library function or syscall. This way we
>>> handle for example EINTR. But later in a stack, we can only add
>>> this errno to the textual error by strerror().
>>> Let this new, recently added API be simpler and clearer, let it
>>> return simple boolean value, so we shouldn't think:
>>>     - should we handle different error codes differently
>>>       (if yes - how exactly, if no - why do we need this information?)
>>>     - should we add it to errp, or it was already added earlier in
>>>       the stack
>>
>>
>> Less aggressive commit message may be:
>>
>> 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.
> 
> Good, except it isn't quite accurate: callers include the return value
> in error messages.
> 
> They format it as a number, which is inappropriate.
> 
> If they formatted it appropriately, with strerror(), would that be
> helpful?  I *think* the answer is no.  The functions set an Error, and
> their callers prepend to that Error's message.  The original Error
> should suffice to tell what went wrong.  The prepended text's purpose is
> to add context.
> 
> I think the easiest way to make this argument is to split this patch
> into a part changing the error messages, and a part changing the return
> type.
> 

Agreed, will do.

-- 
Best regards,
Vladimir