* [PATCH v2 0/2] migration: vmsd errp handlers: return bool @ 2025-10-20 16:03 Vladimir Sementsov-Ogievskiy 2025-10-20 16:03 ` [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path Vladimir Sementsov-Ogievskiy 2025-10-20 16:03 ` [PATCH v2 2/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 0 siblings, 2 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-20 16:03 UTC (permalink / raw) To: peterx Cc: stefanb, farosas, qemu-devel, armbru, berrange, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path 2025-10-20 16:03 [PATCH v2 0/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy @ 2025-10-20 16:03 ` Vladimir Sementsov-Ogievskiy 2025-10-20 17:57 ` Philippe Mathieu-Daudé 2025-10-20 16:03 ` [PATCH v2 2/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-20 16:03 UTC (permalink / raw) To: peterx Cc: stefanb, farosas, qemu-devel, armbru, berrange, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path 2025-10-20 16:03 ` [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path Vladimir Sementsov-Ogievskiy @ 2025-10-20 17:57 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 8+ messages in thread From: Philippe Mathieu-Daudé @ 2025-10-20 17:57 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, peterx Cc: stefanb, farosas, qemu-devel, armbru, berrange 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] migration: vmsd errp handlers: return bool 2025-10-20 16:03 [PATCH v2 0/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 2025-10-20 16:03 ` [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path Vladimir Sementsov-Ogievskiy @ 2025-10-20 16:03 ` Vladimir Sementsov-Ogievskiy 2025-10-21 11:50 ` Vladimir Sementsov-Ogievskiy 2025-10-25 19:50 ` Vladimir Sementsov-Ogievskiy 1 sibling, 2 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-20 16:03 UTC (permalink / raw) To: peterx Cc: stefanb, farosas, qemu-devel, armbru, berrange, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool 2025-10-20 16:03 ` [PATCH v2 2/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy @ 2025-10-21 11:50 ` Vladimir Sementsov-Ogievskiy 2025-10-22 6:14 ` Markus Armbruster 2025-10-25 19:50 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-21 11:50 UTC (permalink / raw) To: peterx; +Cc: stefanb, farosas, qemu-devel, armbru, berrange 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool 2025-10-21 11:50 ` Vladimir Sementsov-Ogievskiy @ 2025-10-22 6:14 ` Markus Armbruster 2025-10-22 6:58 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2025-10-22 6:14 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: peterx, stefanb, farosas, qemu-devel, armbru, berrange 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool 2025-10-22 6:14 ` Markus Armbruster @ 2025-10-22 6:58 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-22 6:58 UTC (permalink / raw) To: Markus Armbruster; +Cc: peterx, stefanb, farosas, qemu-devel, berrange 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] migration: vmsd errp handlers: return bool 2025-10-20 16:03 ` [PATCH v2 2/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 2025-10-21 11:50 ` Vladimir Sementsov-Ogievskiy @ 2025-10-25 19:50 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 8+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2025-10-25 19:50 UTC (permalink / raw) To: peterx; +Cc: stefanb, farosas, qemu-devel, armbru, berrange 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-10-25 19:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-20 16:03 [PATCH v2 0/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 2025-10-20 16:03 ` [PATCH v2 1/2] migration: vmstate_save_state_v(): fix error path Vladimir Sementsov-Ogievskiy 2025-10-20 17:57 ` Philippe Mathieu-Daudé 2025-10-20 16:03 ` [PATCH v2 2/2] migration: vmsd errp handlers: return bool Vladimir Sementsov-Ogievskiy 2025-10-21 11:50 ` Vladimir Sementsov-Ogievskiy 2025-10-22 6:14 ` Markus Armbruster 2025-10-22 6:58 ` Vladimir Sementsov-Ogievskiy 2025-10-25 19:50 ` Vladimir Sementsov-Ogievskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).