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.
Jarno
> 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
