[ovs-dev] [PATCH 6/8] Add connection tracking label support.

[Joe Stringer]

Thanks for the feedback, I've taken the feedback in for the next
version.  A couple of minor comments inline:

On 11 September 2015 at 16:22, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> @@ -6084,12 +6090,11 @@ ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action)
>>
>>      field = ofpact_get_mf_field(a->type, a);
>>      if (outer_action == OFPACT_CT
>> -        && (!field
>> -            || (field && field->id != MFF_CT_MARK))) {
>> +        && (!field || (field && !field_requires_ct(field->id)))) {
>>          return unsupported_nesting(a->type, outer_action);
>>      }
>>
>> -    if (field && outer_action != OFPACT_CT && field->id == MFF_CT_MARK) {
>> +    if (outer_action != OFPACT_CT && field && field_requires_ct(field->id)) {
>>          VLOG_WARN("cannot set CT fields outside of \"ct\" action");
>>          return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
>>      }
>
> Would this also prevent reading from the ct_fields outside of the ct_action? If so, maybe it shouldn’t?

The field for "set_field" is the destination, and for "reg_move" is
retrieved from the "destination". So it should only be for CT field
writes, see ofpact_get_mf_field().

>> @@ -4177,6 +4181,32 @@ put_ct_mark(const struct flow *flow, struct flow *base_flow,
>>  }
>>
>>  static void
>> +put_ct_label(const struct flow *flow, struct flow *base_flow,
>> +             struct ofpbuf *odp_actions, struct flow_wildcards *wc)
>> +{
>> +    const ovs_u128 *key;
>> +    ovs_u128 *mask, *base;
>> +
>> +    key = &flow->ct_label;
>> +    base = &base_flow->ct_label;
>> +    mask = &wc->masks.ct_label;
>> +
>> +    if (!is_all_zeros(mask, sizeof(*mask)) && memcmp(key, base, sizeof(*key))) {
> We have u128 specific functions for both uses here.
>
> Also, what if we have multiple ct actions with labels?
>> +        struct {
>> +            ovs_u128 key;
>> +            ovs_u128 mask;
>> +        } *odp_ct_label;
>> +
>> +        odp_ct_label = nl_msg_put_unspec_uninit(odp_actions, OVS_CT_ATTR_LABEL,
>> +                                                sizeof(*odp_ct_label));
>> +        odp_ct_label->key = *key;
>> +        odp_ct_label->mask = *mask;
>> +        base_flow->ct_label = *base;
>> +        wc->masks.ct_label = *mask;
> These two seem like NOPs. Maybe the intention was to update the base with the new value?

These are leftovers from applying the ct_* field sets to the flow and
using do_xlate_actions() from compose_conntrack_action(). I intend to
replace it with a custom xlate_actions handler.

Thanks,
Joe