On 20.10.25 14:05, Markus Armbruster wrote:
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.
Do you know an example in code where we have both errno return value
and errp, and the return value make sense and used by callers?
No difference between "documented library function or syscall" and a
function we provide ourselves.
I agree... Still the only difference is that for library function
it's OK to provide specific error only for caller to be able to print it
to the user (with help of strerror). But for our functions with errp,
it's assumed that the whole information for the user is already in errp.
So I now think, shouldn't we actually do
diff --git a/include/qapi/error.h b/include/qapi/error.h
index d798faeec3..1c2484187f 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -43,8 +43,7 @@
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
- * • pointer-valued functions return non-null / null pointer, and
- * • integer-valued functions return non-negative / negative.
+ * • pointer-valued functions return non-null / null pointer.
*
* = Creating errors =
*
?
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 ++++------
[..]
--- 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.
Oh right, thanks.
--
Best regards,
Vladimir
