> On Dec 25, 2015, at 4:30 AM, Pablo Neira Ayuso <pa...@netfilter.org> wrote:
>
> On Wed, Dec 16, 2015 at 04:36:41PM -0800, Jarno Rajahalme wrote:
>> Define a new inline function to map conntrack status to enum
>> ip_conntrack_info. This removes the need to otherwise duplicate this
>> code in a later patch ("openvswitch: Find existing conntrack entry
>> after upcall.").
>>
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
>> ---
>> include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
>> net/netfilter/nf_conntrack_core.c | 28 +++++++++-------------------
>> 2 files changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/net/netfilter/nf_conntrack.h
>> b/include/net/netfilter/nf_conntrack.h
>> index fde4068..b3de10e 100644
>> --- a/include/net/netfilter/nf_conntrack.h
>> +++ b/include/net/netfilter/nf_conntrack.h
>> @@ -125,6 +125,21 @@ nf_ct_tuplehash_to_ctrack(const struct
>> nf_conntrack_tuple_hash *hash)
>> tuplehash[hash->tuple.dst.dir]);
>> }
>>
>> +static inline enum ip_conntrack_info
>> +nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
>> +{
>> + const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
>> +
>> + if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
>> + return IP_CT_ESTABLISHED_REPLY;
>> + /* Once we've had two way comms, always ESTABLISHED. */
>> + if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
>> + return IP_CT_ESTABLISHED;
>> + if (test_bit(IPS_EXPECTED_BIT, &ct->status))
>> + return IP_CT_RELATED;
>> + return IP_CT_NEW;
>> +}
>> +
>> static inline u_int16_t nf_ct_l3num(const struct nf_conn *ct)
>> {
>> return ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.l3num;
>> diff --git a/net/netfilter/nf_conntrack_core.c
>> b/net/netfilter/nf_conntrack_core.c
>> index 3cb3cb8..7546fc7 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -1056,25 +1056,15 @@ resolve_normal_ct(struct net *net, struct nf_conn
>> *tmpl,
>> ct = nf_ct_tuplehash_to_ctrack(h);
>>
>> /* It exists; we have (non-exclusive) reference. */
>> - if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY) {
>> - *ctinfo = IP_CT_ESTABLISHED_REPLY;
>> - /* Please set reply bit if this packet OK */
>> - *set_reply = 1;
>> - } else {
>> - /* Once we've had two way comms, always ESTABLISHED. */
>> - if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
>> - pr_debug("nf_conntrack_in: normal packet for %p\n", ct);
>> - *ctinfo = IP_CT_ESTABLISHED;
>> - } else if (test_bit(IPS_EXPECTED_BIT, &ct->status)) {
>> - pr_debug("nf_conntrack_in: related packet for %p\n",
>> - ct);
>> - *ctinfo = IP_CT_RELATED;
>> - } else {
>> - pr_debug("nf_conntrack_in: new packet for %p\n", ct);
>> - *ctinfo = IP_CT_NEW;
>> - }
>> - *set_reply = 0;
>> - }
>> + *ctinfo = nf_ct_get_info(h);
>> + if (*ctinfo == IP_CT_ESTABLISHED)
>> + pr_debug("nf_conntrack_in: normal packet for %p\n", ct);
>> + else if (*ctinfo == IP_CT_RELATED)
>> + pr_debug("nf_conntrack_in: related packet for %p\n", ct);
>> + else if (*ctinfo == IP_CT_NEW)
>> + pr_debug("nf_conntrack_in: new packet for %p\n", ct);
>> + *set_reply = NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY;
>> +
>
> This results in worse code for our codebase.
>
I agree that the separate comparisons for debugging output makes the code
uglier.
> Given the little code that is consolidated in this patchset (the last
> larger patch 8/8 is basically a copy of our NAT path but adapted to
> the openvswitch packet flow logic), I don't think this little effort
> in this patch to consolidate this code is worth.
>
> The right way IMO would be to get openvswitch packet flow closer to
> our codebase, then more code could be reused but I understand that is
> not simple thing.
>
> Please, revamp a new patchset version keeping this function that you
> need in your folder.
>
I just posted a v6 removing this small consolidation, as you requested here.
Thanks for the review,
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
