Actually I've reviewed it again, and I think that board zero is supposed to
always be present, but that might not necessarily be the case. I'm not sure if
the checking in close_dev_ioctl() is sufficient?
Personally, I don't have a problem with adding a NULL check, but you need to
sell it better in the commit message. Just saying Coverity says we should add
a NULL check is not sufficient. Anyone reviewing this code has to investigate,
is Coverity right? How can we trigger this bug? If you did that investigation
for us, then we would review your patches more generously.
When you review lots and lots of patches per day, then you develop rules to
avoid wasting time. And one of those rules is "We don't do things just to make
the checker happy." It's not a rule that I invented, it's the normal rule for
almost everywhere in the kernel.
But if instead of a static checker fix, you sold the patch as a clean up then
maybe that would be fine:
The NULL checking on handle_to_descriptor() is not consistent. The ibclose()
function has a NULL check for this exact function call but it's not checked
here.
It turns out that the zero board is always supposed to be present. It's
allocated in ibopen() and freed in ibclose(). So, really, the NULL check
should not be required. However, the error message in ibclose() suggests
that desc could be NULL in the case of a bug. Let's just be cautious and
add a NULL check here as well.
To be honest, I don't know if that commit message is good enough to get the
patch past Greg but I might have been sold by something like that. At a
minimum it shows that you are invested in the code and we tend of give people
who care about the driver more freedom.
> If we really hate to add that check, then there can be two alternatives
> 1. add a comment
>
Adding a comment is fine.
> 2. change the code as follows:
> /* There is always a board 0 */
> desc = file_priv->descriptors[0];
This is not fine. It's an example of making the code uglier to silence a
static checker warning.
Btw, don't get too hung up on silencing every static checker warning. Kernel
developers are good at fixing any real bugs so just look at how old the warning
is and ignore the old ones. It's not like there is a shortage of real bugs to
fix. For example, in open_dev_ioctl() if increment_open_device_count() fails
then the file_priv->descriptors[i] is not cleaned up. It can't be used or
freed. If ->is_board is false none of the file_priv->descriptors[] elements
that come afterward can be freed either.
regards,
dan carpenter
