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

[Markus Armbruster]

"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.