On Oct 25, 2013, at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote: > 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.
But the old rule is never passed for the check, see: error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow, u16_to_ofp(ofproto->max_ports), rule->table_id); The flow mod comes in with the flow (&fm->match.flow) so the exact same validation is being repeated over and over again, if the rule->table_id remains the same. Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev