checkpatch: Ignore macro arg reuse by default

[jrajahalme]

cilium/cilium has ignored MACRO_ARG_REUSE since
cilium/cilium#25284, update the default ignores so that this does not need to be explicitly added in cilium/cilium any more.

[gentoo-root]

cilium/cilium has ignored MACRO_ARG_REUSE since cilium/cilium#25284

Why was it ignored in the first place? It's a pretty useful check, you would often take this hint into account. I don't think it should be ignored at all, it's not something that we constantly violate in our project, and false positives like the one in your pull request can be dealt with on a case-by-case basis (it's not even an error that displays in red, it's a check that is supposed to draw your attention and double-check the code).

[jrajahalme]

CI test still fails even if it is not displayed in red. Could you tell me how to deal with this in a case-by-case basis. All I could come up with was to ignore this check for the whole cilium/cilium repo.

[jrajahalme]

This was ignored due to the macro really needing to to use the argument multiple times, so in this case it is a false positive.

[gentoo-root]

CI test still fails even if it is not displayed in red. Could you tell me how to deal with this in a case-by-case basis. All I could come up with was to ignore this check for the whole cilium/cilium repo.

That's not a solution either. From my experience, checkpatch shows false positives pretty often, each time with a different check. Sometimes there are even cases when it suggests a change, but after making that change the compiler starts complaining about another thing and suggests a reverse change.

For example, take a look at this change. If I don't remove the curly braces after if (snat_v4_track_connection(..., checkpatch complains that they are not needed. If I do remove them, clang complains:

nat_test.c:116:4: error: add explicit braces to avoid dangling else [-Werror,-Wdangling-else]
                        test_fail();
                        ^
./common.h:138:4: note: expanded from macro 'test_fail'
        } else {                                \
          ^

Such situations happen from time to time, and we need a mechanism or agreement to deal with them (have a way to mark exclusions for linters (with special comments, probably)? just comment on review and agree to ignore that failure?).

Adding more and more checks to the global ignore list is not a solution, this way we'll end up having all checks ignored eventually.

This was ignored due to the macro really needing to to use the argument multiple times, so in this case it is a false positive.

Yes, it was a false positive in your particular patch, the macro parameter was used as a struct field name, not as a "formal parameter". It's not a reason to ignore this check globally, though, because in most cases it will show real cases where we wouldn't want to access the same parameter twice, so it's a useful check in general, it's not a check that throws false positives 90% of times. We should either:

1. Fix checkpatch (I doubt it's feasible in this case).
2. Or find a way to mark such exclusions locally (in a code comment).
3. Or find a way to tell the CI to ignore specific occurrences.
4. Or make an agreement how we communicate that we want to merge PRs with red CI.

[jrajahalme]

IMO #2 above would be the best solution, basically allow a comment right above the offending line basically turning on --ignore options on a case-by-case basis. This would be some feature work on checkpatch, no?

#3 could work if we had a way for the CI to observe PR comments, pick out checkpatch args from there, and pass them to the CI run locally. bpf/Makefile.bpf already passes CHECKPATCH_ARGS environment variable to checkpatch, so the only thing missing is picking them up from PR comments. This would have the downside that the same checkpatch failure can happen on the same code also later, in unrelated PRs, which would be really annoying, especially when the PR author has no idea how to suppress it.

On #4 we have recently had discussion about the use of the ready-to-merge label in the project, with the conclusion that it may only be set once CI is green for all required tests.

So in general I agree, as long as we do not have one of 1-3 above impelemented, we should probably mark checkpatch CI run as not required to not block work on the project.