Switch the new API to simple bool-returning interface, as return value
is not used otherwise than check is function failed or not. No logic
depend on concrete errno values.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c   | 10 ++++------
 docs/devel/migration/main.rst |  6 +++---
 include/migration/vmstate.h   |  6 +++---
 migration/vmstate.c           | 14 ++++++--------
 4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index aa69eb606f..6cc9aa199c 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
 
 /*
  * Load the TPM state blobs into the TPM.
- *
- * Returns negative errno codes in case of error.
  */
-static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
+static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
 {
     TPMBackend *tb = opaque;
     int ret;
 
     ret = tpm_emulator_set_state_blobs(tb, errp);
     if (ret < 0) {
-        return ret;
+        return false;
     }
 
     if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
         error_setg(errp, "Failed to resume tpm");
-        return -EIO;
+        return false;
     }
 
-    return 0;
+    return true;
 }
 
 static const VMStateDescription vmstate_tpm_emulator = {
diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 1afe7b9689..234d280249 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
 
 Following are the errp variants of these functions.
 
-- ``int (*pre_load_errp)(void *opaque, Error **errp);``
+- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
 
   This function is called before we load the state of one device.
 
-- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
+- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
 
   This function is called after we load the state of one device.
 
-- ``int (*pre_save_errp)(void *opaque, Error **errp);``
+- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
 
   This function is called before we save the state of one device.
 
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 63ccaee07a..dbe330dd5f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -218,11 +218,11 @@ struct VMStateDescription {
     int minimum_version_id;
     MigrationPriority priority;
     int (*pre_load)(void *opaque);
-    int (*pre_load_errp)(void *opaque, Error **errp);
+    bool (*pre_load_errp)(void *opaque, Error **errp);
     int (*post_load)(void *opaque, int version_id);
-    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
+    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
     int (*pre_save)(void *opaque);
-    int (*pre_save_errp)(void *opaque, Error **errp);
+    bool (*pre_save_errp)(void *opaque, Error **errp);
     int (*post_save)(void *opaque);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 677e56c84a..adaaf91b3f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return -EINVAL;
     }
     if (vmsd->pre_load_errp) {
-        ret = vmsd->pre_load_errp(opaque, errp);
-        if (ret < 0) {
+        if (!vmsd->pre_load_errp(opaque, errp)) {
             error_prepend(errp, "pre load hook failed for: '%s', "
                           "version_id: %d, minimum version_id: %d: ",
                           vmsd->name, vmsd->version_id,
                           vmsd->minimum_version_id);
-            return ret;
+            return -EINVAL;
         }
     } else if (vmsd->pre_load) {
         ret = vmsd->pre_load(opaque);
@@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return ret;
     }
     if (vmsd->post_load_errp) {
-        ret = vmsd->post_load_errp(opaque, version_id, errp);
-        if (ret < 0) {
+        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
             error_prepend(errp, "post load hook failed for: %s, version_id: "
                           "%d, minimum_version: %d: ", vmsd->name,
                           vmsd->version_id, vmsd->minimum_version_id);
+            ret = -EINVAL;
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -438,11 +437,10 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
     trace_vmstate_save_state_top(vmsd->name);
 
     if (vmsd->pre_save_errp) {
-        ret = vmsd->pre_save_errp(opaque, errp);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
-        if (ret < 0) {
+        if (!vmsd->pre_save_errp(opaque, errp)) {
             error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
-            return ret;
+            return -EINVAL;
         }
     } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
-- 
2.48.1

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

> Switch the new API to simple bool-returning interface, as return value
> is not used otherwise than check is function failed or not. No logic
> depend on concrete errno values.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  backends/tpm/tpm_emulator.c   | 10 ++++------
>  docs/devel/migration/main.rst |  6 +++---
>  include/migration/vmstate.h   |  6 +++---
>  migration/vmstate.c           | 14 ++++++--------
>  4 files changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index aa69eb606f..6cc9aa199c 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
>  
>  /*
>   * Load the TPM state blobs into the TPM.
> - *
> - * Returns negative errno codes in case of error.
>   */
> -static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
> +static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>  {
>      TPMBackend *tb = opaque;
>      int ret;
>  
>      ret = tpm_emulator_set_state_blobs(tb, errp);

Note for later: this returns 0 or -EIO.

>      if (ret < 0) {
> -        return ret;
> +        return false;
>      }
>  
>      if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>          error_setg(errp, "Failed to resume tpm");
> -        return -EIO;
> +        return false;
>      }
>  
> -    return 0;
> +    return true;
>  }
>  
>  static const VMStateDescription vmstate_tpm_emulator = {
> diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> index 1afe7b9689..234d280249 100644
> --- a/docs/devel/migration/main.rst
> +++ b/docs/devel/migration/main.rst
> @@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
>  
>  Following are the errp variants of these functions.
>  
> -- ``int (*pre_load_errp)(void *opaque, Error **errp);``
> +- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
>  
>    This function is called before we load the state of one device.
>  
> -- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
> +- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
>  
>    This function is called after we load the state of one device.
>  
> -- ``int (*pre_save_errp)(void *opaque, Error **errp);``
> +- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
>  
>    This function is called before we save the state of one device.
>  
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 63ccaee07a..dbe330dd5f 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -218,11 +218,11 @@ struct VMStateDescription {
>      int minimum_version_id;
>      MigrationPriority priority;
>      int (*pre_load)(void *opaque);
> -    int (*pre_load_errp)(void *opaque, Error **errp);
> +    bool (*pre_load_errp)(void *opaque, Error **errp);
>      int (*post_load)(void *opaque, int version_id);
> -    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> +    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
>      int (*pre_save)(void *opaque);
> -    int (*pre_save_errp)(void *opaque, Error **errp);
> +    bool (*pre_save_errp)(void *opaque, Error **errp);
>      int (*post_save)(void *opaque);
>      bool (*needed)(void *opaque);
>      bool (*dev_unplug_pending)(void *opaque);
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 677e56c84a..adaaf91b3f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          return -EINVAL;
>      }
>      if (vmsd->pre_load_errp) {
> -        ret = vmsd->pre_load_errp(opaque, errp);
> -        if (ret < 0) {
> +        if (!vmsd->pre_load_errp(opaque, errp)) {
>              error_prepend(errp, "pre load hook failed for: '%s', "
>                            "version_id: %d, minimum version_id: %d: ",
>                            vmsd->name, vmsd->version_id,
>                            vmsd->minimum_version_id);
> -            return ret;
> +            return -EINVAL;
>          }
>      } else if (vmsd->pre_load) {
>          ret = vmsd->pre_load(opaque);
> @@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          return ret;
>      }
>      if (vmsd->post_load_errp) {
> -        ret = vmsd->post_load_errp(opaque, version_id, errp);
> -        if (ret < 0) {
> +        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
>              error_prepend(errp, "post load hook failed for: %s, version_id: "
>                            "%d, minimum_version: %d: ", vmsd->name,
>                            vmsd->version_id, vmsd->minimum_version_id);
> +            ret = -EINVAL;

With ->post_load_errp is tpm_emulator_post_load(), the value returned on
error changes from -EIO to -EINVAL.

Do callers of vmstate_load_state() care?

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

On 27.10.25 13:38, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Switch the new API to simple bool-returning interface, as return value
>> is not used otherwise than check is function failed or not. No logic
>> depend on concrete errno values.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   backends/tpm/tpm_emulator.c   | 10 ++++------
>>   docs/devel/migration/main.rst |  6 +++---
>>   include/migration/vmstate.h   |  6 +++---
>>   migration/vmstate.c           | 14 ++++++--------
>>   4 files changed, 16 insertions(+), 20 deletions(-)
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index aa69eb606f..6cc9aa199c 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
>>   
>>   /*
>>    * Load the TPM state blobs into the TPM.
>> - *
>> - * Returns negative errno codes in case of error.
>>    */
>> -static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>> +static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>   {
>>       TPMBackend *tb = opaque;
>>       int ret;
>>   
>>       ret = tpm_emulator_set_state_blobs(tb, errp);
> 
> Note for later: this returns 0 or -EIO.
> 
>>       if (ret < 0) {
>> -        return ret;
>> +        return false;
>>       }
>>   
>>       if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>>           error_setg(errp, "Failed to resume tpm");
>> -        return -EIO;
>> +        return false;
>>       }
>>   
>> -    return 0;
>> +    return true;
>>   }
>>   
>>   static const VMStateDescription vmstate_tpm_emulator = {
>> diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
>> index 1afe7b9689..234d280249 100644
>> --- a/docs/devel/migration/main.rst
>> +++ b/docs/devel/migration/main.rst
>> @@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
>>   
>>   Following are the errp variants of these functions.
>>   
>> -- ``int (*pre_load_errp)(void *opaque, Error **errp);``
>> +- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
>>   
>>     This function is called before we load the state of one device.
>>   
>> -- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
>> +- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
>>   
>>     This function is called after we load the state of one device.
>>   
>> -- ``int (*pre_save_errp)(void *opaque, Error **errp);``
>> +- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
>>   
>>     This function is called before we save the state of one device.
>>   
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 63ccaee07a..dbe330dd5f 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -218,11 +218,11 @@ struct VMStateDescription {
>>       int minimum_version_id;
>>       MigrationPriority priority;
>>       int (*pre_load)(void *opaque);
>> -    int (*pre_load_errp)(void *opaque, Error **errp);
>> +    bool (*pre_load_errp)(void *opaque, Error **errp);
>>       int (*post_load)(void *opaque, int version_id);
>> -    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
>> +    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
>>       int (*pre_save)(void *opaque);
>> -    int (*pre_save_errp)(void *opaque, Error **errp);
>> +    bool (*pre_save_errp)(void *opaque, Error **errp);
>>       int (*post_save)(void *opaque);
>>       bool (*needed)(void *opaque);
>>       bool (*dev_unplug_pending)(void *opaque);
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 677e56c84a..adaaf91b3f 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           return -EINVAL;
>>       }
>>       if (vmsd->pre_load_errp) {
>> -        ret = vmsd->pre_load_errp(opaque, errp);
>> -        if (ret < 0) {
>> +        if (!vmsd->pre_load_errp(opaque, errp)) {
>>               error_prepend(errp, "pre load hook failed for: '%s', "
>>                             "version_id: %d, minimum version_id: %d: ",
>>                             vmsd->name, vmsd->version_id,
>>                             vmsd->minimum_version_id);
>> -            return ret;
>> +            return -EINVAL;
>>           }
>>       } else if (vmsd->pre_load) {
>>           ret = vmsd->pre_load(opaque);
>> @@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>           return ret;
>>       }
>>       if (vmsd->post_load_errp) {
>> -        ret = vmsd->post_load_errp(opaque, version_id, errp);
>> -        if (ret < 0) {
>> +        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
>>               error_prepend(errp, "post load hook failed for: %s, version_id: "
>>                             "%d, minimum_version: %d: ", vmsd->name,
>>                             vmsd->version_id, vmsd->minimum_version_id);
>> +            ret = -EINVAL;
> 
> With ->post_load_errp is tpm_emulator_post_load(), the value returned on
> error changes from -EIO to -EINVAL.
> 
> Do callers of vmstate_load_state() care?

Fast check.. let see somewhere, OK: get_qlist() in vmstate-types.c.. That's a .get
in VMStateInfo structure.

Oh, and we do print this ret the same way:


int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                        void *opaque, int version_id, Error **errp)

  ...

                     ret = inner_field->info->get(f, curr_elem, size,
                                                  inner_field);
                     if (ret < 0) {
                         error_setg(errp,
                                    "Failed to load element of type %s for %s: "
                                    "%d", inner_field->info->name,
                                    inner_field->name, ret);
                     }


Not saying about a lot  of vmstate_load_state() other calls in the code, and callers do
passthrough the error to their callers, and so on. It's a huge work to analyze all of
them.


Hmm now I see that tpm_emulator_post_load() returns only -EIO code on all error paths.

Would it be correct just use -EIO here in my patch instead of -EINVAL? It will save
current behavior as is.



-- 
Best regards,
Vladimir

Hi Vladimir,

On Mon, Oct 27, 2025 at 05:58:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 27.10.25 13:38, Markus Armbruster wrote:
> > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> > 
> > > Switch the new API to simple bool-returning interface, as return value
> > > is not used otherwise than check is function failed or not. No logic
> > > depend on concrete errno values.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >   backends/tpm/tpm_emulator.c   | 10 ++++------
> > >   docs/devel/migration/main.rst |  6 +++---
> > >   include/migration/vmstate.h   |  6 +++---
> > >   migration/vmstate.c           | 14 ++++++--------
> > >   4 files changed, 16 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> > > index aa69eb606f..6cc9aa199c 100644
> > > --- a/backends/tpm/tpm_emulator.c
> > > +++ b/backends/tpm/tpm_emulator.c
> > > @@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
> > >   /*
> > >    * Load the TPM state blobs into the TPM.
> > > - *
> > > - * Returns negative errno codes in case of error.
> > >    */
> > > -static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
> > > +static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
> > >   {
> > >       TPMBackend *tb = opaque;
> > >       int ret;
> > >       ret = tpm_emulator_set_state_blobs(tb, errp);
> > 
> > Note for later: this returns 0 or -EIO.
> > 
> > >       if (ret < 0) {
> > > -        return ret;
> > > +        return false;
> > >       }
> > >       if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
> > >           error_setg(errp, "Failed to resume tpm");
> > > -        return -EIO;
> > > +        return false;
> > >       }
> > > -    return 0;
> > > +    return true;
> > >   }
> > >   static const VMStateDescription vmstate_tpm_emulator = {
> > > diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
> > > index 1afe7b9689..234d280249 100644
> > > --- a/docs/devel/migration/main.rst
> > > +++ b/docs/devel/migration/main.rst
> > > @@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
> > >   Following are the errp variants of these functions.
> > > -- ``int (*pre_load_errp)(void *opaque, Error **errp);``
> > > +- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
> > >     This function is called before we load the state of one device.
> > > -- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
> > > +- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
> > >     This function is called after we load the state of one device.
> > > -- ``int (*pre_save_errp)(void *opaque, Error **errp);``
> > > +- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
> > >     This function is called before we save the state of one device.
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index 63ccaee07a..dbe330dd5f 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -218,11 +218,11 @@ struct VMStateDescription {
> > >       int minimum_version_id;
> > >       MigrationPriority priority;
> > >       int (*pre_load)(void *opaque);
> > > -    int (*pre_load_errp)(void *opaque, Error **errp);
> > > +    bool (*pre_load_errp)(void *opaque, Error **errp);
> > >       int (*post_load)(void *opaque, int version_id);
> > > -    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
> > > +    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
> > >       int (*pre_save)(void *opaque);
> > > -    int (*pre_save_errp)(void *opaque, Error **errp);
> > > +    bool (*pre_save_errp)(void *opaque, Error **errp);
> > >       int (*post_save)(void *opaque);
> > >       bool (*needed)(void *opaque);
> > >       bool (*dev_unplug_pending)(void *opaque);
> > > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > > index 677e56c84a..adaaf91b3f 100644
> > > --- a/migration/vmstate.c
> > > +++ b/migration/vmstate.c
> > > @@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > >           return -EINVAL;
> > >       }
> > >       if (vmsd->pre_load_errp) {
> > > -        ret = vmsd->pre_load_errp(opaque, errp);
> > > -        if (ret < 0) {
> > > +        if (!vmsd->pre_load_errp(opaque, errp)) {
> > >               error_prepend(errp, "pre load hook failed for: '%s', "
> > >                             "version_id: %d, minimum version_id: %d: ",
> > >                             vmsd->name, vmsd->version_id,
> > >                             vmsd->minimum_version_id);
> > > -            return ret;
> > > +            return -EINVAL;
> > >           }
> > >       } else if (vmsd->pre_load) {
> > >           ret = vmsd->pre_load(opaque);
> > > @@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > >           return ret;
> > >       }
> > >       if (vmsd->post_load_errp) {
> > > -        ret = vmsd->post_load_errp(opaque, version_id, errp);
> > > -        if (ret < 0) {
> > > +        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
> > >               error_prepend(errp, "post load hook failed for: %s, version_id: "
> > >                             "%d, minimum_version: %d: ", vmsd->name,
> > >                             vmsd->version_id, vmsd->minimum_version_id);
> > > +            ret = -EINVAL;
> > 
> > With ->post_load_errp is tpm_emulator_post_load(), the value returned on
> > error changes from -EIO to -EINVAL.
> > 
> > Do callers of vmstate_load_state() care?
> 
> Fast check.. let see somewhere, OK: get_qlist() in vmstate-types.c.. That's a .get
> in VMStateInfo structure.
> 
> Oh, and we do print this ret the same way:
> 
> 
> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                        void *opaque, int version_id, Error **errp)
> 
>  ...
> 
>                     ret = inner_field->info->get(f, curr_elem, size,
>                                                  inner_field);
>                     if (ret < 0) {
>                         error_setg(errp,
>                                    "Failed to load element of type %s for %s: "
>                                    "%d", inner_field->info->name,
>                                    inner_field->name, ret);
>                     }
> 
> 
> Not saying about a lot  of vmstate_load_state() other calls in the code, and callers do
> passthrough the error to their callers, and so on. It's a huge work to analyze all of
> them.
> 
> 
> Hmm now I see that tpm_emulator_post_load() returns only -EIO code on all error paths.
> 
> Would it be correct just use -EIO here in my patch instead of -EINVAL? It will save
> current behavior as is.

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

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

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

Regards,
Arun Menon

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

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

-- 
Best regards,
Vladimir

Hi Vladimir,

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

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

> 
> -- 
> Best regards,
> Vladimir
> 

Regards,
Arun Menon

On 27.10.25 19:30, Arun Menon wrote:
> Hi Vladimir,
> 
> On Mon, Oct 27, 2025 at 05:58:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 27.10.25 13:38, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>>
>>>> Switch the new API to simple bool-returning interface, as return value
>>>> is not used otherwise than check is function failed or not. No logic
>>>> depend on concrete errno values.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>>    backends/tpm/tpm_emulator.c   | 10 ++++------
>>>>    docs/devel/migration/main.rst |  6 +++---
>>>>    include/migration/vmstate.h   |  6 +++---
>>>>    migration/vmstate.c           | 14 ++++++--------
>>>>    4 files changed, 16 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>>>> index aa69eb606f..6cc9aa199c 100644
>>>> --- a/backends/tpm/tpm_emulator.c
>>>> +++ b/backends/tpm/tpm_emulator.c
>>>> @@ -947,25 +947,23 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
>>>>    /*
>>>>     * Load the TPM state blobs into the TPM.
>>>> - *
>>>> - * Returns negative errno codes in case of error.
>>>>     */
>>>> -static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>>> +static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>>>    {
>>>>        TPMBackend *tb = opaque;
>>>>        int ret;
>>>>        ret = tpm_emulator_set_state_blobs(tb, errp);
>>>
>>> Note for later: this returns 0 or -EIO.
>>>
>>>>        if (ret < 0) {
>>>> -        return ret;
>>>> +        return false;
>>>>        }
>>>>        if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>>>>            error_setg(errp, "Failed to resume tpm");
>>>> -        return -EIO;
>>>> +        return false;
>>>>        }
>>>> -    return 0;
>>>> +    return true;
>>>>    }
>>>>    static const VMStateDescription vmstate_tpm_emulator = {
>>>> diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
>>>> index 1afe7b9689..234d280249 100644
>>>> --- a/docs/devel/migration/main.rst
>>>> +++ b/docs/devel/migration/main.rst
>>>> @@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
>>>>    Following are the errp variants of these functions.
>>>> -- ``int (*pre_load_errp)(void *opaque, Error **errp);``
>>>> +- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
>>>>      This function is called before we load the state of one device.
>>>> -- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
>>>> +- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
>>>>      This function is called after we load the state of one device.
>>>> -- ``int (*pre_save_errp)(void *opaque, Error **errp);``
>>>> +- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
>>>>      This function is called before we save the state of one device.
>>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>>>> index 63ccaee07a..dbe330dd5f 100644
>>>> --- a/include/migration/vmstate.h
>>>> +++ b/include/migration/vmstate.h
>>>> @@ -218,11 +218,11 @@ struct VMStateDescription {
>>>>        int minimum_version_id;
>>>>        MigrationPriority priority;
>>>>        int (*pre_load)(void *opaque);
>>>> -    int (*pre_load_errp)(void *opaque, Error **errp);
>>>> +    bool (*pre_load_errp)(void *opaque, Error **errp);
>>>>        int (*post_load)(void *opaque, int version_id);
>>>> -    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
>>>> +    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
>>>>        int (*pre_save)(void *opaque);
>>>> -    int (*pre_save_errp)(void *opaque, Error **errp);
>>>> +    bool (*pre_save_errp)(void *opaque, Error **errp);
>>>>        int (*post_save)(void *opaque);
>>>>        bool (*needed)(void *opaque);
>>>>        bool (*dev_unplug_pending)(void *opaque);
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 677e56c84a..adaaf91b3f 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>            return -EINVAL;
>>>>        }
>>>>        if (vmsd->pre_load_errp) {
>>>> -        ret = vmsd->pre_load_errp(opaque, errp);
>>>> -        if (ret < 0) {
>>>> +        if (!vmsd->pre_load_errp(opaque, errp)) {
>>>>                error_prepend(errp, "pre load hook failed for: '%s', "
>>>>                              "version_id: %d, minimum version_id: %d: ",
>>>>                              vmsd->name, vmsd->version_id,
>>>>                              vmsd->minimum_version_id);
>>>> -            return ret;
>>>> +            return -EINVAL;
>>>>            }
>>>>        } else if (vmsd->pre_load) {
>>>>            ret = vmsd->pre_load(opaque);
>>>> @@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>            return ret;
>>>>        }
>>>>        if (vmsd->post_load_errp) {
>>>> -        ret = vmsd->post_load_errp(opaque, version_id, errp);
>>>> -        if (ret < 0) {
>>>> +        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
>>>>                error_prepend(errp, "post load hook failed for: %s, version_id: "
>>>>                              "%d, minimum_version: %d: ", vmsd->name,
>>>>                              vmsd->version_id, vmsd->minimum_version_id);
>>>> +            ret = -EINVAL;
>>>
>>> With ->post_load_errp is tpm_emulator_post_load(), the value returned on
>>> error changes from -EIO to -EINVAL.
>>>
>>> Do callers of vmstate_load_state() care?
>>
>> Fast check.. let see somewhere, OK: get_qlist() in vmstate-types.c.. That's a .get
>> in VMStateInfo structure.
>>
>> Oh, and we do print this ret the same way:
>>
>>
>> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>                         void *opaque, int version_id, Error **errp)
>>
>>   ...
>>
>>                      ret = inner_field->info->get(f, curr_elem, size,
>>                                                   inner_field);
>>                      if (ret < 0) {
>>                          error_setg(errp,
>>                                     "Failed to load element of type %s for %s: "
>>                                     "%d", inner_field->info->name,
>>                                     inner_field->name, ret);
>>                      }
>>
>>
>> Not saying about a lot  of vmstate_load_state() other calls in the code, and callers do
>> passthrough the error to their callers, and so on. It's a huge work to analyze all of
>> them.
>>
>>
>> Hmm now I see that tpm_emulator_post_load() returns only -EIO code on all error paths.
>>
>> Would it be correct just use -EIO here in my patch instead of -EINVAL? It will save
>> current behavior as is.
> 
> I admit I too was not sure whether to use int or bool return type.
> A bit of the discussion is here: https://lore.kernel.org/qemu-devel/87v7mbsjb0.fsf@pond.sub.org/
> I found few places where a genuine error code was returned from the function
> For example:
> vmbus_post_load() calls vmbus_init() that returns -ENOMEM on failure.
> 
> The intention was to eventually phase out the old implementation and replace them
> with the new ones (errp)
> We wanted the new errp variants to be structurally as close to the old
> ones as possible so that we can perform a bulk change later.
> 

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

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

Reasonable. Thanks for the explanation.

-- 
Best regards,
Vladimir

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

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

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

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

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

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

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

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

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

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

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

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

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

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

More get() users.  Same.

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

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

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

This is the migration core invokations.  Safe.

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

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

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

Same migration core invokations. Safe.

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

Test code only.  Safe.

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

Yet another get().  Safe.

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

-- 
Peter Xu

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

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

-- 
Best regards,
Vladimir

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

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

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

Thanks,

-- 
Peter Xu