diff options
| -rw-r--r-- | lib/ofp-actions.c | 90 | ||||
| -rw-r--r-- | tests/classifier.at | 6 |
2 files changed, 59 insertions, 37 deletions
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 405b7c7ef..10026ab5c 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008-2017 Nicira, Inc. + * Copyright (c) 2008-2017, 2019 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -408,7 +408,8 @@ static void pad_ofpat(struct ofpbuf *openflow, size_t start_ofs); static enum ofperr ofpacts_verify(const struct ofpact[], size_t ofpacts_len, uint32_t allowed_ovsinsts, - enum ofpact_type outer_action); + enum ofpact_type outer_action, + char **errorp); static void put_set_field(struct ofpbuf *openflow, enum ofp_version, enum mf_field_id, uint64_t value); @@ -7683,7 +7684,7 @@ ofpacts_pull_openflow_actions__(struct ofpbuf *openflow, ofpacts_tlv_bitmap, ofpacts); if (!error) { error = ofpacts_verify(ofpacts->data, ofpacts->size, allowed_ovsinsts, - outer_action); + outer_action, NULL); } if (error) { ofpbuf_clear(ofpacts); @@ -8344,7 +8345,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf *openflow, } error = ofpacts_verify(ofpacts->data, ofpacts->size, - (1u << N_OVS_INSTRUCTIONS) - 1, 0); + (1u << N_OVS_INSTRUCTIONS) - 1, 0, NULL); exit: if (error) { ofpbuf_clear(ofpacts); @@ -8505,11 +8506,28 @@ ofpact_get_mf_dst(const struct ofpact *ofpact) return NULL; } +static void OVS_PRINTF_FORMAT(2, 3) +verify_error(char **errorp, const char *format, ...) +{ + va_list args; + va_start(args, format); + char *error = xvasprintf(format, args); + va_end(args); + + if (errorp) { + *errorp = error; + } else { + VLOG_WARN("%s", error); + free(error); + } +} + static enum ofperr -unsupported_nesting(enum ofpact_type action, enum ofpact_type outer_action) +unsupported_nesting(enum ofpact_type action, enum ofpact_type outer_action, + char **errorp) { - VLOG_WARN("%s action doesn't support nested action %s", - ofpact_name(outer_action), ofpact_name(action)); + verify_error(errorp, "%s action doesn't support nested action %s", + ofpact_name(outer_action), ofpact_name(action)); return OFPERR_OFPBAC_BAD_ARGUMENT; } @@ -8521,17 +8539,19 @@ field_requires_ct(enum mf_field_id field) /* Apply nesting constraints for actions */ static enum ofperr -ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action) +ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action, + char **errorp) { const struct mf_field *field = ofpact_get_mf_dst(a); if (field && field_requires_ct(field->id) && outer_action != OFPACT_CT) { - VLOG_WARN("cannot set CT fields outside of ct action"); + verify_error(errorp, "cannot set CT fields outside of ct action"); return OFPERR_OFPBAC_BAD_SET_ARGUMENT; } if (a->type == OFPACT_NAT) { if (outer_action != OFPACT_CT) { - VLOG_WARN("Cannot have NAT action outside of \"ct\" action"); + verify_error(errorp, + "Cannot have NAT action outside of \"ct\" action"); return OFPERR_OFPBAC_BAD_SET_ARGUMENT; } return 0; @@ -8543,10 +8563,11 @@ ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action) if (outer_action == OFPACT_CT) { if (!field) { - return unsupported_nesting(a->type, outer_action); + return unsupported_nesting(a->type, outer_action, errorp); } else if (!field_requires_ct(field->id)) { - VLOG_WARN("%s action doesn't support nested modification " - "of %s", ofpact_name(outer_action), field->name); + verify_error(errorp, + "%s action doesn't support nested modification " + "of %s", ofpact_name(outer_action), field->name); return OFPERR_OFPBAC_BAD_ARGUMENT; } } @@ -8566,7 +8587,8 @@ ofpacts_verify_nested(const struct ofpact *a, enum ofpact_type outer_action) * within another action of type 'outer_action'. */ static enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len, - uint32_t allowed_ovsinsts, enum ofpact_type outer_action) + uint32_t allowed_ovsinsts, enum ofpact_type outer_action, + char **errorp) { const struct ofpact *a; enum ovs_instruction_type inst; @@ -8579,17 +8601,17 @@ ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len, if (a->type == OFPACT_CONJUNCTION) { OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) { if (a->type != OFPACT_CONJUNCTION && a->type != OFPACT_NOTE) { - VLOG_WARN("\"conjunction\" actions may be used along with " - "\"note\" but not any other kind of action " - "(such as the \"%s\" action used here)", - ofpact_name(a->type)); + verify_error(errorp, "\"conjunction\" actions may be used " + "along with \"note\" but not any other kind " + "of action (such as the \"%s\" action used " + "here)", ofpact_name(a->type)); return OFPERR_NXBAC_BAD_CONJUNCTION; } } return 0; } - error = ofpacts_verify_nested(a, outer_action); + error = ofpacts_verify_nested(a, outer_action, errorp); if (error) { return error; } @@ -8603,19 +8625,20 @@ ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len, const char *next_name = ovs_instruction_name_from_type(next); if (next == inst) { - VLOG_WARN("duplicate %s instruction not allowed, for OpenFlow " - "1.1+ compatibility", name); + verify_error(errorp, "duplicate %s instruction not allowed, " + "for OpenFlow 1.1+ compatibility", name); } else { - VLOG_WARN("invalid instruction ordering: %s must appear " - "before %s, for OpenFlow 1.1+ compatibility", - next_name, name); + verify_error(errorp, "invalid instruction ordering: " + "%s must appear before %s, " + "for OpenFlow 1.1+ compatibility", + next_name, name); } return OFPERR_OFPBAC_UNSUPPORTED_ORDER; } if (!((1u << next) & allowed_ovsinsts)) { const char *name = ovs_instruction_name_from_type(next); - VLOG_WARN("%s instruction not allowed here", name); + verify_error(errorp, "%s instruction not allowed here", name); return OFPERR_OFPBIC_UNSUP_INST; } @@ -9141,7 +9164,6 @@ ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, bool allow_instructions, enum ofpact_type outer_action) { int prev_inst = -1; - enum ofperr retval; char *key, *value; bool drop = false; char *pos; @@ -9206,13 +9228,15 @@ ofpacts_parse__(char *str, const struct ofpact_parse_params *pp, "or instruction"); } - retval = ofpacts_verify(pp->ofpacts->data, pp->ofpacts->size, - (allow_instructions - ? (1u << N_OVS_INSTRUCTIONS) - 1 - : 1u << OVSINST_OFPIT11_APPLY_ACTIONS), - outer_action); - if (retval) { - return xstrdup("Incorrect instruction ordering"); + char *error = NULL; + ofpacts_verify(pp->ofpacts->data, pp->ofpacts->size, + (allow_instructions + ? (1u << N_OVS_INSTRUCTIONS) - 1 + : ((1u << OVSINST_OFPIT11_APPLY_ACTIONS) + | (1u << OVSINST_OFPIT13_METER))), + outer_action, &error); + if (error) { + return error; } return NULL; diff --git a/tests/classifier.at b/tests/classifier.at index 86f872db6..88818618b 100644 --- a/tests/classifier.at +++ b/tests/classifier.at @@ -297,12 +297,10 @@ AT_CHECK([ovs-ofctl add-flow br0 'actions=conjunction(3,1/2),note:41.42.43.44.45 AT_CHECK([ovs-ofctl add-flow br0 'actions=note:41.42.43.44.45.46,conjunction(3,1/2)']) # It's not OK to use "conjunction" actions with other types of actions. AT_CHECK([ovs-ofctl '-vPATTERN:console:%c|%p|%m' add-flow br0 'actions=output:1,conjunction(3,1/2)'], [1], [], [dnl -ofp_actions|WARN|"conjunction" actions may be used along with "note" but not any other kind of action (such as the "output" action used here) -ovs-ofctl: Incorrect instruction ordering +ovs-ofctl: "conjunction" actions may be used along with "note" but not any other kind of action (such as the "output" action used here) ]) AT_CHECK([ovs-ofctl '-vPATTERN:console:%c|%p|%m' add-flow br0 'actions=conjunction(3,1/2),output:1'], [1], [], [dnl -ofp_actions|WARN|"conjunction" actions may be used along with "note" but not any other kind of action (such as the "output" action used here) -ovs-ofctl: Incorrect instruction ordering +ovs-ofctl: "conjunction" actions may be used along with "note" but not any other kind of action (such as the "output" action used here) ]) OVS_VSWITCHD_STOP AT_CLEANUP |