On 18 September 2015 at 10:49, Ben Pfaff <b...@nicira.com> wrote: > On Thu, Sep 17, 2015 at 04:04:26PM -0700, Joe Stringer wrote: >> This patch adds a new 128-bit metadata field to the connection tracking >> interface. When a label is specified as part of the ct action and the >> connection is committed, the value is saved with the current connection. >> Subsequent ct lookups with the table specified will expose this metadata >> as the "ct_label" field in the flow. >> >> For example, to allow new connections from port 1->2 and only allow >> established connections from port 2->1, and to associate a label with >> those connections: >> >> priority=1,action=drop >> priority=10,arp,action=normal >> priority=10,icmp,action=normal >> in_port=1,tcp,action=ct(commit,exec(set_field:1->ct_label)),2 >> in_port=2,ct_state=-trk,tcp,action=ct(table=1) >> table=1,in_port=2,ct_state=+trk,ct_label=1,tcp,action=1 >> >> Signed-off-by: Joe Stringer <joestrin...@nicira.com> >> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com> >> --- >> v2: Address feedback from v1 > > MINIFLOW_GET_U128_PTR seems risky. How you can be sure that both 64-bit > components of the u128 are present?
Currently we only check the first 64-bit component. Perhaps we could expand the following: MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) ? .... to check both pieces: (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) && MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD))) ? ... > Is the formatting and parsing of ovs_u128 in lib/meta-flow.[ch] > consistent with what we're doing elsewhere? It essentially treats the > bits in an ovs_u128 as if they were a (putative) ovs_be128; that is, > it's always big-endian. I guess that's OK as long as we're consistent > throughout the code. I don't think there's an "elsewhere" to be consistent with, as such. This patch introduces the u128 type into meta-flow. In terms of the kernel interface, the structure for this field is host byte-order; it's not as though this is actually a field in a packet we will see on the wire. To be more consistent with the other parts of meta-flow.[ch], I suppose that we would introduce an ovs_be128 type rather than using the existing ovs_u128. > The wire format name though (in meta-flow.h etc) should probably not be > u128, because that implies that it's in host byte order. > > I wonder whether ovs_u128 should actually be ovs_be128, with each of the > components defined as big-endian too. Do we ever actually use an > ovs_u128 as an integer? If not, then having it as big-endian would make > it a lot easier to think about (at least for me). Makes sense. I can follow up on adding an ovs_be128 definition and translating to host byte order in the usual places to interface with the kernel. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
