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.