On Thu, Oct 24, 2013 at 02:08:24PM -0700, Jarno Rajahalme wrote:
>
>
> On Oct 21, 2013, at 3:52 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > Commit e3b5693319c (Fix table checking for goto table instruction.) moved
> > action checking into modify_flows__(), for good reason, but as a side
> > effect made modify_flows__() abandon and never commit the ofopgroup that it
> > started, if action checking failed. This commit fixes the problem.
> >
> > The following commands, run under "make sandbox", illustrate the problem.
> > Without this change, the final command hangs because the barrier request
> > that ovs-ofctl sends never gets a response (because barriers wait for all
> > ofopgroups to complete, which never happens). With this commit, the
> > commands complete quickly:
> >
> > ovs-vsctl add-br br0
> > ovs-vsctl set bridge br0
> > protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
> > ovs-ofctl add-flow -O OpenFlow11 br0
> > table=1,action=mod_tp_dst:79,goto_table:2
> > ovs-ofctl add-flow -O OpenFlow11 br0
> > table=1,action=mod_tp_dst:79,goto_table:1
> >
> > Reported-by: Jarno Rajahalme <jrajaha...@vmware.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > ofproto/ofproto.c | 19 ++++++++++++-------
> > tests/ofproto.at | 26 ++++++++++++++++++++++++++
> > 2 files changed, 38 insertions(+), 7 deletions(-)
> >
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index f67e1fb..8dba732 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -4041,6 +4041,18 @@ modify_flows__(struct ofproto *ofproto, struct
> > ofconn *ofconn,
> > enum ofperr error;
> > size_t i;
> >
> > + /* Verify actions before we start to modify any rules, to avoid partial
> > + * flow table modifications. */
> > + for (i = 0; i < rules->n; i++) {
> > + struct rule *rule = rules->rules[i];
> > +
> > + error = ofpacts_check(fm->ofpacts, fm->ofpacts_len,
> > &fm->match.flow,
> > + u16_to_ofp(ofproto->max_ports),
> > rule->table_id);
> > + if (error) {
> > + return error;
> > + }
> > + }
> > +
>
> This fixes the problem I had, thank you!
>
> While we are at this, we should use ofproto_check_ofpacts() instead
> and maybe avoid repeating the same check over and over again. How
> about this incremental:
Can we really avoid repeating the check? Since I proposed this
change, ofpacts_check() now checks consistency of the flow and the
actions, and since the flows vary among the rules that we are
checking, I imagine that some of them could be inconsistent within a
single table, even if others are not.
I agree that we should use ofproto_check_ofpacts() here. That seems
out of scope for a single commit, though, so I'll send out a separate
patch.
I applied this patch more or less as-is, following rebase.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
