[PATCH v2 0/2] migration: vmsd errp handlers: return bool

[PATCH v2 0/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

v2:
01: new, fix bug
02: fix mistakes of v1

Vladimir Sementsov-Ogievskiy (2):
  migration: vmstate_save_state_v(): fix error path
  migration: vmsd errp handlers: return bool

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

-- 
2.48.1

[PATCH v2 1/2] 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>
---
 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 v2 2/2] 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           | 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

Re: [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path

[Philippe Mathieu-Daudé]

On 20/10/25 18:03, 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>
> ---
>   migration/vmstate.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool

[Markus Armbruster]

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.

Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

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