From 51a903f1f3ef0147711575a3eba9ba26f13d0382 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 26 Jun 2015 08:14:15 -0700 Subject: [PATCH] ofp-actions: Support mixing "conjunction" and "note" actions. It doesn't make sense to mix "conjunction" actions with most other kinds of actions. That's because flows with "conjunction" actions aren't ever actually executed, so any actions mixed up with them would never do anything useful. "note" actions are a little different because they never do anything useful anyway: they are just there to allow a controller to annotate flows. It makes as much sense to annotate a flow with "conjunction" actions as it does to annotate any other flow, so this commit makes this possible. Requested-by: Soner Sevinc Signed-off-by: Ben Pfaff Acked-by: Andy Zhou --- lib/ofp-actions.c | 7 ++++--- ofproto/ofproto.c | 36 ++++++++++++++++++------------------ tests/classifier.at | 17 +++++++++++++++++ utilities/ovs-ofctl.8.in | 5 +++-- 4 files changed, 42 insertions(+), 23 deletions(-) diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index 10e2a92c0..eef3389bb 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -5737,9 +5737,10 @@ 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) { - VLOG_WARN("when conjunction action is present, it must be " - "the only kind of action used (saw '%s' action)", + 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)); return OFPERR_NXBAC_BAD_CONJUNCTION; } diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index a49e5a999..273a07310 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -4405,12 +4405,6 @@ evict_rules_from_table(struct oftable *table) return error; } -static bool -is_conjunction(const struct ofpact *ofpacts, size_t ofpacts_len) -{ - return ofpacts_len > 0 && ofpacts->type == OFPACT_CONJUNCTION; -} - static void get_conjunctions(const struct ofputil_flow_mod *fm, struct cls_conjunction **conjsp, size_t *n_conjsp) @@ -4419,23 +4413,29 @@ get_conjunctions(const struct ofputil_flow_mod *fm, struct cls_conjunction *conjs = NULL; int n_conjs = 0; - if (is_conjunction(fm->ofpacts, fm->ofpacts_len)) { - const struct ofpact *ofpact; - int i; - - n_conjs = 0; - OFPACT_FOR_EACH (ofpact, fm->ofpacts, fm->ofpacts_len) { + const struct ofpact *ofpact; + OFPACT_FOR_EACH (ofpact, fm->ofpacts, fm->ofpacts_len) { + if (ofpact->type == OFPACT_CONJUNCTION) { n_conjs++; + } else if (ofpact->type != OFPACT_NOTE) { + /* "conjunction" may appear with "note" actions but not with any + * other type of actions. */ + ovs_assert(!n_conjs); + break; } + } + if (n_conjs) { + int i = 0; conjs = xzalloc(n_conjs * sizeof *conjs); - i = 0; OFPACT_FOR_EACH (ofpact, fm->ofpacts, fm->ofpacts_len) { - struct ofpact_conjunction *oc = ofpact_get_CONJUNCTION(ofpact); - conjs[i].clause = oc->clause; - conjs[i].n_clauses = oc->n_clauses; - conjs[i].id = oc->id; - i++; + if (ofpact->type == OFPACT_CONJUNCTION) { + struct ofpact_conjunction *oc = ofpact_get_CONJUNCTION(ofpact); + conjs[i].clause = oc->clause; + conjs[i].n_clauses = oc->n_clauses; + conjs[i].id = oc->id; + i++; + } } } diff --git a/tests/classifier.at b/tests/classifier.at index 1e75123c5..3520acd07 100644 --- a/tests/classifier.at +++ b/tests/classifier.at @@ -289,3 +289,20 @@ for src in 0 1 2 3; do done OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([conjunctive match and other actions]) +OVS_VSWITCHD_START +# It's OK to use "conjunction" actions with "note" actions. +AT_CHECK([ovs-ofctl add-flow br0 'actions=conjunction(3,1/2),note:41.42.43.44.45.46']) +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 +]) +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_VSWITCHD_STOP +AT_CLEANUP diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 63c2ecca8..91cb39705 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -1961,8 +1961,9 @@ example, then the flow that matches \fBip_src=\fIb\fR would have two of \fBconjunction\fR actions within a list of actions is not significant. .IP \(bu -A flow with \fBconjunction\fR actions may not have any other actions. -(It would not be useful.) +A flow with \fBconjunction\fR actions may also include \fBnote\fR +actions for annotations, but not any other kind of actions. (They +would not be useful because they would never be executed.) .IP \(bu All of the flows that constitute a conjunctive flow with a given \fIid\fR must have the same priority. (Flows with the same \fIid\fR -- 2.20.1