[ovs-dev] [PATCH] ofp-actions: Support mixing "conjunction" and "note" actions.

[Ben Pfaff]

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 <sevincs at vmware.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/ofp-actions.c        |  7 ++++---
 ofproto/ofproto.c        | 36 +++++++++++++++++++-----------------
 tests/classifier.at      | 17 +++++++++++++++++
 utilities/ovs-ofctl.8.in |  5 +++--
 4 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 10e2a92..eef3389 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 08ba043..2a5d831 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4406,12 +4406,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)
@@ -4420,23 +4414,31 @@ 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;
 
-        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 1e75123..3520acd 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 63c2ecc..91cb397 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.1.3