Do not perform validation in learn_parse();

I believe this is consistent with the handling of all other action
parsing called from parse_named_action().

Verification of all actions, including learn actions, occurs separately
in ofpact_check__(). It also occurs via in a call to ofpacts_check()
in parse_ofp_str(),

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 implementation 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 check for the less specific messages
generated by the call to ofpacts_check() in parse_ofp_str() which at the
suggestion of Ben Pfaff was added by a prior patch for this purpose.

Cc: Jesse Gross <jesse@nicira.com>
Cc: Ben Pfaff <blp@nicira.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/learn.c b/lib/learn.c
index b9bbc97..ab403be 100644 (file)
--- 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 (file)
--- 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 970fd53..1c5c761 100644 (file)
--- 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;
             }
 
@@ -1007,7 +1007,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
         enum ofperr err;
 
         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);
 
@@ -1096,7 +1096,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..5eb3e5d 100644 (file)
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -50,12 +50,14 @@ AT_CLEANUP
 
 AT_SETUP([learning action - invalid prerequisites])
 AT_CHECK([[ovs-ofctl parse-flow '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
-]])
+  [1], [], [stderr])
+AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
+  [[destination field ip_dst lacks correct prerequisites
+]], [[]])
 AT_CHECK([[ovs-ofctl parse-flow '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
+  [1], [], [stderr])
+AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
+  [[source field ip_dst lacks correct prerequisites
 ]])
 AT_CLEANUP