Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751465AbcCQCzA (ORCPT ); Wed, 16 Mar 2016 22:55:00 -0400 Received: from relay3-d.mail.gandi.net ([217.70.183.195]:35065 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbcCQCy7 (ORCPT ); Wed, 16 Mar 2016 22:54:59 -0400 X-Originating-IP: 74.125.82.42 MIME-Version: 1.0 In-Reply-To: <2159518.ilphXU8bxQ@wuerfel> References: <1458132481-318209-1-git-send-email-arnd@arndb.de> <20160316132536.GA29550@salvia> <2159518.ilphXU8bxQ@wuerfel> From: Joe Stringer Date: Thu, 17 Mar 2016 15:54:34 +1300 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [ovs-dev] [PATCH] openvswitch: call only into reachable nf-nat code To: Arnd Bergmann Cc: Pablo Neira Ayuso , ovs dev , netdev , linux-kernel@vger.kernel.org, Paolo Abeni , "David S. Miller" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1828 Lines: 39 On 17 March 2016 at 02:56, Arnd Bergmann 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.