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

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
>
>> On 18/09/2023 22:41, Markus Armbruster wrote:
>>> When a function returns 0 on success, negative value on error,
>>> checking for non-zero suffices, but checking for negative is clearer.
>>> So do that.
>>> 
>>
>> This patch is no my favor convention.
>
> Certainly a matter of taste, which means maintainers get to decide, not
> me.
>
> Failure checks can be confusing in C.  Is
>
>     if (foo(...))
>
> checking for success or for failure?  Impossible to tell.  If foo()
> returns a pointer, it almost certainly checks for success.  If it
> returns bool, likewise.  But if it returns an integer, it probably
> checks for failure.
>
> Getting a condition backwards is a common coding mistake.  Consider
> patch review of
>
>     if (condition) {
>         obviously the error case
>     }
>
> Patch review is more likely to catch a backward condition when the
> condition's sense is locally obvious.
>
> Conventions can help.  Here's the one I like:
>
> * Check for a function's failure the same way everywhere.
>
> * When a function returns something "truthy" on success, and something
>   "falsy" on failure, use
>
>     if (!fun(...))
>
>   Special cases:
>
>   - bool true on success, false on failure
>
>   - non-null pointer on success, null pointer on failure
>
> * When a function returns non-negative value on success, negative value
>   on failure, use
>
>     if (fun(...) < 0)
>
> * Avoid non-negative integer error values.
>
> * Avoid if (fun(...)), because it's locally ambiguous.
>
>> @Peter, Juan
>>
>> I'd like to hear your opinions.
>
> Yes, please.

I agree with Markus here for three reasons:

1 - He is my C - lawyer of reference O-)

2 - He has done a lot of work cleaning the error handling on this file,
    that was completely ugly.

3 - I fully agree that code is more maintenable this way.  I.e. if any
    function changes to return positive values for non error, we get
    better coverage.

Later, Juan.