[ovs-dev] [PATCHv3 08/11] Add connection tracking label support.

[Joe Stringer]

On 30 September 2015 at 08:53, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Sep 29, 2015 at 11:56:40PM -0700, Ben Pfaff wrote:
>> On Tue, Sep 29, 2015 at 01:40:31PM -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 TCP connections from port 1->2 and only allow
>> > established connections from port 2->1, and to associate a label with
>> > those connections:
>> >
>> >     table=0,priority=1,action=drop
>> >     table=0,arp,action=normal
>> >     table=0,in_port=1,tcp,action=ct(commit,exec(set_field:1->ct_label)),2
>> >     table=0,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 <joestringer at nicira.com>
>> > Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> > ---
>> > v3:
>> > - Improve ct_label documentation
>> > - Remove extraneous NXM mask checks
>> > - Make get_u128_ptr check both MINIFLOW_IN_MAP for the label
>> > - Convert wire format to use be128
>> > - Add dpctl,ofctl tests
>> > v2:
>> > - Address feedback from v1
>>
>> It's late and I'm getting tired, so I'd better look again later.  For
>> now, one thing comes to mind.  Would it be worth defining
>>
>> #define OVS_U128_MAX (union ovs_u128) { .u64 = { UINT64_MAX, UINT64_MAX } }
>> #define OVS_BE128_MAX (union ovs_be128) { .be64 = { OVS_BE64_MAX, \
>>                                                     OVS_BE64_MAX } }

Sounds reasonable, I'll look for places that could make use of this.

> This:
>         if (strlen(s)) {
> can be written more efficiently as:
>         if (s[0]) {
> or variants.
>
> I think there are about three almost-identical bits of code to parse a
> u128 with an optional mask.  Can any of this duplication be reduced?

I noticed this, skimming back over. Should be able to collapse them down.