ovn: Implement action to exchange two fields.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Justin Pettit <jpettit@nicira.com>

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 0a0158a..4170fc5 100644 (file)
--- 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 exchange 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_EXCHANGE
+            || 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..8270b82 100644 (file)
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -2619,13 +2619,26 @@ expr_is_normalized(const struct expr *expr)
 \f
 /* Action parsing helper. */
 
+/* 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). */
 static bool
-expand_symbol(struct expr_context *ctx, struct expr_field *f,
-              struct expr **prereqsp)
+expand_symbol(struct expr_context *ctx, bool rw, bool exchange,
+              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, exchange ? "exchange" : "assignment");
         return false;
     }
 
@@ -2663,9 +2676,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 +2709,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 exchange = lexer_match(ctx->lexer, LEX_T_EXCHANGE);
+    if (!exchange && !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, exchange, &dst, &prereqs)) {
         goto exit;
     }
 
-    if (ctx->lexer->token.type == LEX_T_ID) {
+    if (exchange || 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, exchange, exchange, &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 (exchange) {
+                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 (exchange) {
+                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,
+                       exchange ? "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 (exchange) {
+            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 +2825,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 "exchange" 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..2ffcfb9 100644 (file)
--- 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_EXCHANGE:
+        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_EXCHANGE;
+            p += 2;
         } else {
             token->type = LEX_T_LT;
         }

diff --git a/ovn/lib/lex.h b/ovn/lib/lex.h
index df4db2d..b5828a2 100644 (file)
--- 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_EXCHANGE,             /* <-> */
 };
 
 /* Subtype for LEX_T_INTEGER and LEX_T_MASKED_INTEGER tokens.

diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index ce69962..4609c2d 100644 (file)
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -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 7622e93..9bf8022 100644 (file)
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -81,7 +81,8 @@ 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.")
 
-(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; =
+# Test that operators are tokenized as expected, even without white space.
+(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || .. , ; = <->
 & => error("`&' is only valid as part of `&&'.")
 | => error("`|' is only valid as part of `||'.")
 
@@ -448,8 +449,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.
 
@@ -480,6 +486,12 @@ 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

diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index c806068..774ebdf 100644 (file)
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -102,7 +102,7 @@ test_lex(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
     ds_init(&input);
     ds_init(&output);
-    while (!ds_get_line(&input, stdin)) {
+    while (!ds_get_test_line(&input, stdin)) {
         struct lexer lexer;
 
         lexer_init(&lexer, ds_cstr(&input));