Re: [PATCH v3 4/4] migration: vmsd errp handlers: return bool

[Arun Menon <armenon@redhat.com>]

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