On Fri, 17 Dec 2021, Abramo Bagnara wrote:

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?

It should, especially if the calling code already uses != 0 or == 0.
Success with > 0 is not that frequent.

But that's for functions that perform a substantial action. It would be
overkill for merely testing a condition.

- Z_OOPS takes an error (defined above)

It should rather be: Z_OOPS() takes an bool argument indicating
whether it should oops the system.

- Z_SYSCALL_VERIFY_MSG takes a boolean and return an error

It may still return a boolean. This is a validation macro. The provided
expression is either true or false. It should itself also return true
for success or false for error. But it currently reads as:

#define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \
bool expr_copy = !(expr); \
if (expr_copy) { \
LOG_MODULE_DECLARE(os, CONFIG_KERNEL_LOG_LEVEL); \
LOG_ERR("syscall %s failed check: " fmt, \
__func__, ##__VA_ARGS__); \
} \
expr_copy; })

Here expr_copy is a really really bad name. This is _not_ a copy of
anything. This is a variable carrying the result of the expression, and
even the inverted result!

So, it would have been much better if it had been written as:

* @return true on success, false on failure
*/
#define Z_SYSCALL_VERIFY_MSG(expr, fmt, ...) ({ \
bool is_ok = (expr); \
if (!is_ok) { \
LOG_MODULE_DECLARE(os, CONFIG_KERNEL_LOG_LEVEL); \
LOG_ERR("syscall %s failed check: " fmt, \
__func__, ##__VA_ARGS__); \
} \
is_ok; })

That is much more self explanatory.

Then you can do constructs like:

Z_OOPS(!Z_SYSCALL_VERIFY_MSG(expression, "expression failed"));

The above can naturally be interpreted as: "if the expression does not
verify then we oops."

Same thing for the other derrivatives.

It does not always have to be 0 means success and <0 means error,
especially in this case where Z_SYSCALL_VERIFY_MSG() has a clear
semantic i.e. it is mainly testing a condition and does not perform any
substantial operation (the log is merely a side effect).


Nicolas