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

[Peter Xu]

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.

Thanks,

-- 
Peter Xu