Vladimir Sementsov-Ogievskiy <[email protected]> writes: > 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().
It's a matter of the function's contract, actually. If the contract is "Return negative value on failure", checking for failure is all you can do with it. Same information as "Return false on failure". If the contract is "Return negative errno on failure", the function is responsible for returning values that make sense. Ideally, the contract spells them all out. No difference between "documented library function or syscall" and a function we provide ourselves. > 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 When no caller actually needs to distinguish between errors, bool is the most obvious interface. > 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 | 31 +++++++++++++------------------ > 4 files changed, 23 insertions(+), 30 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 81eadde553..026fd6f1a4 100644 > --- a/migration/vmstate.c > +++ b/migration/vmstate.c > @@ -153,15 +153,12 @@ int vmstate_load_state(QEMUFile *f, const > VMStateDescription *vmsd, > trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL); > return -EINVAL; > } > - if (vmsd->pre_load_errp) { > - ret = vmsd->pre_load_errp(opaque, errp); > - if (ret < 0) { > - error_prepend(errp, "pre load hook failed for: '%s', " > - "version_id: %d, minimum version_id: %d, " > - "ret: %d: ", vmsd->name, vmsd->version_id, > - vmsd->minimum_version_id, ret); Numeric errno codes in error messages are an anti-pattern. > - return ret; > - } > + if (vmsd->pre_load_errp && !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); Did you forget to delete ", ret %d:" and its argument @ret? It's always zero here now. > + return -EINVAL; > } else if (vmsd->pre_load) { > ret = vmsd->pre_load(opaque); > if (ret) { > @@ -255,13 +252,12 @@ int vmstate_load_state(QEMUFile *f, const > VMStateDescription *vmsd, > qemu_file_set_error(f, ret); > return ret; > } > - if (vmsd->post_load_errp) { > - ret = vmsd->post_load_errp(opaque, version_id, errp); > - if (ret < 0) { > - error_prepend(errp, "post load hook failed for: %s, version_id: " > - "%d, minimum_version: %d, ret: %d: ", vmsd->name, > - vmsd->version_id, vmsd->minimum_version_id, ret); > - } > + if (vmsd->post_load_errp && !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); Likewise. > + ret = -EINVAL; > } else if (vmsd->post_load) { > ret = vmsd->post_load(opaque, version_id); > if (ret < 0) { > @@ -438,9 +434,8 @@ 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); > } Likewise.
