> On Jul 28, 2015, at 8:44 AM, Ben Pfaff <[email protected]> wrote:
>
I missed this in my review yesterday on ovn-architure.7.xml:
> + OpenFlow tables 32 through 47 implement the <code>output</code>
> action
> + in the the logical ingress pipeline. Specifically, table 32 handles
This introduces two "the"s.
Just some minor things on physical_run():
> + /* Table 64, Priority 50.
> + * =======================
> *
> + * Deliver the packet to the local vif. */
> ...
> + ofctrl_add_flow(flow_table, 64, 100, &match, &ofpacts);
This seems to add the flow at priority 50, not 100.
> + const struct sbrec_multicast_group *mc;
> + SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
> + struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
> + struct match match;
Since this block is adding more flows, it might be nice to add comments like
all the other flows, since it's otherwise easy to miss them.
> + /* Table 34, Priority 0.
> + * =======================
> + *
> + * Resubmit packets that don't output to the ingress port to the logical
> + * egress pipeline. */
> + struct match match;
> + match_init_catchall(&match);
> + ofpbuf_clear(&ofpacts);
> + put_resubmit(48, &ofpacts);
> + ofctrl_add_flow(flow_table, 34, 0, &match, &ofpacts);
The rule definition doesn't match the flows written. It took me a minute to
realize this was because there was a flow at priority 100 that enforced the
ingress and egress defined quite a bit earlier. I think just referencing that
other flow here would help.
I don't need to see a respin for this or the previous part of this review, so:
Acked-by: Justin Pettit <[email protected]>
Thanks again for the code and thorough documentation!
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
