expr: Refactor parsing of assignments and exchanges.

As written, it was difficult for the OVN logical action code to add support
for new actions of the form "dst = ...", because the code to parse the left
side of the assignment was a monolithic part of the expr library.  This
commit refactors the code division so that an upcoming patch can support a
new "dst = func(args);" kind of action.

Signed-off-by: Ben Pfaff <blp@ovn.org>

3 files changed, 162 insertions, 97 deletions

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 4a486a002..6a79a5e22 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -106,19 +106,33 @@ action_syntax_error(struct action_context *ctx, const char *message, ...)
 static void
 parse_set_action(struct action_context *ctx)
 {
-    struct expr *prereqs;
+    struct expr *prereqs = NULL;
+    struct expr_field dst;
     char *error;
 
-    error = expr_parse_assignment(ctx->lexer, ctx->ap->symtab,
-                                  ctx->ap->lookup_port, ctx->ap->aux,
-                                  ctx->ofpacts, &prereqs);
+    error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &dst);
+    if (!error) {
+        if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
+            error = expr_parse_exchange(ctx->lexer, &dst, ctx->ap->symtab,
+                                        ctx->ap->lookup_port, ctx->ap->aux,
+                                        ctx->ofpacts, &prereqs);
+        } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
+            error = expr_parse_assignment(
+                ctx->lexer, &dst, ctx->ap->symtab, ctx->ap->lookup_port,
+                ctx->ap->aux, ctx->ofpacts, &prereqs);
+        } else {
+            action_syntax_error(ctx, "expecting `=' or `<->'");
+        }
+        if (!error) {
+            ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
+        }
+    }
+
     if (error) {
+        expr_destroy(prereqs);
         action_error(ctx, "%s", error);
         free(error);
-        return;
     }
-
-    ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
 }
 
 static void
@@ -282,19 +296,23 @@ static bool
 action_parse_field(struct action_context *ctx,
                    int n_bits, struct mf_subfield *sf)
 {
-    struct expr *prereqs;
+    struct expr_field field;
     char *error;
 
-    error = expr_parse_field(ctx->lexer, n_bits, false, ctx->ap->symtab, sf,
-                             &prereqs);
-    if (error) {
-        action_error(ctx, "%s", error);
-        free(error);
-        return false;
+    error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &field);
+    if (!error) {
+        struct expr *prereqs;
+        error = expr_expand_field(ctx->lexer, ctx->ap->symtab,
+                                  &field, n_bits, false, sf, &prereqs);
+        if (!error) {
+            ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
+            return true;
+        }
     }
 
-    ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
-    return true;
+    action_error(ctx, "%s", error);
+    free(error);
+    return false;
 }
 
 static void
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 7ff9538cf..31b94d81c 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -438,15 +438,6 @@ struct expr_constant_set {
     bool in_curlies;              /* Whether the constants were in {}. */
 };
 
-/* A reference to a symbol or a subfield of a symbol.
- *
- * For string fields, ofs and n_bits are 0. */
-struct expr_field {
-    const struct expr_symbol *symbol; /* The symbol. */
-    int ofs;                          /* Starting bit offset. */
-    int n_bits;                       /* Number of bits. */
-};
-
 /* Context maintained during expr_parse(). */
 struct expr_context {
     struct lexer *lexer;        /* Lexer for pulling more tokens. */
@@ -2698,49 +2689,44 @@ 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,
+static char * OVS_WARN_UNUSED_RESULT
+parse_assignment(struct lexer *lexer, struct expr_field *dst,
+                 const struct shash *symtab, bool exchange,
                  bool (*lookup_port)(const void *aux, const char *port_name,
                                      unsigned int *portp),
-                 const void *aux, struct ofpbuf *ofpacts)
+                 const void *aux, struct ofpbuf *ofpacts,
+                 struct expr **prereqsp)
 {
+    struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
     struct expr *prereqs = NULL;
 
     /* Parse destination and do basic checking. */
-    struct expr_field dst;
-    if (!parse_field(ctx, &dst)) {
-        goto exit;
-    }
-    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, true, &dst, &prereqs)) {
+    const struct expr_symbol *orig_dst = dst->symbol;
+    if (!expand_symbol(&ctx, true, dst, &prereqs)) {
         goto exit;
     }
 
-    if (exchange || 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)) {
+        if (!parse_field(&ctx, &src)) {
             goto exit;
         }
         const struct expr_symbol *orig_src = src.symbol;
-        if (!expand_symbol(ctx, exchange, &src, &prereqs)) {
+        if (!expand_symbol(&ctx, exchange, &src, &prereqs)) {
             goto exit;
         }
 
-        if ((dst.symbol->width != 0) != (src.symbol->width != 0)) {
+        if ((dst->symbol->width != 0) != (src.symbol->width != 0)) {
             if (exchange) {
-                expr_error(ctx,
+                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).",
+                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",
@@ -2749,20 +2735,20 @@ parse_assignment(struct expr_context *ctx,
             goto exit;
         }
 
-        if (dst.n_bits != src.n_bits) {
+        if (dst->n_bits != src.n_bits) {
             if (exchange) {
-                expr_error(ctx,
+                expr_error(&ctx,
                            "Can't exchange %d-bit field with %d-bit field.",
-                           dst.n_bits, src.n_bits);
+                           dst->n_bits, src.n_bits);
             } else {
-                expr_error(ctx,
+                expr_error(&ctx,
                            "Can't assign %d-bit value to %d-bit destination.",
-                           src.n_bits, dst.n_bits);
+                           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 "
+        } 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 "
                        "%s.", orig_dst->name, orig_src->name,
                        exchange ? "exchange" : "assignment");
             goto exit;
@@ -2770,38 +2756,38 @@ parse_assignment(struct expr_context *ctx,
 
         if (exchange) {
             init_stack_action(&src, ofpact_put_STACK_PUSH(ofpacts));
-            init_stack_action(&dst, 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));
+            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);
+            mf_subfield_from_expr_field(dst, &move->dst);
         }
     } else {
         struct expr_constant_set cs;
-        if (!parse_constant_set(ctx, &cs)) {
+        if (!parse_constant_set(&ctx, &cs)) {
             goto exit;
         }
 
-        if (!type_check(ctx, &dst, &cs)) {
+        if (!type_check(&ctx, dst, &cs)) {
             goto exit_destroy_cs;
         }
         if (cs.in_curlies) {
-            expr_error(ctx, "Assignments require a single value.");
+            expr_error(&ctx, "Assignments require a single value.");
             goto exit_destroy_cs;
         }
 
         union expr_constant *c = cs.values;
         struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
-        sf->field = dst.symbol->field;
-        if (dst.symbol->width) {
-            mf_subvalue_shift(&c->value, dst.ofs);
+        sf->field = dst->symbol->field;
+        if (dst->symbol->width) {
+            mf_subvalue_shift(&c->value, dst->ofs);
             if (!c->masked) {
                 memset(&c->mask, 0, sizeof c->mask);
-                bitwise_one(&c->mask, sizeof c->mask, dst.ofs, dst.n_bits);
+                bitwise_one(&c->mask, sizeof c->mask, dst->ofs, dst->n_bits);
             } else {
-                mf_subvalue_shift(&c->mask, dst.ofs);
+                mf_subvalue_shift(&c->mask, dst->ofs);
             }
 
             memcpy(&sf->value,
@@ -2835,65 +2821,102 @@ parse_assignment(struct expr_context *ctx,
     }
 
 exit:
-    return prereqs;
+    if (ctx.error) {
+        expr_destroy(prereqs);
+        prereqs = NULL;
+    }
+    *prereqsp = prereqs;
+    return ctx.error;
 }
 
 /* A helper for actions_parse(), to parse an OVN assignment action in the form
- * "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,
+ * "field = value" or "field = field2" into 'ofpacts'.  The caller must have
+ * already parsed and skipped the left-hand side "field =" and pass in the
+ * field as 'dst'.  Other parameters and return value match those for
+ * actions_parse(). */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_assignment(struct lexer *lexer, struct expr_field *dst,
+                      const struct shash *symtab,
                       bool (*lookup_port)(const void *aux,
                                           const char *port_name,
                                           unsigned int *portp),
                       const void *aux,
                       struct ofpbuf *ofpacts, struct expr **prereqsp)
 {
+    return parse_assignment(lexer, dst, symtab, false, lookup_port, aux,
+                            ofpacts, prereqsp);
+}
+
+/* A helper for actions_parse(), to parse an OVN exchange action in the form
+ * "field1 <-> field2" into 'ofpacts'.  The caller must have already parsed and
+ * skipped the left-hand side "field1 <->" and pass in 'field1'.  Other
+ * parameters and return value match those for actions_parse(). */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_exchange(struct lexer *lexer, struct expr_field *field,
+                    const struct shash *symtab,
+                    bool (*lookup_port)(const void *aux,
+                                        const char *port_name,
+                                        unsigned int *portp),
+                    const void *aux,
+                    struct ofpbuf *ofpacts, struct expr **prereqsp)
+{
+    return parse_assignment(lexer, field, symtab, true, lookup_port, aux,
+                            ofpacts, prereqsp);
+}
+
+/* Parses a field or subfield from 'lexer' into 'field', obtaining field names
+ * from 'symtab'.  Returns NULL if successful, otherwise an error message owned
+ * by the caller. */
+char * OVS_WARN_UNUSED_RESULT
+expr_parse_field(struct lexer *lexer, const struct shash *symtab,
+                 struct expr_field *field)
+{
     struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
-    struct expr *prereqs = parse_assignment(&ctx, lookup_port, aux, ofpacts);
-    if (ctx.error) {
-        expr_destroy(prereqs);
-        prereqs = NULL;
+    if (!parse_field(&ctx, field)) {
+        memset(field, 0, sizeof *field);
     }
-    *prereqsp = prereqs;
     return ctx.error;
 }
 
-char *
-expr_parse_field(struct lexer *lexer, int n_bits, bool rw,
-                 const struct shash *symtab,
-                 struct mf_subfield *sf, struct expr **prereqsp)
+/* Takes 'field', which was presumably parsed by expr_parse_field(), and
+ * converts it into mf_subfield 'sf' and a set of prerequisites in '*prereqsp'.
+ *
+ * 'n_bits' specifies the number of bits that the field must have, and 0
+ * indicates a string field; reports an error if 'field' has a different type
+ * or width.  If 'rw' is true, it is an error if 'field' is read-only.  Uses
+ * 'symtab 'for expanding references and 'lexer' for error reporting.
+ *
+ * Returns NULL if successful, otherwise an error message owned by the
+ * caller. */
+char * OVS_WARN_UNUSED_RESULT
+expr_expand_field(struct lexer *lexer, const struct shash *symtab,
+                  const struct expr_field *orig_field, int n_bits, bool rw,
+                  struct mf_subfield *sf, struct expr **prereqsp)
 {
     struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
     struct expr *prereqs = NULL;
 
-    struct expr_field field;
-    if (!parse_field(&ctx, &field)) {
-        goto exit;
-    }
-
-    const struct expr_field orig_field = field;
+    struct expr_field field = *orig_field;
     if (!expand_symbol(&ctx, rw, &field, &prereqs)) {
         goto exit;
     }
-    ovs_assert(field.n_bits == orig_field.n_bits);
+    ovs_assert(field.n_bits == orig_field->n_bits);
 
     if (n_bits != field.n_bits) {
         if (n_bits && field.n_bits) {
             expr_error(&ctx, "Cannot use %d-bit field %s[%d..%d] "
                        "where %d-bit field is required.",
-                       orig_field.n_bits, orig_field.symbol->name,
-                       orig_field.ofs, orig_field.ofs + orig_field.n_bits - 1,
-                       n_bits);
+                       orig_field->n_bits, orig_field->symbol->name,
+                       orig_field->ofs,
+                       orig_field->ofs + orig_field->n_bits - 1, n_bits);
         } else if (n_bits) {
             expr_error(&ctx, "Cannot use string field %s where numeric "
                        "field is required.",
-                       orig_field.symbol->name);
+                       orig_field->symbol->name);
         } else {
             expr_error(&ctx, "Cannot use numeric field %s where string "
                        "field is required.",
-                       orig_field.symbol->name);
+                       orig_field->symbol->name);
         }
     }
 
diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
index 132778991..351d0ff34 100644
--- a/ovn/lib/expr.h
+++ b/ovn/lib/expr.h
@@ -248,6 +248,15 @@ struct expr_symbol {
     bool must_crossproduct;
 };
 
+/* A reference to a symbol or a subfield of a symbol.
+ *
+ * For string fields, ofs and n_bits are 0. */
+struct expr_field {
+    const struct expr_symbol *symbol; /* The symbol. */
+    int ofs;                          /* Starting bit offset. */
+    int n_bits;                       /* Number of bits. */
+};
+
 struct expr_symbol *expr_symtab_add_field(struct shash *symtab,
                                           const char *name, enum mf_field_id,
                                           const char *prereqs,
@@ -381,14 +390,29 @@ void expr_matches_print(const struct hmap *matches, FILE *);
 
 /* Action parsing helper. */
 
-char *expr_parse_assignment(struct lexer *lexer, const struct shash *symtab,
+char *expr_parse_assignment(struct lexer *lexer, struct expr_field *dst,
+                            const struct shash *symtab,
                             bool (*lookup_port)(const void *aux,
                                                 const char *port_name,
                                                 unsigned int *portp),
-                            const void *aux, struct ofpbuf *ofpacts,
-                            struct expr **prereqsp);
-char *expr_parse_field(struct lexer *, int n_bits, bool rw,
-                       const struct shash *symtab, struct mf_subfield *,
-                       struct expr **prereqsp);
+                            const void *aux,
+                            struct ofpbuf *ofpacts, struct expr **prereqsp)
+    OVS_WARN_UNUSED_RESULT;
+char *expr_parse_exchange(struct lexer *lexer, struct expr_field *dst,
+                          const struct shash *symtab,
+                          bool (*lookup_port)(const void *aux,
+                                              const char *port_name,
+                                              unsigned int *portp),
+                          const void *aux,
+                          struct ofpbuf *ofpacts, struct expr **prereqsp)
+    OVS_WARN_UNUSED_RESULT;
+char *expr_parse_field(struct lexer *lexer, const struct shash *symtab,
+                       struct expr_field *field)
+    OVS_WARN_UNUSED_RESULT;
+char *expr_expand_field(struct lexer *lexer, const struct shash *symtab,
+                        const struct expr_field *orig_field,
+                        int n_bits, bool rw,
+                        struct mf_subfield *sf, struct expr **prereqsp)
+    OVS_WARN_UNUSED_RESULT;
 
 #endif /* ovn/expr.h */