Thanks for the review!
> On Jun 21, 2016, at 1:57 PM, Joe Stringer <j...@ovn.org> wrote:
>
> On 20 June 2016 at 17:19, Jarno Rajahalme <ja...@ovn.org> wrote:
>> Only allow setting conntrack mark or labels when the commit flag is
>> specified. This makes sure we can not set them before the connection
>> has been persisted, as in that case the mark and labels would be lost
>> in an event of an userspace upcall.
>>
>> OVS userspace already requires the commit flag to accept setting
>> ct_mark and/or ct_labels. Validate for this on the kernel API.
>>
>> Finally, set conntrack mark and labels right before committing so that
>> the initial conntrack NEW event has the mark and labels.
>>
>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
>
> The structure of this commit message suggests there are multiple
> changes trying to be addressed in one patch. I suggest splitting them
> out.
>
Done for v2 I just sent for net.
> In terms of applying the mark and labels before committing the
> connection, that's actually the behaviour I would expect if you were
> to execute ct(mark=foo,commit). The NEW event should include these
> pieces, and should have all along.
Right, the v2 patch 1/2 does this.
>
>> @@ -1145,6 +1155,20 @@ static int parse_ct(const struct nlattr *attr, struct
>> ovs_conntrack_info *info,
>> }
>> }
>>
>> +#ifdef CONFIG_NF_CONNTRACK_MARK
>> + if (!info->commit && info->mark.mask) {
>> + OVS_NLERR(log,
>> + "Setting conntrack mark requires 'commit' flag.");
>> + return -EINVAL;
>> + }
>> +#endif
>> +#ifdef CONFIG_NF_CONNTRACK_LABELS
>> + if (!info->commit && labels_nonzero(&info->labels.mask)) {
>> + OVS_NLERR(log,
>> + "Setting conntrack labels requires 'commit'
>> flag.");
>> + return -EINVAL;
>> + }
>> +#endif
>
> I'm of mixed minds about this, but I lean towards agreeing with it. On
> one hand, it's applying more restrictions on an otherwise fairly loose
> interface and if anyone is relying on this behaviour then it would be
> surprising to have this restriction introduced. On the other hand, it
> doesn't make a lot of sense to set a label/mark but not to commit the
> connection. As you say, the behaviour isn't exactly consistent in that
> case today anyway: If there was a flow with
> actions=ct(mark=foo),recirc() followed by a userspace upcall, then the
> mark would be reflected in the flow key but not saved to any persisted
> connection. A subsequent ct(commit) after upcall wouldn't persist it,
> either. However if there were two flows already in the datapath to do
> this, then it /would/ be persisted. Restricting the mark/labels
> modification to only if you have the "commit" flag would address that
> consistency issue. The OVS userspace enforcing this constraint also
> hints that this was an unintentional omission from kernel validation.
I separated this out to the v2 patch 2/2.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
