On 30 March 2016 at 19:55, Ben Pfaff <b...@ovn.org> wrote:
> On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote:
>> Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a
>> mask, would previously overwrite the entire ct_mark field rather than
>> modifying only the specified bits. Fix the issue.
>>
>> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>> v2: Remove wildcards from put_ct_mark().
>
> I didn't test this but it looks good and compiles cleanly.
>
> A possible (though unnecessary) enhancement to the test would be to
> verify that setting a bit to 0 also works.
>
> Acked-by: Ben Pfaff <b...@ovn.org>Thanks for the review. Actually, the "set-0" test is necessary and reveals that this series isn't really targeting the core issue. I was a bit confused about how 'wc' were being used and what's the 'correct' way to handle these. After discussion with Jarno I understand that the 'wc' represents both the incoming match on a given field, but also the bits that have been modified in a 'set_field' action. Furthermore, as soon as a 'set_field' action is translated, the field is fully masked. This is actually what leads to the incorrect behaviour for setting the ct_mark/ct_labels. Even when modifying just a single bit of a field, the entire field is marked to change in the 'wc'. I'm looking at preparing a patch which will only mask the bits which are modified by the set_field action. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
