Re: [PATCH 28/52] migration/rdma: Check negative error values the same way everywhere

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Oct 05, 2023 at 07:45:00AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@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.