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 <b...@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 <jrajaha...@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 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
