[PATCH] migration: vmsd errp handlers: return bool

[PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

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           | 31 +++++++++++++------------------
 4 files changed, 23 insertions(+), 30 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 81eadde553..026fd6f1a4 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -153,15 +153,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
         return -EINVAL;
     }
-    if (vmsd->pre_load_errp) {
-        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);
-            return ret;
-        }
+    if (vmsd->pre_load_errp && !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 -EINVAL;
     } else if (vmsd->pre_load) {
         ret = vmsd->pre_load(opaque);
         if (ret) {
@@ -255,13 +252,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         qemu_file_set_error(f, ret);
         return ret;
     }
-    if (vmsd->post_load_errp) {
-        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);
-        }
+    if (vmsd->post_load_errp && !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);
+        ret = -EINVAL;
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
         if (ret < 0) {
@@ -438,9 +434,8 @@ 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);
         }
-- 
2.48.1

Re: [PATCH] migration: vmsd errp handlers: return bool

[Markus Armbruster]

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

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

It's a matter of the function's contract, actually.

If the contract is "Return negative value on failure", checking for
failure is all you can do with it.  Same information as "Return false on
failure".

If the contract is "Return negative errno on failure", the function is
responsible for returning values that make sense.  Ideally, the contract
spells them all out.

No difference between "documented library function or syscall" and a
function we provide ourselves.

> 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

When no caller actually needs to distinguish between errors, bool is the
most obvious interface.

> 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           | 31 +++++++++++++------------------
>  4 files changed, 23 insertions(+), 30 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 81eadde553..026fd6f1a4 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -153,15 +153,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>          return -EINVAL;
>      }
> -    if (vmsd->pre_load_errp) {
> -        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);

Numeric errno codes in error messages are an anti-pattern.

> -            return ret;
> -        }
> +    if (vmsd->pre_load_errp && !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);

Did you forget to delete ", ret %d:" and its argument @ret?  It's always
zero here now.

> +        return -EINVAL;
>      } else if (vmsd->pre_load) {
>          ret = vmsd->pre_load(opaque);
>          if (ret) {
> @@ -255,13 +252,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          qemu_file_set_error(f, ret);
>          return ret;
>      }
> -    if (vmsd->post_load_errp) {
> -        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);
> -        }
> +    if (vmsd->post_load_errp && !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);

Likewise.

> +        ret = -EINVAL;
>      } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
>          if (ret < 0) {
> @@ -438,9 +434,8 @@ 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);
>          }

Likewise.

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 20.10.25 14:05, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> 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().
> 
> It's a matter of the function's contract, actually.
> 
> If the contract is "Return negative value on failure", checking for
> failure is all you can do with it.  Same information as "Return false on
> failure".
> 
> If the contract is "Return negative errno on failure", the function is
> responsible for returning values that make sense.  Ideally, the contract
> spells them all out.
> 

Do you know an example in code where we have both errno return value
and errp, and the return value make sense and used by callers?

> No difference between "documented library function or syscall" and a
> function we provide ourselves.

I agree... Still the only difference is that for library function
it's OK to provide specific error only for caller to be able to print it
to the user (with help of strerror). But for our functions with errp,
it's assumed that the whole information for the user is already in errp.

So I now think, shouldn't we actually do

diff --git a/include/qapi/error.h b/include/qapi/error.h
index d798faeec3..1c2484187f 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -43,8 +43,7 @@
   *   avoid useless error object creation and destruction.  Note that
   *   we still have many functions returning void.  We recommend
   *   • bool-valued functions return true on success / false on failure,
- *   • pointer-valued functions return non-null / null pointer, and
- *   • integer-valued functions return non-negative / negative.
+ *   • pointer-valued functions return non-null / null pointer.
   *
   * = Creating errors =
   *


?

> 
>> 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
> 
> When no caller actually needs to distinguish between errors, bool is the
> most obvious interface.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   backends/tpm/tpm_emulator.c   | 10 ++++------

[..]

>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -153,15 +153,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>>           return -EINVAL;
>>       }
>> -    if (vmsd->pre_load_errp) {
>> -        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);
> 
> Numeric errno codes in error messages are an anti-pattern.
> 
>> -            return ret;
>> -        }
>> +    if (vmsd->pre_load_errp && !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);
> 
> Did you forget to delete ", ret %d:" and its argument @ret?  It's always
> zero here now.
> 
Oh right, thanks.

-- 
Best regards,
Vladimir

Re: [PATCH] migration: vmsd errp handlers: return bool

[Daniel P. Berrangé]

On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 20.10.25 14:05, Markus Armbruster wrote:
> > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> > 
> > > 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().
> > 
> > It's a matter of the function's contract, actually.
> > 
> > If the contract is "Return negative value on failure", checking for
> > failure is all you can do with it.  Same information as "Return false on
> > failure".
> > 
> > If the contract is "Return negative errno on failure", the function is
> > responsible for returning values that make sense.  Ideally, the contract
> > spells them all out.
> > 
> 
> Do you know an example in code where we have both errno return value
> and errp, and the return value make sense and used by callers?

If there are examples of that, I would generally consider them to be
bugs.

IMHO if a method is using "Error **errp", then it should be considered
forbidden to return 'errno' values.

If there is a need for distinguishing some cases from others, then keep
with int '0/-1' example, but turn it into a multi-value return such as
1/0/-1, or 0/-1/-2/-3/..., etc with named constants for the unusual
scenarios. An example of that would be QIOChannel were we introduced
"#define QIO_CHANNEL_ERR_BLOCK -2" to replace the need for EAGAIN checks
in callers.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] migration: vmsd errp handlers: return bool

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 20.10.25 14:05, Markus Armbruster wrote:
>> > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> > 
>> > > 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().
>> > 
>> > It's a matter of the function's contract, actually.
>> > 
>> > If the contract is "Return negative value on failure", checking for
>> > failure is all you can do with it.  Same information as "Return false on
>> > failure".
>> > 
>> > If the contract is "Return negative errno on failure", the function is
>> > responsible for returning values that make sense.  Ideally, the contract
>> > spells them all out.
>> > 
>> 
>> Do you know an example in code where we have both errno return value
>> and errp, and the return value make sense and used by callers?
>
> If there are examples of that, I would generally consider them to be
> bugs.
>
> IMHO if a method is using "Error **errp", then it should be considered
> forbidden to return 'errno' values.

Several subsystems disagree :)

Quick & dirty search without a claim to accuracy or completeness:

    $ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'

> If there is a need for distinguishing some cases from others, then keep
> with int '0/-1' example, but turn it into a multi-value return such as
> 1/0/-1, or 0/-1/-2/-3/..., etc with named constants for the unusual
> scenarios. An example of that would be QIOChannel were we introduced
> "#define QIO_CHANNEL_ERR_BLOCK -2" to replace the need for EAGAIN checks
> in callers.

Defining your own error codes is fine.

Reusing errno codes can also be fine.

In both cases, the function contract is a load-bearing component.

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 20.10.25 17:34, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> On 20.10.25 14:05, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>
>>>>> 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().
>>>>
>>>> It's a matter of the function's contract, actually.
>>>>
>>>> If the contract is "Return negative value on failure", checking for
>>>> failure is all you can do with it.  Same information as "Return false on
>>>> failure".
>>>>
>>>> If the contract is "Return negative errno on failure", the function is
>>>> responsible for returning values that make sense.  Ideally, the contract
>>>> spells them all out.
>>>>
>>>
>>> Do you know an example in code where we have both errno return value
>>> and errp, and the return value make sense and used by callers?
>>
>> If there are examples of that, I would generally consider them to be
>> bugs.
>>
>> IMHO if a method is using "Error **errp", then it should be considered
>> forbidden to return 'errno' values.
> 
> Several subsystems disagree :)

I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
but blindly follow usual pattern of returning -errno together with
errp, while having no reasonable contract on concrete errno values,
and with this errno finally unused (used only to check, it is it < 0,
like boolean). In other words, the only contract they have is
"< 0 is error, otherwise success".

> 
> Quick & dirty search without a claim to accuracy or completeness:
> 
>      $ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'
> 
>> If there is a need for distinguishing some cases from others, then keep
>> with int '0/-1' example, but turn it into a multi-value return such as
>> 1/0/-1, or 0/-1/-2/-3/..., etc with named constants for the unusual
>> scenarios. An example of that would be QIOChannel were we introduced
>> "#define QIO_CHANNEL_ERR_BLOCK -2" to replace the need for EAGAIN checks
>> in callers.
> 
> Defining your own error codes is fine.
> 
> Reusing errno codes can also be fine.
> 
> In both cases, the function contract is a load-bearing component.
> 


-- 
Best regards,
Vladimir

Re: [PATCH] migration: vmsd errp handlers: return bool

[Markus Armbruster]

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

> On 20.10.25 17:34, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>>> On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 20.10.25 14:05, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>>
>>>>>> 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().
>>>>>
>>>>> It's a matter of the function's contract, actually.
>>>>>
>>>>> If the contract is "Return negative value on failure", checking for
>>>>> failure is all you can do with it.  Same information as "Return false on
>>>>> failure".
>>>>>
>>>>> If the contract is "Return negative errno on failure", the function is
>>>>> responsible for returning values that make sense.  Ideally, the contract
>>>>> spells them all out.
>>>>>
>>>>
>>>> Do you know an example in code where we have both errno return value
>>>> and errp, and the return value make sense and used by callers?
>>>
>>> If there are examples of that, I would generally consider them to be
>>> bugs.
>>>
>>> IMHO if a method is using "Error **errp", then it should be considered
>>> forbidden to return 'errno' values.
>>
>> Several subsystems disagree :)
>
> I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
> but blindly follow usual pattern of returning -errno together with
> errp, while having no reasonable contract on concrete errno values,
> and with this errno finally unused (used only to check, it is it < 0,
> like boolean). In other words, the only contract they have is
> "< 0 is error, otherwise success".

Functions that could just as well return -1 instead of errno exist.

Functions that return negative errno with callers that use them also
exist.

I'm not going to speculate on relative frequency.

I much prefer written function contracts.  But if a caller relies on
negative errno codes, there is an unwritten contract whether we like it
or not.

>> Quick & dirty search without a claim to accuracy or completeness:
>>      $ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'

[...]

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 20.10.25 19:40, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 20.10.25 17:34, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> On 20.10.25 14:05, Markus Armbruster wrote:
>>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>>>>
>>>>>>> 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().
>>>>>>
>>>>>> It's a matter of the function's contract, actually.
>>>>>>
>>>>>> If the contract is "Return negative value on failure", checking for
>>>>>> failure is all you can do with it.  Same information as "Return false on
>>>>>> failure".
>>>>>>
>>>>>> If the contract is "Return negative errno on failure", the function is
>>>>>> responsible for returning values that make sense.  Ideally, the contract
>>>>>> spells them all out.
>>>>>>
>>>>>
>>>>> Do you know an example in code where we have both errno return value
>>>>> and errp, and the return value make sense and used by callers?
>>>>
>>>> If there are examples of that, I would generally consider them to be
>>>> bugs.
>>>>
>>>> IMHO if a method is using "Error **errp", then it should be considered
>>>> forbidden to return 'errno' values.
>>>
>>> Several subsystems disagree :)
>>
>> I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
>> but blindly follow usual pattern of returning -errno together with
>> errp, while having no reasonable contract on concrete errno values,
>> and with this errno finally unused (used only to check, it is it < 0,
>> like boolean). In other words, the only contract they have is
>> "< 0 is error, otherwise success".
> 
> Functions that could just as well return -1 instead of errno exist.
> 
> Functions that return negative errno with callers that use them also
> exist.


But do functions that return negative errno together with errp, with
callers that use this errno exit? I don't ask to find, that's not simple.
I just say, that I myself don't know any of such functions.


upd: I found two!

how:

1. git grep -A 20 'ret = .*errp)'
2. in opened pager, do `/if \(ret == -E`


in iommufd_cdev_autodomains_get() we do something just wrong: we clear errp
after iommufd_cdev_attach_ioas_hwpt(), but return false, which is treated
as error (but with cleared errp!) by callers...


in qemu_read_default_config_file() we do correct thing, but keeping in mind,
that it's very seldom practice (around one case), we'd better add a boolean
parameter to qemu_read_config_file(), and parse errno exactly after call to
fopen().


trying with local_err gives a bit more:

git grep -A 20 'ret = .*&\(local_\)\?err)' | grep 'ret == -E'
block.c-    if (ret == -ENOTSUP) {
block.c-    if (ret == -EFBIG) {
block/snapshot.c-    if (ret == -ENOENT || ret == -EINVAL) {
hw/core/loader-fit.c-    if (ret == -ENOENT) {
hw/scsi/megasas.c-        assert(!ret || ret == -ENOTSUP);
hw/scsi/mptsas.c-        assert(!ret || ret == -ENOTSUP);
hw/usb/hcd-xhci-pci.c-        assert(!ret || ret == -ENOTSUP);
hw/vfio/pci.c-        if (ret == -ENOTSUP) {
nbd/server.c-    } while (ret == -EAGAIN && !client->quiescing);
nbd/server.c-    if (ret == -EAGAIN) {
nbd/server.c-    if (ret == -EIO) {
qemu-img.c-        if (ret == -ENOTSUP) {


I still think, that these are very seldom cases, some of them are just wrong,
some make sense, but their contract may be simplified.

> 
> I'm not going to speculate on relative frequency.
> 
> I much prefer written function contracts.  But if a caller relies on
> negative errno codes, there is an unwritten contract whether we like it
> or not.

Agree.

I just want to say, that usual pattern

int func1(..., Error *errp) {
     ...
     ret = func2(..., Error *errp);
     if (ret < 0) {
         return ret;
     }
     ...
}

is very error-prone, if func1 has some unwritten contract about _different_
errno values. As this unwritten contract may be easily broken somewhere
in the stack, not exactly in func1.


> 
>>> Quick & dirty search without a claim to accuracy or completeness:
>>>       $ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'
> 
> [...]
> 


-- 
Best regards,
Vladimir

Re: [PATCH] migration: vmsd errp handlers: return bool

[Markus Armbruster]

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

> On 20.10.25 19:40, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> On 20.10.25 17:34, Markus Armbruster wrote:
>>>> Daniel P. Berrangé <berrange@redhat.com> writes:

[...]

>>>>> IMHO if a method is using "Error **errp", then it should be considered
>>>>> forbidden to return 'errno' values.
>>>>
>>>> Several subsystems disagree :)
>>>
>>> I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
>>> but blindly follow usual pattern of returning -errno together with
>>> errp, while having no reasonable contract on concrete errno values,
>>> and with this errno finally unused (used only to check, it is it < 0,
>>> like boolean). In other words, the only contract they have is
>>> "< 0 is error, otherwise success".
>>
>> Functions that could just as well return -1 instead of errno exist.
>>
>> Functions that return negative errno with callers that use them also
>> exist.
>
> But do functions that return negative errno together with errp, with
> callers that use this errno exit? I don't ask to find, that's not simple.
> I just say, that I myself don't know any of such functions.
>
>
> upd: I found two!
>
> how:
>
> 1. git grep -A 20 'ret = .*errp)'
> 2. in opened pager, do `/if \(ret == -E`
>
>
> in iommufd_cdev_autodomains_get() we do something just wrong: we clear errp
> after iommufd_cdev_attach_ioas_hwpt(), but return false, which is treated
> as error (but with cleared errp!) by callers...

Returning failure without setting an error is commonly wrong, and when
it's not, it's a bad interface.  However, I can't see how this function
could do that.  Can you enlighten me?

> in qemu_read_default_config_file() we do correct thing, but keeping in mind,
> that it's very seldom practice (around one case), we'd better add a boolean
> parameter to qemu_read_config_file(), and parse errno exactly after call to
> fopen().

In my opinion, this function is just fine.  There are of course other
ways to skin this cat.

> trying with local_err gives a bit more:
>
> git grep -A 20 'ret = .*&\(local_\)\?err)' | grep 'ret == -E'
> block.c-    if (ret == -ENOTSUP) {
> block.c-    if (ret == -EFBIG) {
> block/snapshot.c-    if (ret == -ENOENT || ret == -EINVAL) {
> hw/core/loader-fit.c-    if (ret == -ENOENT) {
> hw/scsi/megasas.c-        assert(!ret || ret == -ENOTSUP);
> hw/scsi/mptsas.c-        assert(!ret || ret == -ENOTSUP);
> hw/usb/hcd-xhci-pci.c-        assert(!ret || ret == -ENOTSUP);
> hw/vfio/pci.c-        if (ret == -ENOTSUP) {
> nbd/server.c-    } while (ret == -EAGAIN && !client->quiescing);
> nbd/server.c-    if (ret == -EAGAIN) {
> nbd/server.c-    if (ret == -EIO) {
> qemu-img.c-        if (ret == -ENOTSUP) {
>
>
> I still think, that these are very seldom cases, some of them are just wrong,
> some make sense, but their contract may be simplified.
>
>> I'm not going to speculate on relative frequency.
>>
>> I much prefer written function contracts.  But if a caller relies on
>> negative errno codes, there is an unwritten contract whether we like it
>> or not.
>
> Agree.
>
> I just want to say, that usual pattern
>
> int func1(..., Error *errp) {
>     ...
>     ret = func2(..., Error *errp);
>     if (ret < 0) {
>         return ret;
>     }
>     ...
> }
>
> is very error-prone, if func1 has some unwritten contract about _different_
> errno values. As this unwritten contract may be easily broken somewhere
> in the stack, not exactly in func1.

I readily concede:

(1) If func() lacks a written contract, passing on func2()'s value makes
the implied contract harder to see.

(2) If func() has a written contract, passing on func2()'s value makes
it harder to verify, and easier to break accidentally.

(3) When no caller needs to discriminate between different errors,
returning -1 or false results in a slightly simpler interface.

Still, has this plagued us in practice?

The only issue in this area that has plagued me enough to remember is
functions returning both -1 and negative errno.  Which works fine as
long as callers only check for negative, but is in my opinion
intolerably sloppy.  Whether the function takes an errp or not doesn't
matter.

I don't think a blanket prohibition of returning negative errno makes
sense.  Discouraging it maybe, but given how rarely it's done, I doubt
it's worth the bother.

Do feel free to send patches to simplify interfaces regardless.

>>>> Quick & dirty search without a claim to accuracy or completeness:
>>>>       $ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'
>>
>> [...]

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 21.10.25 14:36, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 20.10.25 19:40, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>
>>>> On 20.10.25 17:34, Markus Armbruster wrote:
>>>>> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> [...]
> 
>>>>>> IMHO if a method is using "Error **errp", then it should be considered
>>>>>> forbidden to return 'errno' values.
>>>>>
>>>>> Several subsystems disagree :)
>>>>
>>>> I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
>>>> but blindly follow usual pattern of returning -errno together with
>>>> errp, while having no reasonable contract on concrete errno values,
>>>> and with this errno finally unused (used only to check, it is it < 0,
>>>> like boolean). In other words, the only contract they have is
>>>> "< 0 is error, otherwise success".
>>>
>>> Functions that could just as well return -1 instead of errno exist.
>>>
>>> Functions that return negative errno with callers that use them also
>>> exist.
>>
>> But do functions that return negative errno together with errp, with
>> callers that use this errno exit? I don't ask to find, that's not simple.
>> I just say, that I myself don't know any of such functions.
>>
>>
>> upd: I found two!
>>
>> how:
>>
>> 1. git grep -A 20 'ret = .*errp)'
>> 2. in opened pager, do `/if \(ret == -E`
>>
>>
>> in iommufd_cdev_autodomains_get() we do something just wrong: we clear errp
>> after iommufd_cdev_attach_ioas_hwpt(), but return false, which is treated
>> as error (but with cleared errp!) by callers...
> 
> Returning failure without setting an error is commonly wrong, and when
> it's not, it's a bad interface.  However, I can't see how this function
> could do that.  Can you enlighten me?

Oops, I missed "continue;" looking at later "return false;". So this case
is fine too.

> 
>> in qemu_read_default_config_file() we do correct thing, but keeping in mind,
>> that it's very seldom practice (around one case), we'd better add a boolean
>> parameter to qemu_read_config_file(), and parse errno exactly after call to
>> fopen().
> 
> In my opinion, this function is just fine.  There are of course other
> ways to skin this cat.
> 
>> trying with local_err gives a bit more:
>>
>> git grep -A 20 'ret = .*&\(local_\)\?err)' | grep 'ret == -E'
>> block.c-    if (ret == -ENOTSUP) {
>> block.c-    if (ret == -EFBIG) {
>> block/snapshot.c-    if (ret == -ENOENT || ret == -EINVAL) {
>> hw/core/loader-fit.c-    if (ret == -ENOENT) {
>> hw/scsi/megasas.c-        assert(!ret || ret == -ENOTSUP);
>> hw/scsi/mptsas.c-        assert(!ret || ret == -ENOTSUP);
>> hw/usb/hcd-xhci-pci.c-        assert(!ret || ret == -ENOTSUP);
>> hw/vfio/pci.c-        if (ret == -ENOTSUP) {
>> nbd/server.c-    } while (ret == -EAGAIN && !client->quiescing);
>> nbd/server.c-    if (ret == -EAGAIN) {
>> nbd/server.c-    if (ret == -EIO) {
>> qemu-img.c-        if (ret == -ENOTSUP) {
>>
>>
>> I still think, that these are very seldom cases, some of them are just wrong,
>> some make sense, but their contract may be simplified.
>>
>>> I'm not going to speculate on relative frequency.
>>>
>>> I much prefer written function contracts.  But if a caller relies on
>>> negative errno codes, there is an unwritten contract whether we like it
>>> or not.
>>
>> Agree.
>>
>> I just want to say, that usual pattern
>>
>> int func1(..., Error *errp) {
>>      ...
>>      ret = func2(..., Error *errp);
>>      if (ret < 0) {
>>          return ret;
>>      }
>>      ...
>> }
>>
>> is very error-prone, if func1 has some unwritten contract about _different_
>> errno values. As this unwritten contract may be easily broken somewhere
>> in the stack, not exactly in func1.
> 
> I readily concede:
> 
> (1) If func() lacks a written contract, passing on func2()'s value makes
> the implied contract harder to see.
> 
> (2) If func() has a written contract, passing on func2()'s value makes
> it harder to verify, and easier to break accidentally.
> 
> (3) When no caller needs to discriminate between different errors,
> returning -1 or false results in a slightly simpler interface.
> 
> Still, has this plagued us in practice?
> 
> The only issue in this area that has plagued me enough to remember is
> functions returning both -1 and negative errno.  Which works fine as
> long as callers only check for negative, but is in my opinion
> intolerably sloppy.  Whether the function takes an errp or not doesn't
> matter.
> 
> I don't think a blanket prohibition of returning negative errno makes
> sense.  Discouraging it maybe, but given how rarely it's done, I doubt
> it's worth the bother.

Agreed.

> 
> Do feel free to send patches to simplify interfaces regardless.
> 

First is "[PATCH v2 2/2] migration: vmsd errp handlers: return bool" :)
Now I'm fine to reword the commit message in more relaxed attitude
to returning -errno.

-- 
Best regards,
Vladimir

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 20.10.25 14:22, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index d798faeec3..1c2484187f 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -43,8 +43,7 @@
>    *   avoid useless error object creation and destruction.  Note that
>    *   we still have many functions returning void.  We recommend
>    *   • bool-valued functions return true on success / false on failure,
> - *   • pointer-valued functions return non-null / null pointer, and
> - *   • integer-valued functions return non-negative / negative.
> + *   • pointer-valued functions return non-null / null pointer.
>    *
>    * = Creating errors =
>    *
> 

Still, we have functions that may return positive numbers, like
numbers of bytes read. So integer return value makes sense, and
removing this recommendation for integer-valued functions is wrong.

May be adding some additional advice like:

Don't blindly passthorough the -errno together with errp, if you
don't sure it is reasonably used. Prefer boolean for functions
which doesn't need to return some positive integer on success.
Prefer the only -1 error value for integer-valued functions, if
you don't need more different errors.

-- 
Best regards,
Vladimir

Re: [PATCH] migration: vmsd errp handlers: return bool

[Peter Xu]

On Mon, Oct 20, 2025 at 12:19:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -255,13 +252,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          qemu_file_set_error(f, ret);
>          return ret;
>      }
> -    if (vmsd->post_load_errp) {
> -        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);
> -        }
> +    if (vmsd->post_load_errp && !vmsd->post_load_errp(opaque, version_id,

I think this will change the semantics even if I do not expect a real user
to exist.. if post_load_errp() returned true here, then it'll keep looking
for post_load() and execute.

It might still be good to keep the old semantics, so that if
post_load_errp() is provided, then we ignore post_load().  Same to the rest
hooks.

> +                                                      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);
> +        ret = -EINVAL;
>      } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
>          if (ret < 0) {
> @@ -438,9 +434,8 @@ 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);
>          }
> -- 
> 2.48.1
> 

-- 
Peter Xu

Re: [PATCH] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 20.10.25 16:49, Peter Xu wrote:
> On Mon, Oct 20, 2025 at 12:19:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> @@ -255,13 +252,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           qemu_file_set_error(f, ret);
>>           return ret;
>>       }
>> -    if (vmsd->post_load_errp) {
>> -        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);
>> -        }
>> +    if (vmsd->post_load_errp && !vmsd->post_load_errp(opaque, version_id,
> 
> I think this will change the semantics even if I do not expect a real user
> to exist.. if post_load_errp() returned true here, then it'll keep looking
> for post_load() and execute.
> 
> It might still be good to keep the old semantics, so that if
> post_load_errp() is provided, then we ignore post_load().  Same to the rest
> hooks.
> 

Oh, right.

>> +                                                      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);
>> +        ret = -EINVAL;
>>       } else if (vmsd->post_load) {
>>           ret = vmsd->post_load(opaque, version_id);
>>           if (ret < 0) {
>> @@ -438,9 +434,8 @@ 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);
>>           }
>> -- 
>> 2.48.1
>>
> 


-- 
Best regards,
Vladimir