On 2019-09-04, at 21:05:35 +0200, Pablo Neira Ayuso wrote: > Thanks for working on this. Happy to help. > Could you squash a few of these patches to get a smaller patchset? Absolutely. > My suggestions: > > * Squash 01/30, 02/30 and 03/30, call this something like: "netfilter: > add missing include guard". Just document that the chunk for the > flowtable is fixing up a comment. Will do. > * For 04/30, since this is about SPDX, I would suggest you leave this > behind and we wait for someone to make a whole pass over the > netfilter headers to check for missing SPDX tags? Not a deal > breaker, you can keep it in this batch if you like. Will drop it. This was a bit speculative: I think I've got it right, but, as you say, this may be one to leave to someone with more expertise. > * Squash 05/30, 06/30 and 07/30, call this I'd suggest: "netfilter: > fix coding style errors", document the stray semi-colons, the > Kconfig missing indent and the trailing whitespaces. Will do. > * Squash 09/30, 10/30, 11/30, 12/30 and 12/30. They all refer to > #include updates, could you squash and document these updates? Will do. > * 14/30, "netfilter: remove superfluous header" I'd suggest you rename > this to "netfilter: remove nf_conntrack_icmpv6.h header". Will do. > * 17/30 I don't think struct nf_bridge_frag_data qualifies for the > global netfilter.h header. What about netfilter_bridge.h? > * Please, squash 21/30 and 22/30. Will do. > * With 20/30 gets more ifdef pollution to optimize a case where kernel > is compiled without this trackers. I would prefer you keep this > back. > > * 24/30 nft_set_pktinfo_ipv6_validate() definition already > deals with this in the right way. > > * 25/30 nf_conntrack_zones_common.h only makes sense if NF_CONNTRACK > is enabled, I don't understand. > > * 27/30 identation is not correct, not using tabs. > > * 26/30 is adding more #ifdef CONFIG_NETFILTER to the netfilter.h > header. They make sense to make this new infra to compile headers, > but from developer perspective is confusing. > > * 30/30 very similar to 26/30... As I mentioned in the cover-letter the idea behind my approach was to config out as much code as possible: if header H is only required when config C is enabled, then wrap it in an `#if IS_ENABLED(CONFIG_C)`. However, you're clearly not keen, and, having had a poke around in other headers that have been moved off the blacklist, I've come to the con- clusion that it was the wrong way to go: we want less #ifdeffery, not more. Will rework this part of the series. J.
Attachment:
signature.asc
Description: PGP signature
