On 18 September 2015 at 09:58, Ben Pfaff <b...@nicira.com> wrote:
> On Thu, Sep 17, 2015 at 04:04:24PM -0700, Joe Stringer wrote:
>> This patch adds a new 32-bit metadata field to the connection tracking
>> interface. When a mark 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_mark" 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 mark 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_mark)),2
>> in_port=2,ct_state=-trk,tcp,action=ct(table=1)
>> table=1,in_port=2,ct_state=+trk,ct_mark=1,tcp,action=1
>>
>> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
>> Acked-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> Thanks a lot for writing this!
>
> Without the explanation and the example in the commit log above, the
> power and the restrictions and the intent are hard to understand. Do
> you think you could add any or all of the above to the documentation
> somewhere?
A fair request. I see that the match fields for
ct_zone/ct_mark/ct_label are a bit lacking in ovs-ofctl, and I'll do
another iteration across the ovs-ofctl documentation in general to try
to make it flow better. I can also add a basic example of using
connection tracking after the action documentation.
> decode_NXAST_RAW_CT() assumes that the nested actions are always
> OpenFlow 1.0 actions. I think that's a bad idea--I think that the
> nested actions should use the same OpenFlow version as NXAST_RAW_CT
> itself. I don't think that information is readily available to decode
> functions, so you might have to add a new parameter to all the decode
> functions (in a separate patch please to avoid making this one hard to
> review).
Agree, next series will have this patch.
> It seems slightly inconsistent for decode_NXAST_RAW_CT() to have just
> one case where it returns without pushing the actions back on. I'd
> change the
> return OFPERR_OFPBAC_BAD_ARGUMENT;
> to
> error = OFPERR_OFPBAC_BAD_ARGUMENT;
> unless there's some reason for the difference that I didn't spot.
Nope, that was unintended. Good catch.
> ofpacts_verify_nested() seems odd. First it seems strange to verify
> 'outer_action' at all like this:
> + if (outer_action != OFPACT_CT && outer_action != OFPACT_WRITE_ACTIONS) {
> + return unsupported_nesting(a->type, outer_action);
> + }
> because aren't the possibilities known at compile time? If I'm right
> about that, then if we need to check 'outer_action' then I'd guess an
> ovs_assert would be the right thing since we'd be checking an invariant.
OK.
> Second, the check at the end of ofpacts_verify_nested() seems like
> something we'd want to check for actions that aren't inside an outer
> action at all, not just something to check inside OFPACT_WRITE_ACTIONS:
> + if (field && outer_action != OFPACT_CT && field->id == MFF_CT_MARK) {
> + VLOG_WARN("cannot set CT fields outside of \"ct\" action");
> + return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> }
True, the function originally handled both cases but this behaviour
was dropped by mistake. I'll update it so that ofpacts_verify_nested()
handles all constraints related to nesting, and is called
unconditionally. I should be able to write a simple test to ensure
this behaviour, too.
> This comment on CT_MEMBERS strikes me a bit oddly. There's ordinarily
> no need to worry about alignment in a struct unless you want tight
> packing or to exactly match some protocol definition. Neither one comes
> up here as far as I can tell. Also I don't know why ct_pad is there
> when there's an automatically sized pad[] member in struct
> ofpact_conntrack:
> +/* Even though zone_src contains a pointer, it should be 64-bit aligned due
> + * to the initial ofpact,flags,zone_imm members in struct ofpact_conntrack.
> */
> #define CT_MEMBERS \
> uint16_t flags; \
> uint16_t zone_imm; \
> struct mf_subfield zone_src; \
> - uint8_t recirc_table;
> + uint8_t recirc_table; \
> + uint16_t ct_pad;
This is one of the pieces that changed between the version I sent to
you separately vs. the email above, sorry about that. The ct_pad
member is no longer present, CT_MEMBERS still exists but there's a
different explanation:
/* We want to determine the size of these elements at compile time to ensure
* actions alignment, but we also want to allow ofpact_conntrack to have
* basic _put(), _get(), etc accessors defined below which access these
* members directly from ofpact_conntrack. An anonymous struct will serve
* both of these purposes. */
#define CT_MEMBERS \
struct { \
struct ofpact ofpact; \
uint16_t flags; \
uint16_t zone_imm; \
struct mf_subfield zone_src; \
uint16_t alg; \
uint8_t recirc_table; \
}
/* This is (and should only be) used for calculating action alignment. */
struct ofpact_ct_pad {
CT_MEMBERS;
};
/* OFPACT_CT.
*
* Used for NXAST_CT. */
struct ofpact_conntrack {
union {
CT_MEMBERS;
uint8_t pad[OFPACT_ALIGN(sizeof(struct ofpact_ct_pad))];
};
struct ofpact actions[];
};
BUILD_ASSERT_DECL(offsetof(struct ofpact_conntrack, actions)
% OFPACT_ALIGNTO == 0);
BUILD_ASSERT_DECL(offsetof(struct ofpact_conntrack, actions)
== sizeof(struct ofpact_conntrack));
> The documentation seems a little incomplete, because it says that
> certain actions can only be taken inside ct(), but it leaves out the
> severe restrictions on what is allowed to be done inside ct().
I'll update it.
> I'm not certain that verification actually recurses into OFPACT_CT
> nested actions. I don't see a test case that provokes a nested action
> error, which would probably be easy to write.
Sure, I'll add it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
