Re: [ovs-dev] [PATCH] openvswitch: call only into reachable nf-nat code

[Joe Stringer]

On 17 March 2016 at 02:56, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wednesday 16 March 2016 14:25:36 Pablo Neira Ayuso wrote:
>> Not related with this patch, just a side note/recommendation.
>>
>> I understand this code just got into tree, and that this needs a bit
>> work/iterations but this thing above is ugly, I wonder if there is a
>> better way to avoid this.
>>
>> Probably with some modularization of the openvswitch code this will
>> look better, I mean:
>>
>> 1) adding Kconfig switches to enable conntrack and NAT support to
>>    net/openvswitch/Kconfig.
>>
>> 2) Move the NAT code to the corresponding openvswitch/nat.c file.
>>
>> Just my two cents.
>
> Yes, I think that would be good too. I also found that the driver
> used to look like that but it was changed as part of f88f69dd17f1
> ("openvswitch: Remove conntrack Kconfig option.").

I don't see how it helps to separate the conntrack, nat pieces into
different Kconfig switches unless you're splitting their code
completely out of the openvswitch module.

Prior to f88f69dd17f1, OPENVSWITCH Kconfig option had no dependency on
NF_CONNTRACK. OPENVSWITCH_CONNTRACK was a switch to build those pieces
into the OPENVSWITCH module (ie,
"openvswitch-$(CONFIG_OPENVSWITCH_CONNTRACK) += conntrack.o"). If you
configured NF_CONNTRACK=m, OPENVSWITCH=y, OPENVSWITCH_CONNTRACK=y then
it would break in the same kind of way as the bug that Arnd is
reporting. So, that patch was introduced to shift the dependency up to
the OPENVSWITCH kconfig option. At that point, there was no strong
benefit to keeping the conntrack separately configurable, so that was
removed. Further splitting the code out in some way (eg new modules)
seemed way overkill to tidy up some ugly-looking dependency logic.

Maybe someone with better Kconfig-fu than me can point out a better way.