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 <b...@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_jing2...@hotmail.com>
Signed-off-by: Ben Pfaff <b...@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.ha...@student.kit.edu
Jari Sundell sundell.softw...@gmail.com
Jed Daniels openvswi...@jeddaniels.com
Jeongkeun Lee jk...@hp.com
+Jing Ai ai_jing2...@hotmail.com
Joan Cirer j...@ev0.net
John Galgay j...@galgay.net
Kevin Mancuso kevin.manc...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
