Il 17/12/21 04:06, Nicolas Pitre ha scritto:

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.

Let resume state after commit with the points still to be decided, so we can be more specific and finalize:

- 0 is success, non-zero is error: the comunity agrees to change to have

= 0 success, < 0 is error? Is it ok to replace current 1 with -EINVAL?

- Z_OOPS takes an error (defined above)

- Z_SYSCALL_VERIFY_MSG takes a boolean and return an error

- Z_SYSCALL_VERIFY is a frontend for Z_SYSCALL_VERIFY_MSG

- Z_SYSCALL_MEMORY returns the result of Z_SYSCALL_VERIFY_MSG

- Z_SYSCALL_MEMORY_READ and Z_SYSCALL_MEMORY_WRITE are frontends of Z_SYSCALL_MEMORY

- Z_SYSCALL_MEMORY_ARRAY combine Z_SYSCALL_VERIFY_MSG and Z_SYSCALL_MEMORY

- Z_SYSCALL_MEMORY_ARRAY_READ/Z_SYSCALL_MEMORY_ARRAY_WRITE are fronted of Z_SYSCALL_MEMORY_ARRAY

- Z_SYSCALL_DRIVER_OP returns the result of Z_SYSCALL_VERIFY_MSG

- Z_SYSCALL_SPECIFIC_DRIVER combine Z_SYSCALL_OBJ and Z_SYSCALL_VERIFY_MSG

- Z_SYSCALL_IS_OBJ returns the result of Z_SYSCALL_VERIFY_MSG: the comunity agrees that is misnamed? What is a better name?

- Z_SYSCALL_OBJ, Z_SYSCALL_OBJ_INIT, Z_SYSCALL_OBJ_NEVER_INIT are frontends of Z_SYSCALL_IS_OBJ




--
Abramo Bagnara

BUGSENG srl - http://bugseng.com
mailto:abramo.bagnara@...