[ovs-dev] [PATCH] ofp-actions: Allow write_metadata instruction before goto_table instruction.

Ben Pfaff blp at nicira.com
Thu Jan 3 17:03:50 UTC 2013


On Thu, Jan 03, 2013 at 06:41:37AM +0000, Jing Ai wrote:
> In the current codes, when running "ovs-ofctl add-flow br0 <match
> conditions>,actions=write_metadata:<64-bit
> metadata>,goto_table:<table_id>", it gives the following
> error:2012-12-29T00:23:23Z|00001|ofp_actions|WARN|write_metadata
> instruction must be specified after other
> instructions/actionsovs-ofctl: Incorrect instruction orderingAccording
> to OpenFlow spec 1.2, only goto_table instruction could go after
> write_metadata instruction. This patch intends to address the above
> issue.

It doesn't generalize enough to all the possible order violations.
Here's a more general version.  Will you please verify that it also
solves the problem you see?

Thanks,

Ben.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 3 Jan 2013 09:02:52 -0800
Subject: [PATCH] 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 at hotmail.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 AUTHORS              |    1 +
 lib/ofp-actions.c    |   38 ++++++++++++++++++++++++++------------
 tests/ofp-actions.at |   37 ++++++++++++++++++++++++++++++++-----
 3 files changed, 59 insertions(+), 17 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index 5d34fbe..060fc19 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -133,6 +133,7 @@ Janis Hamme             janis.hamme at student.kit.edu
 Jari Sundell            sundell.software at gmail.com
 Jed Daniels             openvswitch at jeddaniels.com
 Jeongkeun Lee           jklee at hp.com
+Jing Ai                 ai_jing2000 at hotmail.com
 Joan Cirer              joan at ev0.net
 John Galgay             john at galgay.net
 Kevin Mancuso           kevin.mancuso at rackspace.com
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6468cac..37205b2 100644
--- 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,37 @@ 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 = 0;
 
     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
--- 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.
-- 
1.7.10.4




More information about the dev mailing list