1. Patchew
  2. QEMU
  3. [PATCH v3 0/4] migration: vmsd errp handlers: return bool
  4. View patch

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

Vladimir Sementsov-Ogievskiy posted 4 patches 3 months, 2 weeks ago
Maintainers: Stefan Berger <stefanb@linux.vnet.ibm.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
There is a newer version of this series
[PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs
Posted by Vladimir Sementsov-Ogievskiy 3 months, 2 weeks ago 
First, the handlers should put all needed information into errp,
we should not append error number here.

Second, the only realization of new _errp API is
tpm_emulator_post_load(), which on some failure paths returns
-errno, but on the others simply -1. So printing this additional
number may be misleading. tpm_emulator.c needs a lot more work
to report good error message on all error paths.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/vmstate.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index fd066f910e..677e56c84a 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_load_errp(opaque, errp);
         if (ret < 0) {
             error_prepend(errp, "pre load hook failed for: '%s', "
-                          "version_id: %d, minimum version_id: %d, "
-                          "ret: %d: ", vmsd->name, vmsd->version_id,
-                          vmsd->minimum_version_id, ret);
+                          "version_id: %d, minimum version_id: %d: ",
+                          vmsd->name, vmsd->version_id,
+                          vmsd->minimum_version_id);
             return ret;
         }
     } else if (vmsd->pre_load) {
@@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->post_load_errp(opaque, version_id, errp);
         if (ret < 0) {
             error_prepend(errp, "post load hook failed for: %s, version_id: "
-                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
-                          vmsd->version_id, vmsd->minimum_version_id, ret);
+                          "%d, minimum_version: %d: ", vmsd->name,
+                          vmsd->version_id, vmsd->minimum_version_id);
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_save_errp(opaque, errp);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret < 0) {
-            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
-                          vmsd->name, ret);
+            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
             return ret;
         }
     } else if (vmsd->pre_save) {
-- 
2.48.1
Re: [PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs
Posted by Stefan Berger 3 months, 2 weeks ago 

On 10/25/25 4:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> First, the handlers should put all needed information into errp,
> we should not append error number here.
> 
> Second, the only realization of new _errp API is
> tpm_emulator_post_load(), which on some failure paths returns
> -errno, but on the others simply -1. So printing this additional
> number may be misleading. tpm_emulator.c needs a lot more work
> to report good error message on all error paths.

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

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   migration/vmstate.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fd066f910e..677e56c84a 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>           ret = vmsd->pre_load_errp(opaque, errp);
>           if (ret < 0) {
>               error_prepend(errp, "pre load hook failed for: '%s', "
> -                          "version_id: %d, minimum version_id: %d, "
> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
> -                          vmsd->minimum_version_id, ret);
> +                          "version_id: %d, minimum version_id: %d: ",
> +                          vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id);
>               return ret;
>           }
>       } else if (vmsd->pre_load) {
> @@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>           ret = vmsd->post_load_errp(opaque, version_id, errp);
>           if (ret < 0) {
>               error_prepend(errp, "post load hook failed for: %s, version_id: "
> -                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
> -                          vmsd->version_id, vmsd->minimum_version_id, ret);
> +                          "%d, minimum_version: %d: ", vmsd->name,
> +                          vmsd->version_id, vmsd->minimum_version_id);
>           }
>       } else if (vmsd->post_load) {
>           ret = vmsd->post_load(opaque, version_id);
> @@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>           ret = vmsd->pre_save_errp(opaque, errp);
>           trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>           if (ret < 0) {
> -            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
> -                          vmsd->name, ret);
> +            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>               return ret;
>           }
>       } else if (vmsd->pre_save) {
Re: [PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs
Posted by Vladimir Sementsov-Ogievskiy 3 months, 2 weeks ago 
On 27.10.25 17:38, Stefan Berger wrote:
> 
> 
> On 10/25/25 4:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> First, the handlers should put all needed information into errp,
>> we should not append error number here.
>>
>> Second, the only realization of new _errp API is
>> tpm_emulator_post_load(), which on some failure paths returns
>> -errno, but on the others simply -1. So printing this additional
>> number may be misleading. tpm_emulator.c needs a lot more work
>> to report good error message on all error paths.
> 
> You mean in all paths or just the .post_load_errp path? In the .post_load_errp cases -EIO is currently always returned if called functions failed and return a -1. So their -1 does not propagate.


Oops, excuse me, I was wrong.

Now I see that tpm_emulator_post_load return either 0 or -EIO on all paths.


> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   migration/vmstate.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index fd066f910e..677e56c84a 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->pre_load_errp(opaque, errp);
>>           if (ret < 0) {
>>               error_prepend(errp, "pre load hook failed for: '%s', "
>> -                          "version_id: %d, minimum version_id: %d, "
>> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
>> -                          vmsd->minimum_version_id, ret);
>> +                          "version_id: %d, minimum version_id: %d: ",
>> +                          vmsd->name, vmsd->version_id,
>> +                          vmsd->minimum_version_id);
>>               return ret;
>>           }
>>       } else if (vmsd->pre_load) {
>> @@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->post_load_errp(opaque, version_id, errp);
>>           if (ret < 0) {
>>               error_prepend(errp, "post load hook failed for: %s, version_id: "
>> -                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
>> -                          vmsd->version_id, vmsd->minimum_version_id, ret);
>> +                          "%d, minimum_version: %d: ", vmsd->name,
>> +                          vmsd->version_id, vmsd->minimum_version_id);
>>           }
>>       } else if (vmsd->post_load) {
>>           ret = vmsd->post_load(opaque, version_id);
>> @@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->pre_save_errp(opaque, errp);
>>           trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>>           if (ret < 0) {
>> -            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
>> -                          vmsd->name, ret);
>> +            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>>               return ret;
>>           }
>>       } else if (vmsd->pre_save) {
> 


-- 
Best regards,
Vladimir
Re: [PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs
Posted by Markus Armbruster 3 months, 2 weeks ago 
What do you mean by "new _errp APIs"?  Is it the pre_load_errp(),
post_load_errp(), pre_save_errp() callbacks?

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

> First, the handlers should put all needed information into errp,
> we should not append error number here.
>
> Second, the only realization of new _errp API is
> tpm_emulator_post_load(), which on some failure paths returns
> -errno, but on the others simply -1. So printing this additional
> number may be misleading. tpm_emulator.c needs a lot more work
> to report good error message on all error paths.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/vmstate.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fd066f910e..677e56c84a 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->pre_load_errp(opaque, errp);
>          if (ret < 0) {
>              error_prepend(errp, "pre load hook failed for: '%s', "
> -                          "version_id: %d, minimum version_id: %d, "
> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
> -                          vmsd->minimum_version_id, ret);
> +                          "version_id: %d, minimum version_id: %d: ",
> +                          vmsd->name, vmsd->version_id,
> +                          vmsd->minimum_version_id);
>              return ret;
>          }
>      } else if (vmsd->pre_load) {
> @@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->post_load_errp(opaque, version_id, errp);
>          if (ret < 0) {
>              error_prepend(errp, "post load hook failed for: %s, version_id: "
> -                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
> -                          vmsd->version_id, vmsd->minimum_version_id, ret);
> +                          "%d, minimum_version: %d: ", vmsd->name,
> +                          vmsd->version_id, vmsd->minimum_version_id);
>          }
>      } else if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
> @@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->pre_save_errp(opaque, errp);
>          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>          if (ret < 0) {
> -            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
> -                          vmsd->name, ret);
> +            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>              return ret;
>          }
>      } else if (vmsd->pre_save) {

All good.  However, there are more error messages with numeric error
codes in this file.  I figure you're leaving them for another day.
That's okay, but I'd suggest to mention this in your commit message.
Re: [PATCH v3 3/4] migration/vmstate: stop reporting error number for new _errp APIs
Posted by Vladimir Sementsov-Ogievskiy 3 months, 2 weeks ago 
On 27.10.25 13:23, Markus Armbruster wrote:
> What do you mean by "new _errp APIs"?  Is it the pre_load_errp(),
> post_load_errp(), pre_save_errp() callbacks?

Yes

> 
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> First, the handlers should put all needed information into errp,
>> we should not append error number here.
>>
>> Second, the only realization of new _errp API is
>> tpm_emulator_post_load(), which on some failure paths returns
>> -errno, but on the others simply -1. So printing this additional
>> number may be misleading. tpm_emulator.c needs a lot more work
>> to report good error message on all error paths.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   migration/vmstate.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index fd066f910e..677e56c84a 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->pre_load_errp(opaque, errp);
>>           if (ret < 0) {
>>               error_prepend(errp, "pre load hook failed for: '%s', "
>> -                          "version_id: %d, minimum version_id: %d, "
>> -                          "ret: %d: ", vmsd->name, vmsd->version_id,
>> -                          vmsd->minimum_version_id, ret);
>> +                          "version_id: %d, minimum version_id: %d: ",
>> +                          vmsd->name, vmsd->version_id,
>> +                          vmsd->minimum_version_id);
>>               return ret;
>>           }
>>       } else if (vmsd->pre_load) {
>> @@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->post_load_errp(opaque, version_id, errp);
>>           if (ret < 0) {
>>               error_prepend(errp, "post load hook failed for: %s, version_id: "
>> -                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
>> -                          vmsd->version_id, vmsd->minimum_version_id, ret);
>> +                          "%d, minimum_version: %d: ", vmsd->name,
>> +                          vmsd->version_id, vmsd->minimum_version_id);
>>           }
>>       } else if (vmsd->post_load) {
>>           ret = vmsd->post_load(opaque, version_id);
>> @@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>>           ret = vmsd->pre_save_errp(opaque, errp);
>>           trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>>           if (ret < 0) {
>> -            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
>> -                          vmsd->name, ret);
>> +            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
>>               return ret;
>>           }
>>       } else if (vmsd->pre_save) {
> 
> All good.  However, there are more error messages with numeric error
> codes in this file.  I figure you're leaving them for another day.
> That's okay, but I'd suggest to mention this in your commit message.
> 

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

-- 
Best regards,
Vladimir

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.