On 21.10.25 14:36, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
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:
[...]
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...
Returning failure without setting an error is commonly wrong, and when
it's not, it's a bad interface. However, I can't see how this function
could do that. Can you enlighten me?
Oops, I missed "continue;" looking at later "return false;". So this case
is fine too.
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().
In my opinion, this function is just fine. There are of course other
ways to skin this cat.
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.
I readily concede:
(1) If func() lacks a written contract, passing on func2()'s value makes
the implied contract harder to see.
(2) If func() has a written contract, passing on func2()'s value makes
it harder to verify, and easier to break accidentally.
(3) When no caller needs to discriminate between different errors,
returning -1 or false results in a slightly simpler interface.
Still, has this plagued us in practice?
The only issue in this area that has plagued me enough to remember is
functions returning both -1 and negative errno. Which works fine as
long as callers only check for negative, but is in my opinion
intolerably sloppy. Whether the function takes an errp or not doesn't
matter.
I don't think a blanket prohibition of returning negative errno makes
sense. Discouraging it maybe, but given how rarely it's done, I doubt
it's worth the bother.
Agreed.
Do feel free to send patches to simplify interfaces regardless.
First is "[PATCH v2 2/2] migration: vmsd errp handlers: return bool" :)
Now I'm fine to reword the commit message in more relaxed attitude
to returning -errno.
--
Best regards,
Vladimir
