On Wed, Feb 10, 2016 at 03:27:53PM -0800, Jarno Rajahalme wrote:
> With one question below:
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
Thanks.
> > On Feb 9, 2016, at 10:10 PM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > In my opinion, this is less confusing in multiple ways. I now understand
> > the code better myself.
> >
> > Signed-off-by: Ben Pfaff <b...@ovn.org>
> > /* We've let OFPP_NORMAL and the learning action look at the
> > - * packet, so drop it now if forwarding is disabled. */
> > + * packet, so cancel all actions and recirculation if
> > forwarding is
> > + * disabled. */
> > if (in_port && (!xport_stp_forward_state(in_port) ||
> > !xport_rstp_forward_state(in_port))) {
> > - /* Drop all actions added by do_xlate_actions() above. */
> > ctx.odp_actions->size = sample_actions_len;
> > -
> > - /* Undo changes that may have been done for recirculation.
> > */
> > ctx_cancel_recirculation(&ctx);
> > - } else if (ctx.action_set.size) {
> > - /* Translate action set only if not dropping the packet and
> > - * not recirculating. */
> > - if (!exit_recirculates(&ctx)) {
> > - xlate_action_set(&ctx);
> > - }
> > + ofpbuf_clear(&ctx.action_set);
>
> How come we did not do this before?
The previous code is harder than usual to read from the unified diff.
Here it is:
/* We've let OFPP_NORMAL and the learning action look at the
* packet, so drop it now if forwarding is disabled. */
if (in_port && (!xport_stp_forward_state(in_port) ||
!xport_rstp_forward_state(in_port))) {
/* Drop all actions added by do_xlate_actions() above. */
ctx.odp_actions->size = sample_actions_len;
/* Undo changes that may have been done for recirculation. */
if (exit_recirculates(&ctx)) {
ctx.action_set.size = ctx.recirc_action_offset;
ctx.recirc_action_offset = -1;
ctx.last_unroll_offset = -1;
}
} else if (ctx.action_set.size) {
/* Translate action set only if not dropping the packet and
* not recirculating. */
if (!exit_recirculates(&ctx)) {
xlate_action_set(&ctx);
}
}
Looking at it carefully, you can see that previously we didn't need to
clear the action set because we only translated the action set if
forwarding was enabled. That is, the "else" prevented the action set
from being translated. We could still use that method after my change;
that is, I could write it as:
if (in_port && (!xport_stp_forward_state(in_port) ||
!xport_rstp_forward_state(in_port))) {
ctx.odp_actions->size = sample_actions_len;
ctx_cancel_recirculation(&ctx);
} else if (!ctx.recirculating) {
xlate_action_set(&ctx);
}
I find the "new" way that I'm suggesting with this patch marginally
easier to understand. To me, it is really clear from the new code that
we're canceling everything.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
