I'd appreciate a review for this because I'd like to get it into OVS
2.4. The "conjunction match" feature is new in 2.4 and I'd like to have
it have this feature there.
On Mon, Jun 15, 2015 at 01:58:32PM -0700, Ben Pfaff wrote:
> 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 <sevi...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
