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 <[email protected]> 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 <[email protected]> > > > --- > > > 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/[email protected]/ 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
