[ovs-dev] [PATCH] ofproto: Avoid abandoning an ofopgroup without committing it.

[Ben Pfaff]

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 <jrajahalme at vmware.com>
Signed-off-by: Ben Pfaff <blp at 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;
+        }
+    }
+
     type = fm->command == OFPFC_ADD ? OFOPERATION_REPLACE : OFOPERATION_MODIFY;
     group = ofopgroup_create(ofproto, ofconn, request, fm->buffer_id);
     error = OFPERR_OFPBRC_EPERM;
@@ -4059,13 +4071,6 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn,
             continue;
         }
 
-        /* Verify actions. */
-        error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
-                              u16_to_ofp(ofproto->max_ports), rule->table_id);
-        if (error) {
-            return error;
-        }
-
         actions_changed = !ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
                                          rule->actions->ofpacts,
                                          rule->actions->ofpacts_len);
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 0e1d41b..ffb3f3d 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -275,6 +275,32 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 dump-flows br0 | ofctl_strip], [0], [OFPST_FLO
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto - flow_mod negative test (OpenFlow 1.1)])
+OVS_VSWITCHD_START(
+  [set bridge br0 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13])
+AT_CHECK([ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=mod_tp_dst:79,goto_table:2])
+AT_CHECK([ovs-ofctl add-flow -O OpenFlow11 br0 table=1,action=mod_tp_dst:79,goto_table:1],
+  [1], [], [stderr])
+
+# The output should look like this:
+#
+# OFPT_ERROR (OF1.1) (xid=0x2): OFPBRC_BAD_TABLE_ID
+# OFPT_FLOW_MOD (OF1.1) (xid=0x2):
+# (***truncated to 64 bytes from 160***)
+# 00000000  02 0e 00 a0 00 00 00 02-00 00 00 00 00 00 00 00 |................|
+# 00000010  00 00 00 00 00 00 00 00-01 00 00 00 00 00 80 00 |................|
+# 00000020  ff ff ff ff ff ff ff ff-ff ff ff ff 00 00 00 00 |................|
+# 00000030  00 00 00 58 00 00 00 00-00 00 03 ff 00 00 00 00 |...X............|
+#
+# This 'sed' command captures the error message but drops details.
+AT_CHECK([sed '/truncated/d
+/^000000.0/d' stderr | STRIP_XIDS], [0],
+  [OFPT_ERROR (OF1.1): OFPBRC_BAD_TABLE_ID
+OFPT_FLOW_MOD (OF1.1):
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto - set-field flow_mod commands (NXM)])
 OVS_VSWITCHD_START
 AT_CHECK([ovs-ofctl add-flow br0 ipv6,table=1,in_port=3,actions=drop])
-- 
1.7.10.4