diff mbox

[ovs-dev,v2,3/3] ovn: Implement action to exchange two fields.

Message ID 1443646592-20290-4-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Sept. 30, 2015, 8:56 p.m. UTC
Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/TODO          |  5 ---
 ovn/lib/actions.c |  4 ++-
 ovn/lib/expr.c    | 97 ++++++++++++++++++++++++++++++++++++++-----------------
 ovn/lib/lex.c     |  6 ++++
 ovn/lib/lex.h     |  1 +
 ovn/ovn-sb.xml    | 19 ++++++++---
 tests/ovn.at      | 14 +++++++-
 7 files changed, 105 insertions(+), 41 deletions(-)

Comments

Justin Pettit Oct. 7, 2015, 8:17 p.m. UTC | #1
> On Sep 30, 2015, at 1:56 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> static bool
> -expand_symbol(struct expr_context *ctx, struct expr_field *f,
> -              struct expr **prereqsp)
> +expand_symbol(struct expr_context *ctx, bool rw, bool swap,
> +              struct expr_field *f, struct expr **prereqsp)

I assume "rw" is "read/write".  It might be nice to document it, though.

> {
> +    const struct expr_symbol *orig_symbol = f->symbol;
> +
>     if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) {
> -        expr_error(ctx, "Predicate symbol %s cannot be used in assignment.",
> -                   f->symbol->name);
> +        expr_error(ctx, "Predicate symbol %s cannot be used in %s.",
> +                   f->symbol->name, swap ? "exchange" : "assignment");

In most internal uses, "swap" is used instead of "exchange"; but external uses seem to prefer "exchange".  It's not a big deal, but it can be a bit easier to work through the code if the names are the same.

> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 28b0d2c..3ae99b3 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -806,11 +806,11 @@
>           <p>
>             Sets data or metadata field <var>field1</var> to the value of data
>             or metadata field <var>field2</var>, e.g. <code>reg0 =
> -            ip4.src;</code> to copy <code>ip4.src</code> into
> -            <code>reg0</code>.  To modify only a subset of a field's bits,
> -            specify a subfield for <var>field1</var> or <var>field2</var> or
> -            both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP
> -            from the least-significant bits of <code>reg0</code>.
> +            ip4.src;</code> copies <code>ip4.src</code> into <code>reg0</code>.
> +            To modify only a subset of a field's bits, specify a subfield for
> +            <var>field1</var> or <var>field2</var> or both, e.g. <code>vlan.pcp
> +            = reg0[0..2];</code> copies the least-significant bits of
> +            <code>reg0</code> into the VLAN PCP.

This isn't a big deal, but it seems like it belongs more in the previous patch.

> -(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
> +(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
> & => error("`&' is only valid as part of `&&'.")
> | => error("`|' is only valid as part of `||'.")

Not related to this patch in particular, but it might be worth adding a comment about this test, since it looks odd.

> +ip.proto = reg0[0..7]; => Field ip.proto is not modifiable.

Also not a big deal, but it seems like this test belongs in the previous patch.

It's nice to see push/pop being put to use!

Acked-by: Justin Pettit <jpettit@nicira.com>

--Justin
Ben Pfaff Oct. 7, 2015, 8:49 p.m. UTC | #2
On Wed, Oct 07, 2015 at 01:17:22PM -0700, Justin Pettit wrote:
> 
> > On Sep 30, 2015, at 1:56 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > static bool
> > -expand_symbol(struct expr_context *ctx, struct expr_field *f,
> > -              struct expr **prereqsp)
> > +expand_symbol(struct expr_context *ctx, bool rw, bool swap,
> > +              struct expr_field *f, struct expr **prereqsp)
> 
> I assume "rw" is "read/write".  It might be nice to document it, though.

OK, I added a comment:

/* Expands 'f' repeatedly as long as it has an expansion, that is, as long as
 * it is a subfield or a predicate.  Adds any prerequisites for 'f' to
 * '*prereqs'.
 *
 * If 'rw', verifies that 'f' is a read/write field.
 *
 * 'exchange' should be true if an exchange action is being parsed.  This is
 * only used to improve error message phrasing.
 *
 * Returns true if successful, false if an error was encountered (in which case
 * 'ctx->error' reports the particular error). */

> > {
> > +    const struct expr_symbol *orig_symbol = f->symbol;
> > +
> >     if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) {
> > -        expr_error(ctx, "Predicate symbol %s cannot be used in assignment.",
> > -                   f->symbol->name);
> > +        expr_error(ctx, "Predicate symbol %s cannot be used in %s.",
> > +                   f->symbol->name, swap ? "exchange" : "assignment");
> 
> In most internal uses, "swap" is used instead of "exchange"; but
> external uses seem to prefer "exchange".  It's not a big deal, but it
> can be a bit easier to work through the code if the names are the
> same.

OK, I changed all of them to "exchange".

> > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> > index 28b0d2c..3ae99b3 100644
> > --- a/ovn/ovn-sb.xml
> > +++ b/ovn/ovn-sb.xml
> > @@ -806,11 +806,11 @@
> >           <p>
> >             Sets data or metadata field <var>field1</var> to the value of data
> >             or metadata field <var>field2</var>, e.g. <code>reg0 =
> > -            ip4.src;</code> to copy <code>ip4.src</code> into
> > -            <code>reg0</code>.  To modify only a subset of a field's bits,
> > -            specify a subfield for <var>field1</var> or <var>field2</var> or
> > -            both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP
> > -            from the least-significant bits of <code>reg0</code>.
> > +            ip4.src;</code> copies <code>ip4.src</code> into <code>reg0</code>.
> > +            To modify only a subset of a field's bits, specify a subfield for
> > +            <var>field1</var> or <var>field2</var> or both, e.g. <code>vlan.pcp
> > +            = reg0[0..2];</code> copies the least-significant bits of
> > +            <code>reg0</code> into the VLAN PCP.
> 
> This isn't a big deal, but it seems like it belongs more in the previous patch.

You're right.

I've moved it now.

> > -(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
> > +(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
> > & => error("`&' is only valid as part of `&&'.")
> > | => error("`|' is only valid as part of `||'.")
> 
> Not related to this patch in particular, but it might be worth adding a comment about this test, since it looks odd.

OK, I added a comment.

> > +ip.proto = reg0[0..7]; => Field ip.proto is not modifiable.
> 
> Also not a big deal, but it seems like this test belongs in the previous patch.

Right again, also moved, thanks.

> It's nice to see push/pop being put to use!
> 
> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks for the review, I applied this patch to master.
diff mbox

Patch

diff --git a/ovn/TODO b/ovn/TODO
index 740ea17..b29df75 100644
--- a/ovn/TODO
+++ b/ovn/TODO
@@ -130,11 +130,6 @@  ovn-sb.xml includes a tentative specification for this action.
 IPv6 will probably need an action or actions for ND that is similar to
 the "arp" action, and an action for generating
 
-*** Other actions.
-
-Possibly we'll need to implement "field1 <-> field2;" for swapping
-fields.
-
 *** ovn-controller translation to OpenFlow
 
 The following two translation strategies come to mind.  Some of the
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 0a0158a..935706bc 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -105,6 +105,7 @@  action_syntax_error(struct action_context *ctx, const char *message, ...)
     ctx->error = ds_steal_cstr(&s);
 }
 
+/* Parses an assignment or swap action. */
 static void
 parse_set_action(struct action_context *ctx)
 {
@@ -153,7 +154,8 @@  parse_actions(struct action_context *ctx)
         }
 
         enum lex_type lookahead = lexer_lookahead(ctx->lexer);
-        if (lookahead == LEX_T_EQUALS || lookahead == LEX_T_LSQUARE) {
+        if (lookahead == LEX_T_EQUALS || lookahead == LEX_T_SWAP
+            || lookahead == LEX_T_LSQUARE) {
             parse_set_action(ctx);
         } else if (lexer_match_id(ctx->lexer, "next")) {
             if (ctx->next_table_id) {
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index b7a3f7f..41da0fe 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -2620,12 +2620,14 @@  expr_is_normalized(const struct expr *expr)
 /* Action parsing helper. */
 
 static bool
-expand_symbol(struct expr_context *ctx, struct expr_field *f,
-              struct expr **prereqsp)
+expand_symbol(struct expr_context *ctx, bool rw, bool swap,
+              struct expr_field *f, struct expr **prereqsp)
 {
+    const struct expr_symbol *orig_symbol = f->symbol;
+
     if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) {
-        expr_error(ctx, "Predicate symbol %s cannot be used in assignment.",
-                   f->symbol->name);
+        expr_error(ctx, "Predicate symbol %s cannot be used in %s.",
+                   f->symbol->name, swap ? "exchange" : "assignment");
         return false;
     }
 
@@ -2663,9 +2665,28 @@  expand_symbol(struct expr_context *ctx, struct expr_field *f,
         f->ofs += expansion.ofs;
     }
 
+    if (rw && !f->symbol->field->writable) {
+        expr_error(ctx, "Field %s is not modifiable.", orig_symbol->name);
+        return false;
+    }
+
     return true;
 }
 
+static void
+mf_subfield_from_expr_field(const struct expr_field *f, struct mf_subfield *sf)
+{
+    sf->field = f->symbol->field;
+    sf->ofs = f->ofs;
+    sf->n_bits = f->n_bits ? f->n_bits : f->symbol->field->n_bits;
+}
+
+static void
+init_stack_action(const struct expr_field *f, struct ofpact_stack *stack)
+{
+    mf_subfield_from_expr_field(f, &stack->subfield);
+}
+
 static struct expr *
 parse_assignment(struct expr_context *ctx, const struct simap *ports,
                  struct ofpbuf *ofpacts)
@@ -2677,56 +2698,73 @@  parse_assignment(struct expr_context *ctx, const struct simap *ports,
     if (!parse_field(ctx, &dst)) {
         goto exit;
     }
-    if (!lexer_match(ctx->lexer, LEX_T_EQUALS)) {
+    bool swap = lexer_match(ctx->lexer, LEX_T_SWAP);
+    if (!swap && !lexer_match(ctx->lexer, LEX_T_EQUALS)) {
         expr_syntax_error(ctx, "expecting `='.");
         goto exit;
     }
     const struct expr_symbol *orig_dst = dst.symbol;
-    if (!expand_symbol(ctx, &dst, &prereqs)) {
-        goto exit;
-    }
-    if (!dst.symbol->field->writable) {
-        expr_error(ctx, "Field %s is not modifiable.", orig_dst->name);
+    if (!expand_symbol(ctx, true, swap, &dst, &prereqs)) {
         goto exit;
     }
 
-    if (ctx->lexer->token.type == LEX_T_ID) {
+    if (swap || ctx->lexer->token.type == LEX_T_ID) {
         struct expr_field src;
         if (!parse_field(ctx, &src)) {
             goto exit;
         }
         const struct expr_symbol *orig_src = src.symbol;
-        if (!expand_symbol(ctx, &src, &prereqs)) {
+        if (!expand_symbol(ctx, swap, swap, &src, &prereqs)) {
             goto exit;
         }
 
         if ((dst.symbol->width != 0) != (src.symbol->width != 0)) {
-            expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).",
-                       orig_src->width ? "integer" : "string",
-                       orig_src->name,
-                       orig_dst->width ? "integer" : "string",
-                       orig_dst->name);
+            if (swap) {
+                expr_error(ctx,
+                           "Can't exchange %s field (%s) with %s field (%s).",
+                           orig_dst->width ? "integer" : "string",
+                           orig_dst->name,
+                           orig_src->width ? "integer" : "string",
+                           orig_src->name);
+            } else {
+                expr_error(ctx, "Can't assign %s field (%s) to %s field (%s).",
+                           orig_src->width ? "integer" : "string",
+                           orig_src->name,
+                           orig_dst->width ? "integer" : "string",
+                           orig_dst->name);
+            }
             goto exit;
         }
 
         if (dst.n_bits != src.n_bits) {
-            expr_error(ctx, "Can't assign %d-bit value to %d-bit destination.",
-                       src.n_bits, dst.n_bits);
+            if (swap) {
+                expr_error(ctx,
+                           "Can't exchange %d-bit field with %d-bit field.",
+                           dst.n_bits, src.n_bits);
+            } else {
+                expr_error(ctx,
+                           "Can't assign %d-bit value to %d-bit destination.",
+                           src.n_bits, dst.n_bits);
+            }
             goto exit;
         } else if (!dst.n_bits
                    && dst.symbol->field->n_bits != src.symbol->field->n_bits) {
             expr_error(ctx, "String fields %s and %s are incompatible for "
-                       "assignment.", orig_dst->name, orig_src->name);
+                       "%s.", orig_dst->name, orig_src->name,
+                       swap ? "exchange" : "assignment");
             goto exit;
         }
 
-        struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
-        move->src.field = src.symbol->field;
-        move->src.ofs = src.ofs;
-        move->src.n_bits = src.n_bits ? src.n_bits : src.symbol->field->n_bits;
-        move->dst.field = dst.symbol->field;
-        move->dst.ofs = dst.ofs;
-        move->dst.n_bits = dst.n_bits ? dst.n_bits : dst.symbol->field->n_bits;
+        if (swap) {
+            init_stack_action(&src, ofpact_put_STACK_PUSH(ofpacts));
+            init_stack_action(&dst, ofpact_put_STACK_PUSH(ofpacts));
+            init_stack_action(&src, ofpact_put_STACK_POP(ofpacts));
+            init_stack_action(&dst, ofpact_put_STACK_POP(ofpacts));
+        } else {
+            struct ofpact_reg_move *move = ofpact_put_REG_MOVE(ofpacts);
+            mf_subfield_from_expr_field(&src, &move->src);
+            mf_subfield_from_expr_field(&dst, &move->dst);
+        }
     } else {
         struct expr_constant_set cs;
         if (!parse_constant_set(ctx, &cs)) {
@@ -2776,8 +2814,9 @@  exit:
 }
 
 /* A helper for actions_parse(), to parse an OVN assignment action in the form
- * "field = value" or "field1 = field2" into 'ofpacts'.  The parameters and
- * return value match those for actions_parse(). */
+ * "field = value" or "field1 = field2", or a "swap" action in the form "field1
+ * <-> field2", into 'ofpacts'.  The parameters and return value match those
+ * for actions_parse(). */
 char *
 expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
                       const struct simap *ports,
diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
index 46e86c2..332802e 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -238,6 +238,9 @@  lex_token_format(const struct lex_token *token, struct ds *s)
     case LEX_T_EQUALS:
         ds_put_cstr(s, "=");
         break;
+    case LEX_T_SWAP:
+        ds_put_cstr(s, "<->");
+        break;
     default:
         OVS_NOT_REACHED();
     }
@@ -599,6 +602,9 @@  next:
         if (*p == '=') {
             token->type = LEX_T_LE;
             p++;
+        } else if (*p == '-' && p[1] == '>') {
+            token->type = LEX_T_SWAP;
+            p += 2;
         } else {
             token->type = LEX_T_LT;
         }
diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
index df4db2d..c27b189 100644
--- a/ovn/lib/lex.h
+++ b/ovn/lib/lex.h
@@ -58,6 +58,7 @@  enum lex_type {
     LEX_T_COMMA,                /* , */
     LEX_T_SEMICOLON,            /* ; */
     LEX_T_EQUALS,               /* = */
+    LEX_T_SWAP,                 /* <-> */
 };
 
 /* Subtype for LEX_T_INTEGER and LEX_T_MASKED_INTEGER tokens.
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 28b0d2c..3ae99b3 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -806,11 +806,11 @@ 
           <p>
             Sets data or metadata field <var>field1</var> to the value of data
             or metadata field <var>field2</var>, e.g. <code>reg0 =
-            ip4.src;</code> to copy <code>ip4.src</code> into
-            <code>reg0</code>.  To modify only a subset of a field's bits,
-            specify a subfield for <var>field1</var> or <var>field2</var> or
-            both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN PCP
-            from the least-significant bits of <code>reg0</code>.
+            ip4.src;</code> copies <code>ip4.src</code> into <code>reg0</code>.
+            To modify only a subset of a field's bits, specify a subfield for
+            <var>field1</var> or <var>field2</var> or both, e.g. <code>vlan.pcp
+            = reg0[0..2];</code> copies the least-significant bits of
+            <code>reg0</code> into the VLAN PCP.
           </p>
 
           <p>
@@ -827,6 +827,15 @@ 
             that a logical flow with such an assignment will never be matched.
           </p>
         </dd>
+
+        <dt><code><var>field1</var> &lt;-&gt; <var>field2</var>;</code></dt>
+        <dd>
+          <p>
+            Similar to <code><var>field1</var> = <var>field2</var>;</code>
+            except that the two values are exchanged instead of copied.  Both
+            <var>field1</var> and <var>field2</var> must modifiable.
+          </p>
+        </dd>
       </dl>
 
       <p>
diff --git a/tests/ovn.at b/tests/ovn.at
index 96f190d..4e9e1be 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -81,7 +81,7 @@  ff:ff:ff:ff:ff:ff/fe:ff:ff:ff:ff:ff => error("Value contains unmasked 1-bits.")
 fe:x => error("Invalid numeric constant.")
 00:01:02:03:04:x => error("Invalid numeric constant.")
 
-(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
+(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
 & => error("`&' is only valid as part of `&&'.")
 | => error("`|' is only valid as part of `||'.")
 
@@ -448,8 +448,13 @@  reg0 = reg1; => actions=move:OXM_OF_PKT_REG0[0..31]->OXM_OF_PKT_REG0[32..63], pr
 vlan.pcp = reg0[0..2]; => actions=move:OXM_OF_PKT_REG0[32..34]->NXM_OF_VLAN_TCI[13..15], prereqs=vlan.tci[12]
 reg0[10] = vlan.pcp[1]; => actions=move:NXM_OF_VLAN_TCI[14]->OXM_OF_PKT_REG0[42], prereqs=vlan.tci[12]
 outport = inport; => actions=move:NXM_NX_REG6[]->NXM_NX_REG7[], prereqs=1
+reg0 <-> reg1; => actions=push:OXM_OF_PKT_REG0[0..31],push:OXM_OF_PKT_REG0[32..63],pop:OXM_OF_PKT_REG0[0..31],pop:OXM_OF_PKT_REG0[32..63], prereqs=1
+vlan.pcp <-> reg0[0..2]; => actions=push:OXM_OF_PKT_REG0[32..34],push:NXM_OF_VLAN_TCI[13..15],pop:OXM_OF_PKT_REG0[32..34],pop:NXM_OF_VLAN_TCI[13..15], prereqs=vlan.tci[12]
+reg0[10] <-> vlan.pcp[1]; => actions=push:NXM_OF_VLAN_TCI[14],push:OXM_OF_PKT_REG0[42],pop:NXM_OF_VLAN_TCI[14],pop:OXM_OF_PKT_REG0[42], prereqs=vlan.tci[12]
+outport <-> inport; => actions=push:NXM_NX_REG6[],push:NXM_NX_REG7[],pop:NXM_NX_REG6[],pop:NXM_NX_REG7[], prereqs=1
 # Contradictionary prerequisites (allowed but not useful):
 ip4.src = ip6.src[0..31]; => actions=move:NXM_NX_IPV6_SRC[0..31]->NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd
+ip4.src <-> ip6.src[0..31]; => actions=push:NXM_NX_IPV6_SRC[0..31],push:NXM_OF_IP_SRC[],pop:NXM_NX_IPV6_SRC[0..31],pop:NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd
 
 ## Negative tests.
 
@@ -479,6 +484,13 @@  reg0[0] = vlan.present; => Predicate symbol vlan.present cannot be used in assig
 reg0 = reg1[0..10]; => Can't assign 11-bit value to 32-bit destination.
 inport = reg0; => Can't assign integer field (reg0) to string field (inport).
 inport = big_string; => String fields inport and big_string are incompatible for assignment.
+ip.proto = reg0[0..7]; => Field ip.proto is not modifiable.
+reg0[0] <-> vlan.present; => Predicate symbol vlan.present cannot be used in exchange.
+reg0 <-> reg1[0..10]; => Can't exchange 32-bit field with 11-bit field.
+inport <-> reg0; => Can't exchange string field (inport) with integer field (reg0).
+inport <-> big_string; => String fields inport and big_string are incompatible for exchange.
+ip.proto <-> reg0[0..7]; => Field ip.proto is not modifiable.
+reg0[0..7] <-> ip.proto; => Field ip.proto is not modifiable.
 ]])
 sed 's/ =>.*//' test-cases.txt > input.txt
 sed 's/.* => //' test-cases.txt > expout