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