[ovs-dev] [PATCH] Do not perform validation of learn actions during parsing
Simon Horman
horms at verge.net.au
Thu May 2 09:06:35 UTC 2013
Do not perform validation of learn actions during parsing.
I believe this is consistent with the handling of all other actions.
Verification of all actions, including learn actions, occurs separately
in ofpact_check__().
This the code portion of this patch is larger than might otherwise be
expected as the flow argument of learn_parse() is now unused and thus
removed. This propagates up the call-chain some way.
This was suggested by Jesse Gross in response to an enhancement
I made to the validation performed during parsing learn actions
to allow it to correctly account for changes to the dl_type
due to MPLS push and pop actions.
Tests have also been updated to use ovs-ofctl add-flow* instead
of ovs-ofctl parse-flow to to allow verification occur using
ofpact_check__().
Cc: Jesse Gross <jesse at nicira.com>
Signed-off-by: Simon Horman <horms+renesas at verge.net.au>
---
lib/learn.c | 25 +----------
lib/learn.h | 2 +-
lib/ofp-parse.c | 20 ++++-----
tests/learn.at | 126 ++++++++++++++++++++++++++++++++++++--------------------
4 files changed, 93 insertions(+), 80 deletions(-)
diff --git a/lib/learn.c b/lib/learn.c
index b9bbc97..ab403be 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -512,14 +512,13 @@ learn_parse_spec(const char *orig, char *name, char *value,
*
* Modifies 'arg'. */
void
-learn_parse(char *arg, const struct flow *flow, struct ofpbuf *ofpacts)
+learn_parse(char *arg, struct ofpbuf *ofpacts)
{
char *orig = xstrdup(arg);
char *name, *value;
struct ofpact_learn *learn;
struct match match;
- enum ofperr error;
learn = ofpact_put_LEARN(ofpacts);
learn->idle_timeout = OFP_FLOW_PERMANENT;
@@ -556,21 +555,6 @@ learn_parse(char *arg, const struct flow *flow, struct ofpbuf *ofpacts)
learn_parse_spec(orig, name, value, spec);
- /* Check prerequisites. */
- if (spec->src_type == NX_LEARN_SRC_FIELD
- && flow && !mf_are_prereqs_ok(spec->src.field, flow)) {
- ovs_fatal(0, "%s: cannot specify source field %s because "
- "prerequisites are not satisfied",
- orig, spec->src.field->name);
- }
- if ((spec->dst_type == NX_LEARN_DST_MATCH
- || spec->dst_type == NX_LEARN_DST_LOAD)
- && !mf_are_prereqs_ok(spec->dst.field, &match.flow)) {
- ovs_fatal(0, "%s: cannot specify destination field %s because "
- "prerequisites are not satisfied",
- orig, spec->dst.field->name);
- }
-
/* Update 'match' to allow for satisfying destination
* prerequisites. */
if (spec->src_type == NX_LEARN_SRC_IMMEDIATE
@@ -581,13 +565,6 @@ learn_parse(char *arg, const struct flow *flow, struct ofpbuf *ofpacts)
}
ofpact_update_len(ofpacts, &learn->ofpact);
- /* In theory the above should have caught any errors, but... */
- if (flow) {
- error = learn_check(learn, flow);
- if (error) {
- ovs_fatal(0, "%s: %s", orig, ofperr_to_string(error));
- }
- }
free(orig);
}
diff --git a/lib/learn.h b/lib/learn.h
index adf597e..8ea8dcc 100644
--- a/lib/learn.h
+++ b/lib/learn.h
@@ -39,7 +39,7 @@ void learn_to_nxast(const struct ofpact_learn *, struct ofpbuf *openflow);
void learn_execute(const struct ofpact_learn *, const struct flow *,
struct ofputil_flow_mod *, struct ofpbuf *ofpacts);
-void learn_parse(char *, const struct flow *, struct ofpbuf *ofpacts);
+void learn_parse(char *, struct ofpbuf *ofpacts);
void learn_format(const struct ofpact_learn *, struct ds *);
#endif /* learn.h */
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 295c038..fce0765 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -418,7 +418,7 @@ parse_sample(struct ofpbuf *b, char *arg)
}
static void
-parse_named_action(enum ofputil_action_code code, const struct flow *flow,
+parse_named_action(enum ofputil_action_code code,
char *arg, struct ofpbuf *ofpacts)
{
struct ofpact_tunnel *tunnel;
@@ -579,7 +579,7 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
NOT_REACHED();
case OFPUTIL_NXAST_LEARN:
- learn_parse(arg, flow, ofpacts);
+ learn_parse(arg, ofpacts);
break;
case OFPUTIL_NXAST_EXIT:
@@ -634,12 +634,12 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
}
static bool
-str_to_ofpact__(const struct flow *flow, char *pos, char *act, char *arg,
+str_to_ofpact__(char *pos, char *act, char *arg,
struct ofpbuf *ofpacts, int n_actions)
{
int code = ofputil_action_code_from_name(act);
if (code >= 0) {
- parse_named_action(code, flow, arg, ofpacts);
+ parse_named_action(code, arg, ofpacts);
} else if (!strcasecmp(act, "drop")) {
if (n_actions) {
ovs_fatal(0, "Drop actions must not be preceded by other "
@@ -662,7 +662,7 @@ str_to_ofpact__(const struct flow *flow, char *pos, char *act, char *arg,
}
static void
-str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
+str_to_ofpacts(char *str, struct ofpbuf *ofpacts)
{
char *pos, *act, *arg;
enum ofperr error;
@@ -671,7 +671,7 @@ str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
pos = str;
n_actions = 0;
while (ofputil_parse_key_value(&pos, &act, &arg)) {
- if (!str_to_ofpact__(flow, pos, act, arg, ofpacts, n_actions)) {
+ if (!str_to_ofpact__(pos, act, arg, ofpacts, n_actions)) {
break;
}
n_actions++;
@@ -729,7 +729,7 @@ parse_named_instruction(enum ovs_instruction_type type,
}
static void
-str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
+str_to_inst_ofpacts(char *str, struct ofpbuf *ofpacts)
{
char *pos, *inst, *arg;
int type;
@@ -741,7 +741,7 @@ str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
while (ofputil_parse_key_value(&pos, &inst, &arg)) {
type = ofpact_instruction_type_from_name(inst);
if (type < 0) {
- if (!str_to_ofpact__(flow, pos, inst, arg, ofpacts, n_actions)) {
+ if (!str_to_ofpact__(pos, inst, arg, ofpacts, n_actions)) {
break;
}
@@ -1006,7 +1006,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
struct ofpbuf ofpacts;
ofpbuf_init(&ofpacts, 32);
- str_to_inst_ofpacts(&fm->match.flow, act_str, &ofpacts);
+ str_to_inst_ofpacts(act_str, &ofpacts);
fm->ofpacts_len = ofpacts.size;
fm->ofpacts = ofpbuf_steal_data(&ofpacts);
} else {
@@ -1088,7 +1088,7 @@ void
parse_ofpacts(const char *s_, struct ofpbuf *ofpacts)
{
char *s = xstrdup(s_);
- str_to_ofpacts(NULL, s, ofpacts);
+ str_to_ofpacts(s, ofpacts);
free(s);
}
diff --git a/tests/learn.at b/tests/learn.at
index 8f59b63..1b7d618 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -1,62 +1,98 @@
AT_BANNER([learning action])
AT_SETUP([learning action - parsing and formatting])
-AT_DATA([flows.txt], [[
-actions=learn()
-actions=learn(NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], output:NXM_OF_IN_PORT[], load:10->NXM_NX_REG0[5..10])
-actions=learn(table=1,idle_timeout=10, hard_timeout=20, fin_idle_timeout=5, fin_hard_timeout=10, priority=10, cookie=0xfedcba9876543210, in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
-]])
-AT_CHECK([ovs-ofctl parse-flows flows.txt], [0],
-[[usable protocols: any
-chosen protocol: OpenFlow10-table_id
-OFPT_FLOW_MOD (xid=0x1): ADD actions=learn(table=1)
-OFPT_FLOW_MOD (xid=0x2): ADD actions=learn(table=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[],load:0xa->NXM_NX_REG0[5..10])
-OFPT_FLOW_MOD (xid=0x3): ADD actions=learn(table=1,idle_timeout=10,hard_timeout=20,fin_idle_timeout=5,fin_hard_timeout=10,priority=10,cookie=0xfedcba9876543210,in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
-]])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [dnl
+dl_src=10:11:11:11:11:11 actions=learn()
+dl_src=10:11:11:11:11:12 actions=learn(NXM_OF_VLAN_TCI[[0..11]], NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]], output:NXM_OF_IN_PORT[[]], load:10->NXM_NX_REG0[[5..10]])
+dl_src=10:11:11:11:11:13 actions=learn(table=1,idle_timeout=10, hard_timeout=20, fin_idle_timeout=5, fin_hard_timeout=10, priority=10, cookie=0xfedcba9876543210, in_port=99,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],load:NXM_OF_IN_PORT[[]]->NXM_NX_REG1[[16..31]])
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ dl_src=10:11:11:11:11:11 actions=learn(table=1)
+ dl_src=10:11:11:11:11:12 actions=learn(table=1,NXM_OF_VLAN_TCI[[0..11]],NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:NXM_OF_IN_PORT[[]],load:0xa->NXM_NX_REG0[[5..10]])
+ dl_src=10:11:11:11:11:13 actions=learn(table=1,idle_timeout=10,hard_timeout=20,fin_idle_timeout=5,fin_hard_timeout=10,priority=10,cookie=0xfedcba9876543210,in_port=99,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],load:NXM_OF_IN_PORT[[]]->NXM_NX_REG1[[16..31]])
+NXST_FLOW reply:
+])
+OVS_VSWITCHD_STOP
AT_CLEANUP
AT_SETUP([learning action - examples])
-AT_DATA([flows.txt], [[
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_DATA([flows.txt], [
# These are the examples from nicira-ext.h.
-actions=learn(in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
-actions=learn(NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
-table=0 actions=learn(table=1,hard_timeout=10, NXM_OF_VLAN_TCI[0..11],output:NXM_OF_IN_PORT[]), resubmit(,1)
-table=1 priority=0 actions=flood
-]])
-AT_CHECK([ovs-ofctl parse-flows flows.txt], [0],
-[[usable protocols: OXM,OpenFlow10+table_id,NXM+table_id
-chosen protocol: OpenFlow10+table_id
-OFPT_FLOW_MOD (xid=0x1): ADD table:255 actions=learn(table=1,in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31])
-OFPT_FLOW_MOD (xid=0x2): ADD table:255 actions=learn(table=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[])
-OFPT_FLOW_MOD (xid=0x3): ADD actions=learn(table=1,hard_timeout=10,NXM_OF_VLAN_TCI[0..11],output:NXM_OF_IN_PORT[]),resubmit(,1)
-OFPT_FLOW_MOD (xid=0x4): ADD table:1 priority=0 actions=FLOOD
-]])
-AT_CLEANUP
+dl_src=10:11:11:11:11:11 actions=learn(in_port=99,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]], load:NXM_OF_IN_PORT[[]]->NXM_NX_REG1[[16..31]])
+dl_src=10:11:11:11:11:12 actions=learn(NXM_OF_VLAN_TCI[[0..11]], NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:NXM_OF_IN_PORT[[]])
+dl_src=10:11:11:11:11:13 table=0 actions=learn(table=1,hard_timeout=10, NXM_OF_VLAN_TCI[[0..11]],output:NXM_OF_IN_PORT[[]]), resubmit(,1)
+dl_src=10:11:11:11:11:14 table=1 priority=0 actions=flood
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ dl_src=10:11:11:11:11:11 actions=learn(table=1,in_port=99,NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],load:NXM_OF_IN_PORT[[]]->NXM_NX_REG1[[16..31]])
+ dl_src=10:11:11:11:11:12 actions=learn(table=1,NXM_OF_VLAN_TCI[[0..11]],NXM_OF_ETH_DST[[]]=NXM_OF_ETH_SRC[[]],output:NXM_OF_IN_PORT[[]])
+ dl_src=10:11:11:11:11:13 actions=learn(table=1,hard_timeout=10,NXM_OF_VLAN_TCI[[0..11]],output:NXM_OF_IN_PORT[[]]),resubmit(,1)
+ table=1, priority=0,dl_src=10:11:11:11:11:14 actions=FLOOD
+NXST_FLOW reply:
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
AT_SETUP([learning action - satisfied prerequisites])
-AT_DATA([flows.txt],
-[[actions=learn(eth_type=0x800,load:5->NXM_OF_IP_DST[])
-ip,actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1[])
-ip,actions=learn(eth_type=0x800,OXM_OF_IPV4_DST[])
-]])
-AT_CHECK([ovs-ofctl parse-flows flows.txt], [0],
-[[usable protocols: any
-chosen protocol: OpenFlow10-table_id
-OFPT_FLOW_MOD (xid=0x1): ADD actions=learn(table=1,eth_type=0x800,load:0x5->NXM_OF_IP_DST[])
-OFPT_FLOW_MOD (xid=0x2): ADD ip actions=learn(table=1,load:NXM_OF_IP_DST[]->NXM_NX_REG1[])
-OFPT_FLOW_MOD (xid=0x3): ADD ip actions=learn(table=1,eth_type=0x800,NXM_OF_IP_DST[])
-]])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+AT_DATA([flows.txt],[dnl
+dl_src=10:11:11:11:11:11 actions=learn(eth_type=0x800,load:5->NXM_OF_IP_DST[[]])
+dl_src=10:11:11:11:11:12 ip,actions=learn(load:NXM_OF_IP_DST[[]]->NXM_NX_REG1[[]])
+dl_src=10:11:11:11:11:13 ip,actions=learn(eth_type=0x800,OXM_OF_IPV4_DST[[]])
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
+AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl
+ dl_src=10:11:11:11:11:11 actions=learn(table=1,eth_type=0x800,load:0x5->NXM_OF_IP_DST[[]])
+ ip,dl_src=10:11:11:11:11:12 actions=learn(table=1,load:NXM_OF_IP_DST[[]]->NXM_NX_REG1[[]])
+ ip,dl_src=10:11:11:11:11:13 actions=learn(table=1,eth_type=0x800,NXM_OF_IP_DST[[]])
+NXST_FLOW reply:
+])
+OVS_VSWITCHD_STOP
AT_CLEANUP
AT_SETUP([learning action - invalid prerequisites])
-AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:5->NXM_OF_IP_DST[])']],
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([[ovs-ofctl add-flow br0 'actions=learn(load:5->NXM_OF_IP_DST[])']],
[1], [],
- [[ovs-ofctl: load:5->NXM_OF_IP_DST[]: cannot specify destination field ip_dst because prerequisites are not satisfied
-]])
-AT_CHECK([[ovs-ofctl parse-flow 'actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1[])']],
+ [dnl
+OFPT_ERROR (xid=0x2): OFPBAC_BAD_ARGUMENT
+OFPT_FLOW_MOD (xid=0x2):
+(***truncated to 64 bytes from 120***)
+00000000 01 0e 00 78 00 00 00 02-00 38 20 ff 00 00 00 00 |...x.....8 .....|
+00000010 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
+00000020 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
+00000030 00 00 00 00 00 00 00 00-00 00 00 00 00 00 80 00 |................|
+])
+AT_CHECK([[ovs-ofctl add-flow br0 'actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1[])']],
[1], [],
- [[ovs-ofctl: load:NXM_OF_IP_DST[]->NXM_NX_REG1[]: cannot specify source field ip_dst because prerequisites are not satisfied
-]])
+ [dnl
+OFPT_ERROR (xid=0x2): OFPBAC_BAD_ARGUMENT
+OFPT_FLOW_MOD (xid=0x2):
+(***truncated to 64 bytes from 120***)
+00000000 01 0e 00 78 00 00 00 02-00 38 20 ff 00 00 00 00 |...x.....8 .....|
+00000010 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
+00000020 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 |................|
+00000030 00 00 00 00 00 00 00 00-00 00 00 00 00 00 80 00 |................|
+])
+OVS_VSWITCHD_STOP(["/field ip_dst lacks correct prerequisites/d"])
AT_CLEANUP
AT_SETUP([learning action - standard VLAN+MAC learning])
--
1.8.2.1
More information about the dev
mailing list