[Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive

Lukas Bulwahn lukas.bulwahn at gmail.com
Wed Jul 15 05:32:25 UTC 2020



On Tue, 14 Jul 2020, Mrinal Pandey wrote:

> 
> 
> On Sat, Jul 11, 2020 at 3:17 AM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
> 
> >> For Issue 5: Can you provide me (and the CC: the list) the list of
> >> false positives (the commit hashes) you found for issue 5 on
> >> EXPORT_SYMBOL?
> >
> >
> > Here are the commit hashes for which the warning is issued:
> > 54505a1e2083
> > 75d75b7a4d54
> > 8084c99b9af6
> > bfdaf029c9c9
> > dfd402a4c4ba
> >
> >> Can you also provide a short rationale/explanation for
> >> each case that you considered a false positive?
> >
> >
> > In each case the `EXPORT_SYMBOL()` is correctly written and the variable/function to be exported
> > is also inside the parentheses, still, we get the warning. Please let me know if I am wrong here.
> 
> I checked those warnings. Some of the patches are good cases to check if we can improve the heuristics on
> EXPORT_SYMBOL().
> 
> E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user() into csum_and_copy_from_user()") looks
> sound to me, and checkpatch.pl should not really warn about that.
> 
> Mrinal, maybe you can find out why checkpatch.pl believes that this case is wrong and needs to be warned
> about?
> 
> 
> Sir,
> 
> Commit `54505a1e2083`, `8084c99b9af6` and `bfdaf029c9c9` have style issues. The documentation says to use
> `EXPORT_SYMBOL()` just after the closing bracket `}` of the function, it is exporting. In these commits `EXPORT
> SYMBOL()`
> is either defined at a point later or has been defined after leaving a blank line which is totally unneeded.
>

Possibly, a new line is okay; I think we would some statistics on the 
overall use of EXPORT_SYMBOL() to see if a new line is generally accepted 
or if it is really a rare case (<3%) and checkpatch.pl should keep 
warning about that.
 
> However, commits `dfd402a4c4ba` and `75d75b7a4d54` look fine to me and have one thing in common, the
> `EXPORT_SYMBOL()` in these is used inside of a macro. checkpatch shouldn't warn about these.
> 

That is probably tricky to handle with checkpatch.pl, though.

> I have noticed checkpatch denoting wrong line numbers for certain files for several commits.
> For example, for the above commit
> `bfdaf029c9c9`, the following warning is issued:
> 
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #74: FILE: arch/ia64/lib/csum_partial_copy.c:122:
> +EXPORT_SYMBOL(csum_and_copy_from_user);
> 
> but the file arch/ia64/lib/csum_partial_copy.c is only 113 lines long.
> Please let me know if you can reproduce this.

Can you find out how the line number is computed?

Of course, checkpatch.pl is running on the patch, not on any specific 
file. So, in general, you really do not know how long the file was that 
the author based the patch on.

In our case, you need to check the state of the file at the commit above.

I checked that and it seemed correct to me, line 122 refers to 
EXPORT_SYMBOL. So, I do not see the bug here.



Lukas


More information about the Linux-kernel-mentees mailing list