On Fri, Oct 25, 2013 at 01:44:17PM -0700, Jarno Rajahalme wrote:
>
> On Oct 25, 2013, at 1:32 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > 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.
> >
>
> It seems to me that we are checking the new actions against the new flow
> (both from the new flow mod) in the context of the old rule's table_id, i.e.
> the check calls do not really vary by the rule (other than rule's table id)
> at all.
I don't understand yet.
Let me provide an example. Suppose we do a flow_mod that changes all
of the actions in table 0 from whatever they were previously to
"mod_tp_src:80". If the first rule whose change we validate in that
table satisfies the prerequisites for mod_tp_src, but other rules in
the table do not satisfy the prerequisites, then I think that we would
allow the flow_mod to go through without noticing the problem.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
