On Thu, Jul 7, 2016 at 12:13 PM, Justin Pettit <jpet...@ovn.org> wrote:
>
> > On Jun 30, 2016, at 10:14 PM, Russell Bryant <russ...@ovn.org> wrote:
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 260cc14..d2bddcb 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> >
>
> >
> > @@ -282,20 +299,37 @@
> > </li>
> >
> > <li>
> > - A priority-65535 flow that allows any traffic that has been
> > - committed to the connection tracker (i.e., established flows).
> > + A priority-65535 flow that allows any traffic in the reply
> > + direction for a connection that has been committed to the
> > + connection tracker (i.e., established flows), as long as
> > + the committed flow does not have <code>ct_label[0]=1</code> set.
>
> There are a couple of places where "<code>ct_label[0]=1</code> set", but I
> think it would be clearer to just drop "=1". Especially if you take my
> later suggestion to use a symbolic name for the field.
>
> > @@ -1444,38 +1472,106 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows, struct hmap *ports)
> > ...
> > + ds_put_format(&match, "((ct.new && !ct.est)"
> > + " || (!ct.new && ct.est &&
> !ct.rpl "
> > + "&& ct_label[0] == 1)) "
>
> It might be nice to use a symbolic name, similar to "eth.mcast". That way
> it's meaning is clearer, and if we ever need to change the label, it only
> needs to be done in one place.
>
> > + if (has_stateful) {
> > + /* The implementation of "drop" differs if stateful
> ACLs are in
> > + * use for this datapath. In that case, the actions
> differ
> > + * depending on whether the connection was previously
> committed
> > + * to the connection tracker with ct_commit.
> > + *
> > + * If the packet is not part of an established
> connection, then
> > + * we can simply drop it. */
>
> Minor nit: I think it might be clearer to break the two paragraphs into
> two comments because the first paragraph applies to the entire code block.
>
> > + ds_put_format(&match,
> > + "(!ct.est || (ct.est && ct_label[0] ==
> 1)) "
> > + "&& (%s)",
> > + acl->match);
> > + ovn_lflow_add(lflows, od, stage, acl->priority +
> > + OVN_ACL_PRI_OFFSET, ds_cstr(&match), "drop;");
> > +
> > + /* For an existing connection without ct_label set,
> we've
> > + * encountered a policy change. ACLs previously allowed
> > + * this connection and we committed the connection
> tracking
> > + * entry. Current policy says that we should drop this
> > + * connection. First, we set bit 0 of ct_label to
> indicate
> > + * that this connection is set for deletion. By not
> > + * specifying "next;", we implicitly drop the packet
> after
> > + * updating conntrack state. */
> > +
> > + ds_clear(&match);
>
> I think you can drop the blank line after the comment.
>
> Thanks for implementing this. I apologize for taking so long to review
> it. As penance, I'd be happy to rebase the code if you'd like.
>
> Acked-by: Justin Pettit <jpet...@ovn.org>
>
Thanks for the review!
As for the rebase, I certainly would not be upset if someone rebased it for
me. :-) Guru offered as well, since the conflict came from his LB
series. However, both of you are doing other useful and important work, so
I by no means expect anyone to do it. I'll get it at some point in the
next couple weeks otherwise.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
