[ovs-dev] [RFC PATCH 1/2] Widen TCP flags handling.

Ben Pfaff blp at nicira.com
Tue Sep 24 23:50:37 UTC 2013


On Wed, Sep 18, 2013 at 01:42:40PM -0700, Jarno Rajahalme wrote:
> Widen TCP flags handling from 7 bits (uint8_t) to 12
> bits (uint16_t).  The kernel interface remains at 8
> bits, which makes no functional difference now, as none
> of the higher bits is currenlty of interest to the

"currently"

> userspace.
> 
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

You need to get a review from Jesse or Pravin on the kernel parts, but
I noticed some things myself.

> index 4defcdb..b43d4b3 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1155,7 +1155,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
>  	used = flow->used;
>  	stats.n_packets = flow->packet_count;
>  	stats.n_bytes = flow->byte_count;
> -	tcp_flags = flow->tcp_flags;
> +	tcp_flags = ntohs(flow->tcp_flags);

It seems a little odd to assign the result of ntohs() directly to a
u8.  I know it's intentional but it looks like a mistake.  I'd be
tempted to do something to make it obviously correct.  Maybe add "&
0xff"?

>  	spin_unlock_bh(&flow->lock);
>  
>  	if (used &&
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 29122af..fd715aa 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -389,19 +389,19 @@ void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key *src,
>  		*d++ = *s++ & *m++;
>  }
>  
> -#define TCP_FLAGS_OFFSET 13
> -#define TCP_FLAG_MASK 0x3f
> +#define TCP_FLAGS_OFFSET 6
> +#define TCP_FLAG_MASK 0x0fff

I usually expect an offset to be in bytes, not u16s.

The code in ovs_flow_used() is kind of ugly.  I see that there's a
tcp_flag_word() macro these days.  Maybe we could use it to avoid
weird by-hand pointer arithmetic.

The change to netflow_v5_record seems bogus since that's a wire
protocol and defines tcp_flags as 8 bits.  The Cisco definition
implies that the pad byte must be zero, see
http://www.cisco.com/en/US/docs/net_mgmt/netflow_collection_engine/3.6/user/guide/format.html



More information about the dev mailing list