Re: [PATCH net-next v5 2/8] netfilter: Factor out nf_ct_get_info().

[Pablo Neira Ayuso <pablo-Cap9r6Oaw4JrovVCs/uTlw@public.gmane.org>]

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 <jarno@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.

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.

Thanks.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev