Abramo Bagnara

Il 17/12/21 23:21, Nicolas Pitre ha scritto:
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_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_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).
IOW you are proposing to do exactly what I presented the first time, but inverting the boolean logic from original true for failure and false for success to false for failure and true for success.

It sounds great, but wasn't better then to accept my original changes and open an issue for reverting current logic?

It seems obvious to me that a change aimed to improve code quality (in this case simply avoiding integer and boolean mixing) that changes semantic of functions would have had *zero* chances to be accepted, am I missing something?

Abramo Bagnara


Join to automatically receive all group messages.