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?
> 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.
Do feel free to send patches to simplify interfaces regardless.
>>>> 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 }'
>>
>> [...]
