[ovs-dev] [RFC PATCH 1/2] Widen TCP flags handling.
Ben Pfaff
blp at nicira.com
Wed Sep 25 00:12:42 UTC 2013
OK.
The netflow stuff is purely userspace.
On Tue, Sep 24, 2013 at 04:57:33PM -0700, Jarno Rajahalme wrote:
> Ben,
>
> All valid points, will address once I hear back from Jesse or Pravin.
>
> Thanks,
>
> Jarno
>
> On Sep 24, 2013, at 4:50 PM, Ben Pfaff <blp at nicira.com> wrote:
>
> > 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