On Thu, 16 Dec 2021, Abramo Bagnara wrote:

And a real kicker is this change:

- * @return 0 on success, nonzero on failure
+ * @return false on success, true on failure

Who really thinks that the above is idiomatic and an improvement in code
understandability? It is not that MISRA does require it in this
particular case, but it certainly did skew the mindset of the person who
did that change.

I'd like very much you take a look to

https://github.com/Abramo-Bagnara/zephyr/commit/87a42d4185828fb1e651604b8ee878063fb6b08a

so to be sure I have intercepted correctly the community expectations.

Wow! OK... I see that this fu^H^Hmixed-up convention already exists in
the tree today. You simply followed that precedent.

[...]
* @return False on success, True on failure
*/
#define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \
[...]

The above is very wrong. It is completely counter-intuitive. But given
its name, most people won't expect it to return a bool but rather a 0
for success, which makes the code work the same.

Here's what I think the logic should be:

- Boolean results should come with a function whose name clearly implies
a condition. Example: k_is_user_context(), is_buf_set(),
is_el2_sec_supported(), etc.

- A function that accomplishes something and returns the outcome
(success vs failure) should use 0 for success and a negative value for
errors. Sometimes such functions may return different success results
using positive values, hence the negative values for errors.

So when it says: "returns 0 for success, non-zero for error", in fact
it should rather be "0 for success, negative error code otherwise".
Hence you should not return 1 for error but rather something like
-EINVAL or any other error code that best fits the error condition.
Most of the time the caller doesn't care about the actual error code
but that still makes the callee more self-explanatory.


Nicolas