From dd43a558597bbf99a62541bc77e85f86f63e2f12 Mon Sep 17 00:00:00 2001 From: Simon Horman Date: Wed, 8 May 2013 10:50:15 +0900 Subject: [PATCH] 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 Cc: Ben Pfaff Signed-off-by: Simon Horman Signed-off-by: Ben Pfaff --- lib/learn.c | 25 +------------------------ lib/learn.h | 2 +- lib/ofp-parse.c | 20 ++++++++++---------- tests/learn.at | 12 +++++++----- 4 files changed, 19 insertions(+), 40 deletions(-) diff --git a/lib/learn.c b/lib/learn.c index b9bbc973e..ab403bee5 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 adf597e92..8ea8dcca6 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 970fd5361..1c5c761e5 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; } @@ -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 8f59b63cc..5eb3e5d27 100644 --- 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 -- 2.43.0