checkpatch: correct diagnostic for ENOSYS use #33637
Conversation
The rationale is for Linux; use the rationale for Zephyr. See zephyrproject-rtos#23727 Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
pabigot
force-pushed
the
nordic/20210323a
branch
from
4cf587a to
d99bf6f
Compare
| if ($line =~ /\bENOSYS\b/) { | ||
| WARN("ENOSYS", | ||
| "ENOSYS means 'invalid syscall nr' and nothing else\n" . $herecurr); | ||
| "Use ENOTSUP instead of ENOSYS in Zephyr API\n" . $herecurr); |
There was a problem hiding this comment.
will this issue a warning on any mention of ENOSYS or only on function returns?
There was a problem hiding this comment.
It will diagnose the same uses it does today, but with a message that is meaningful for Zephyr.
There was a problem hiding this comment.
this means that if I am checking some return code from a module that returns NOSYS we will have the above warning, which is not what we want:
code like this:
ret = some_third_party_lib();
if (ret == -ENOSYS) {
}
returns:
-:9: WARNING:ENOSYS: ENOSYS means 'invalid syscall nr' and nothing else
#9: FILE: drivers/dma/dma_dw.c:262:
+ if (ret == -ENOSYS) {
is this what we want?
There was a problem hiding this comment.
or with this PR...
-:9: WARNING:ENOSYS: Use ENOTSUP instead of ENOSYS in Zephyr API
#9: FILE: drivers/dma/dma_dw.c:262:
+ if (ret == -ENOSYS) {
There was a problem hiding this comment.
The existing message appears to be emitted on every introduction of ENOSYS regardless of context, so yes, with this change we would continue to get false positives, but with a more relevant message that would make more clear why it should be ignored ("this use isn't in Zephyr API, so we can ignore it").
Historically such false positives don't appear to be a concern. dma_dw.c does not actually have the code that evokes a diagnostic, and in fact there are no checks of a return value against -ENOSYS in Zephyr today (exclusive of a couple test files). There are many checks against -ENOTSUP, in drivers, samples, and subsystems as well as tests.
Improving the existing diagnostic is the solution I'm offering. There's no obligation to accept it. If somebody would prefer to try to enhance checkpatch to identify only cases where -ENOSYS is returned, taking into account that may be from ret = -ENOSYS; ... ; return ret; feel free; or decide the status quo is better than changing the message.
There was a problem hiding this comment.
How about we tweak the text instead, with something along the lines of:
Use ENOTSUP instead of ENOSYS as return codes Zephyr APIs. If this warning appears as a result of checking the error code returned by an external API, then this check can be ignored.
There was a problem hiding this comment.
How about we tweak the text instead, with something along the lines of:
IMO we should drop this check completely and just document whatever we want in the coding guidelines. This is becoming too verbose and more confusing TBH.
I am going to take my time and think about the whole issue again. I still think it is not the right solution to forbid a return code that has a meaning and usecases just because we made a mistake and we are too deep in the mud.
We could say that device driver shall use NOTSUP and make it a rule that we do not have to change in the future but still allow usage of NOSYS where it makes sense (outside of drivers).
The rationale is for Linux; use the rationale for Zephyr.
See #23727
Signed-off-by: Peter Bigot peter.bigot@nordicsemi.no