On 20.10.25 19:40, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
On 20.10.25 17:34, Markus Armbruster wrote:
Daniel P. Berrangé <[email protected]> writes:
On Mon, Oct 20, 2025 at 02:22:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
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?
If there are examples of that, I would generally consider them to be
bugs.
IMHO if a method is using "Error **errp", then it should be considered
forbidden to return 'errno' values.
Several subsystems disagree :)
I'd vote, that in 99% (or more) cases, they don't reasonably disagree,
but blindly follow usual pattern of returning -errno together with
errp, while having no reasonable contract on concrete errno values,
and with this errno finally unused (used only to check, it is it < 0,
like boolean). In other words, the only contract they have is
"< 0 is error, otherwise success".
Functions that could just as well return -1 instead of errno exist.
Functions that return negative errno with callers that use them also
exist.
But do functions that return negative errno together with errp, with
callers that use this errno exit? I don't ask to find, that's not simple.
I just say, that I myself don't know any of such functions.
upd: I found two!
how:
1. git grep -A 20 'ret = .*errp)'
2. in opened pager, do `/if \(ret == -E`
in iommufd_cdev_autodomains_get() we do something just wrong: we clear errp
after iommufd_cdev_attach_ioas_hwpt(), but return false, which is treated
as error (but with cleared errp!) by callers...
in qemu_read_default_config_file() we do correct thing, but keeping in mind,
that it's very seldom practice (around one case), we'd better add a boolean
parameter to qemu_read_config_file(), and parse errno exactly after call to
fopen().
trying with local_err gives a bit more:
git grep -A 20 'ret = .*&\(local_\)\?err)' | grep 'ret == -E'
block.c- if (ret == -ENOTSUP) {
block.c- if (ret == -EFBIG) {
block/snapshot.c- if (ret == -ENOENT || ret == -EINVAL) {
hw/core/loader-fit.c- if (ret == -ENOENT) {
hw/scsi/megasas.c- assert(!ret || ret == -ENOTSUP);
hw/scsi/mptsas.c- assert(!ret || ret == -ENOTSUP);
hw/usb/hcd-xhci-pci.c- assert(!ret || ret == -ENOTSUP);
hw/vfio/pci.c- if (ret == -ENOTSUP) {
nbd/server.c- } while (ret == -EAGAIN && !client->quiescing);
nbd/server.c- if (ret == -EAGAIN) {
nbd/server.c- if (ret == -EIO) {
qemu-img.c- if (ret == -ENOTSUP) {
I still think, that these are very seldom cases, some of them are just wrong,
some make sense, but their contract may be simplified.
I'm not going to speculate on relative frequency.
I much prefer written function contracts. But if a caller relies on
negative errno codes, there is an unwritten contract whether we like it
or not.
Agree.
I just want to say, that usual pattern
int func1(..., Error *errp) {
...
ret = func2(..., Error *errp);
if (ret < 0) {
return ret;
}
...
}
is very error-prone, if func1 has some unwritten contract about _different_
errno values. As this unwritten contract may be easily broken somewhere
in the stack, not exactly in func1.
Quick & dirty search without a claim to accuracy or completeness:
$ git-ls-files \*.[ch] | xargs awk '/, Error \*\*errp/ { on=1 } on && /return -E/ { print
FILENAME ":" FNR ":" $0 } /^}/ { on=0 }'
[...]
--
Best regards,
Vladimir
