Peter Xu <pet...@redhat.com> writes:
> On Thu, Oct 05, 2023 at 07:45:00AM +0200, Markus Armbruster wrote:
>> Peter Xu <pet...@redhat.com> writes:
>>
>> > Sorry Zhijian, I missed this email.
>> >
>> > On Wed, Oct 04, 2023 at 06:32:14PM +0200, Juan Quintela wrote:
>> >> > * Avoid non-negative integer error values.
>> >
>> > Perhaps we need to forbid that if doing this.
>> >
>> > I can see Zhijian's point, where "if (ret)" can also capture unexpected
>> > positive returns, while "if (ret < 0)" is not clear on who's handling ret>0
>> > case. Personally I like that, too.
>>
>> It's clear either way :)
>>
>> The problem is calling a function whose contract specifies "return 0 on
>> success, negative value on failure".
>>
>> If it returns positive value, the contract is broken, and all bets are
>> off.
>>
>> If you check the return value like
>>
>> if (ret < 0) {
>> ... handle error and fail ...
>> }
>> ... carry on ...
>>
>> then an unexpected positive value will clearly be treated as success.
>>
>> If you check it like
>>
>> if (ret) {
>> ... handle error and fail ...
>> }
>> ... carry on ...
>>
>> then it will clearly be treated as failure.
>>
>> But we don't know what it is! Treating it as success can be wrong,
>> treating it as failure can be just as wrong.
>
> Right, IMHO the major difference is when there's a bug in the retval
> protocl of the API we're invoking.
>
> With "if (ret)" we capture that protocol bug, treating it as a failure (of
> that buggy API). With "if (ret<0)" we don't yet capture it, either
> everything will just keep working, or something weird happens later. Not
> so predictable in this case.
I don't think misinterpreting a violation of the contract as failure is
safer than misinterpreting it as success.
Where we have reason to worry about contract violations, we should
assert() it's not violated.
