ofp-actions: Fix the check for instruction ordering and duplication.

Open vSwitch enforces that, when instructions appear as Nicira extensions
in OpenFlow 1.0 action lists, the instructions appear in the order that
an OpenFlow 1.1+ switch would execute them and that no duplicates appear.
But the check wasn't general enough, because it had been implemented when
only one instruction was implemented and never later generalized.  This
commit fixes the problem.

One of the tests was actually testing for the wrong behavior that the
check implemented, so this commit corrects that test.  It also updates
another test with updated log messages.

Reported-by: Jing Ai <ai_jing2000@hotmail.com>
Tested-by: Jing Ai <ai_jing2000@hotmail.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/AUTHORS b/AUTHORS
index 5d34fbe..060fc19 100644 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -133,6 +133,7 @@ Janis Hamme             janis.hamme@student.kit.edu
 Jari Sundell            sundell.software@gmail.com
 Jed Daniels             openvswitch@jeddaniels.com
 Jeongkeun Lee           jklee@hp.com
+Jing Ai                 ai_jing2000@hotmail.com
 Joan Cirer              joan@ev0.net
 John Galgay             john@galgay.net
 Kevin Mancuso           kevin.mancuso@rackspace.com

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6468cac..1bc8a9c 100644 (file)
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1153,23 +1153,38 @@ enum ofperr
 ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len)
 {
     const struct ofpact *a;
-    const struct ofpact_metadata *om = NULL;
+    enum ovs_instruction_type inst;
 
+    inst = OVSINST_OFPIT11_APPLY_ACTIONS;
     OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-        if (om) {
-            if (a->type == OFPACT_WRITE_METADATA) {
-                VLOG_WARN("duplicate write_metadata instruction specified");
-                return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
+        enum ovs_instruction_type next;
+
+        if (a->type == OFPACT_CLEAR_ACTIONS) {
+            next = OVSINST_OFPIT11_CLEAR_ACTIONS;
+        } else if (a->type == OFPACT_WRITE_METADATA) {
+            next = OVSINST_OFPIT11_WRITE_METADATA;
+        } else if (a->type == OFPACT_GOTO_TABLE) {
+            next = OVSINST_OFPIT11_GOTO_TABLE;
+        } else {
+            next = OVSINST_OFPIT11_APPLY_ACTIONS;
+        }
+
+        if (inst != OVSINST_OFPIT11_APPLY_ACTIONS && next <= inst) {
+            const char *name = ofpact_instruction_name_from_type(inst);
+            const char *next_name = ofpact_instruction_name_from_type(next);
+
+            if (next == inst) {
+                VLOG_WARN("duplicate %s instruction not allowed, for OpenFlow "
+                          "1.1+ compatibility", name);
             } else {
-                VLOG_WARN("write_metadata instruction must be specified after "
-                          "other instructions/actions");
-                return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
+                VLOG_WARN("invalid instruction ordering: %s must appear "
+                          "before %s, for OpenFlow 1.1+ compatibility",
+                          next_name, name);
             }
+            return OFPERR_OFPBAC_UNSUPPORTED_ORDER;
         }
 
-        if (a->type == OFPACT_WRITE_METADATA) {
-            om = (const struct ofpact_metadata *) a;
-        }
+        inst = next;
     }
 
     return 0;

diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 30fcf51..aa51e08 100644 (file)
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -240,12 +240,12 @@ dnl action instead, so parse-ofp11-actions will recognise and drop this action.
 ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff
 
 dnl Write-Metadata duplicated.
-& ofp_actions|WARN|duplicate write_metadata instruction specified
+& ofp_actions|WARN|duplicate write_metadata instruction not allowed, for OpenFlow 1.1+ compatibility
 # bad OF1.1 actions: OFPBAC_UNSUPPORTED_ORDER
 ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff
 
 dnl Write-Metadata in wrong position.
-& ofp_actions|WARN|write_metadata instruction must be specified after other instructions/actions
+& ofp_actions|WARN|invalid instruction ordering: apply_actions must appear before write_metadata, for OpenFlow 1.1+ compatibility
 # bad OF1.1 actions: OFPBAC_UNSUPPORTED_ORDER
 ffff 0020 00002320 0016 000000000000 fedcba9876543210 ffffffffffffffff ffff 0010 00002320 0002 0000 12345678
 
@@ -382,9 +382,36 @@ dnl Write-Metadata duplicated.
 # bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER
 0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00
 
-dnl Write-Metadata in wrong position.
-& ofp_actions|WARN|write_metadata instruction must be specified after other instructions/actions
-# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER
+dnl Write-Metadata in wrong position (OpenFlow 1.1+ disregards the order
+dnl and OVS reorders it to the canonical order)
+# actions=write_metadata:0xfedcba9876543210,goto_table:1
+#  1: 01 -> 02
+#  3: 08 -> 18
+#  4: 01 -> 00
+#  8: 00 -> fe
+#  9: 02 -> dc
+# 10: 00 -> ba
+# 11: 18 -> 98
+# 12: 00 -> 76
+# 13: 00 -> 54
+# 14: 00 -> 32
+# 15: 00 -> 10
+# 16: fe -> ff
+# 17: dc -> ff
+# 18: ba -> ff
+# 19: 98 -> ff
+# 20: 76 -> ff
+# 21: 54 -> ff
+# 22: 32 -> ff
+# 23: 10 -> ff
+# 24: ff -> 00
+# 25: ff -> 01
+# 26: ff -> 00
+# 27: ff -> 08
+# 28: ff -> 01
+# 29: ff -> 00
+# 30: ff -> 00
+# 31: ff -> 00
 0001 0008 01 000000 0002 0018 00000000 fedcba9876543210 ffffffffffffffff
 
 dnl Write-Actions not supported yet.