[ovs-dev] [PATCH v3 4/9] lex: Integrate error handling into struct lexer.

Ben Pfaff blp at ovn.org
Mon Aug 8 18:21:47 UTC 2016


The actions and expr modules had each developed their own error handling
code that were very similar.  Upcoming code needs similar error handling,
so rather than duplicating it again, integrate it into the lexer itself.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 include/ovn/actions.h |   5 +-
 include/ovn/expr.h    |  16 +--
 include/ovn/lex.h     |  12 ++
 ovn/lib/actions.c     | 337 +++++++++++++++++---------------------------------
 ovn/lib/expr.c        | 249 ++++++++++++++-----------------------
 ovn/lib/lex.c         | 109 ++++++++++++++++
 tests/ovn.at          |   4 +-
 7 files changed, 339 insertions(+), 393 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index d24db59..9ff7b27 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -371,9 +371,8 @@ struct ovnact_parse_params {
     uint8_t cur_ltable;         /* 0 <= cur_ltable < n_tables. */
 };
 
-char *ovnacts_parse(struct lexer *, const struct ovnact_parse_params *,
-                    struct ofpbuf *ovnacts, struct expr **prereqsp)
-    OVS_WARN_UNUSED_RESULT;
+bool ovnacts_parse(struct lexer *, const struct ovnact_parse_params *,
+                    struct ofpbuf *ovnacts, struct expr **prereqsp);
 char *ovnacts_parse_string(const char *s, const struct ovnact_parse_params *,
                            struct ofpbuf *ovnacts, struct expr **prereqsp)
     OVS_WARN_UNUSED_RESULT;
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 50cb73f..e1c1886 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -265,9 +265,8 @@ struct expr_field {
     int n_bits;                       /* Number of bits. */
 };
 
-char *expr_field_parse(struct lexer *lexer, const struct shash *symtab,
-                       struct expr_field *field, struct expr **prereqsp)
-    OVS_WARN_UNUSED_RESULT;
+bool expr_field_parse(struct lexer *, const struct shash *symtab,
+                      struct expr_field *, struct expr **prereqsp);
 void expr_field_format(const struct expr_field *, struct ds *);
 
 struct expr_symbol *expr_symtab_add_field(struct shash *symtab,
@@ -365,8 +364,7 @@ expr_from_node(const struct ovs_list *node)
 void expr_format(const struct expr *, struct ds *);
 void expr_print(const struct expr *);
 struct expr *expr_parse(struct lexer *, const struct shash *symtab,
-                        const struct shash *macros,
-                        char **errorp);
+                        const struct shash *macros);
 struct expr *expr_parse_string(const char *, const struct shash *symtab,
                                const struct shash *macros,
                                char **errorp);
@@ -434,9 +432,8 @@ union expr_constant {
     char *string;
 };
 
-char *expr_constant_parse(struct lexer *, const struct expr_field *,
-                          union expr_constant *)
-    OVS_WARN_UNUSED_RESULT;
+bool expr_constant_parse(struct lexer *, const struct expr_field *,
+                         union expr_constant *);
 void expr_constant_format(const union expr_constant *,
                           enum expr_constant_type, struct ds *);
 void expr_constant_destroy(const union expr_constant *,
@@ -450,8 +447,7 @@ struct expr_constant_set {
     bool in_curlies;              /* Whether the constants were in {}. */
 };
 
-char *expr_constant_set_parse(struct lexer *, struct expr_constant_set *cs)
-    OVS_WARN_UNUSED_RESULT;
+bool expr_constant_set_parse(struct lexer *, struct expr_constant_set *);
 void expr_constant_set_format(const struct expr_constant_set *, struct ds *);
 void expr_constant_set_destroy(struct expr_constant_set *cs);
 
diff --git a/include/ovn/lex.h b/include/ovn/lex.h
index 4de48c7..0876d1b 100644
--- a/include/ovn/lex.h
+++ b/include/ovn/lex.h
@@ -124,6 +124,7 @@ struct lexer {
     const char *input;          /* Remaining input (not owned by lexer). */
     const char *start;          /* Start of current token in 'input'. */
     struct lex_token token;     /* Current token (owned by lexer). */
+    char *error;                /* Error message, if any (owned by lexer). */
 };
 
 void lexer_init(struct lexer *, const char *input);
@@ -132,8 +133,19 @@ void lexer_destroy(struct lexer *);
 enum lex_type lexer_get(struct lexer *);
 enum lex_type lexer_lookahead(const struct lexer *);
 bool lexer_match(struct lexer *, enum lex_type);
+bool lexer_force_match(struct lexer *, enum lex_type);
 bool lexer_match_id(struct lexer *, const char *id);
 bool lexer_is_int(const struct lexer *);
 bool lexer_get_int(struct lexer *, int *value);
+bool lexer_force_int(struct lexer *, int *value);
+
+void lexer_force_end(struct lexer *);
+
+void lexer_error(struct lexer *, const char *message, ...)
+    OVS_PRINTF_FORMAT(2, 3);
+void lexer_syntax_error(struct lexer *, const char *message, ...)
+    OVS_PRINTF_FORMAT(2, 3);
+
+char *lexer_steal_error(struct lexer *);
 
 #endif /* ovn/lex.h */
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index eb6ccfa..95ee0c6 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -1,4 +1,4 @@
-/*
+ /*
  * Copyright (c) 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -175,7 +175,6 @@ put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
 struct action_context {
     const struct ovnact_parse_params *pp; /* Parameters. */
     struct lexer *lexer;        /* Lexer for pulling more tokens. */
-    char *error;                /* Error, if any, otherwise NULL. */
     struct ofpbuf *ovnacts;     /* Actions. */
     struct expr *prereqs;       /* Prerequisites to apply to match. */
 };
@@ -183,112 +182,21 @@ struct action_context {
 static bool parse_action(struct action_context *);
 
 static bool
-action_error_handle_common(struct action_context *ctx)
+action_parse_field(struct action_context *ctx,
+                   int n_bits, bool rw, struct expr_field *f)
 {
-    if (ctx->error) {
-        /* Already have an error, suppress this one since the cascade seems
-         * unlikely to be useful. */
-        return true;
-    } else if (ctx->lexer->token.type == LEX_T_ERROR) {
-        /* The lexer signaled an error.  Nothing at the action level
-         * accepts an error token, so we'll inevitably end up here with some
-         * meaningless parse error.  Report the lexical error instead. */
-        ctx->error = xstrdup(ctx->lexer->token.s);
-        return true;
-    } else {
+    if (!expr_field_parse(ctx->lexer, ctx->pp->symtab, f, &ctx->prereqs)) {
         return false;
     }
-}
-
-static void OVS_PRINTF_FORMAT(2, 3)
-action_error(struct action_context *ctx, const char *message, ...)
-{
-    if (action_error_handle_common(ctx)) {
-        return;
-    }
-
-    va_list args;
-    va_start(args, message);
-    ctx->error = xvasprintf(message, args);
-    va_end(args);
-}
-
-static void OVS_PRINTF_FORMAT(2, 3)
-action_syntax_error(struct action_context *ctx, const char *message, ...)
-{
-    if (action_error_handle_common(ctx)) {
-        return;
-    }
-
-    struct ds s;
-
-    ds_init(&s);
-    ds_put_cstr(&s, "Syntax error");
-    if (ctx->lexer->token.type == LEX_T_END) {
-        ds_put_cstr(&s, " at end of input");
-    } else if (ctx->lexer->start) {
-        ds_put_format(&s, " at `%.*s'",
-                      (int) (ctx->lexer->input - ctx->lexer->start),
-                      ctx->lexer->start);
-    }
-
-    if (message) {
-        ds_put_char(&s, ' ');
-
-        va_list args;
-        va_start(args, message);
-        ds_put_format_valist(&s, message, args);
-        va_end(args);
-    }
-    ds_put_char(&s, '.');
-
-    ctx->error = ds_steal_cstr(&s);
-}
-
-static bool
-action_force_match(struct action_context *ctx, enum lex_type t)
-{
-    if (lexer_match(ctx->lexer, t)) {
-        return true;
-    } else {
-        struct lex_token token = { .type = t };
-        struct ds s = DS_EMPTY_INITIALIZER;
-        lex_token_format(&token, &s);
-
-        action_syntax_error(ctx, "expecting `%s'", ds_cstr(&s));
-
-        ds_destroy(&s);
 
+    char *error = expr_type_check(f, n_bits, rw);
+    if (error) {
+        lexer_error(ctx->lexer, "%s", error);
+        free(error);
         return false;
     }
-}
 
-static bool
-action_parse_field(struct action_context *ctx,
-                   int n_bits, bool rw, struct expr_field *f)
-{
-    char *error = expr_field_parse(ctx->lexer, ctx->pp->symtab, f,
-                                   &ctx->prereqs);
-    if (!error) {
-        error = expr_type_check(f, n_bits, rw);
-        if (!error) {
-            return true;
-        }
-    }
-
-    action_error(ctx, "%s", error);
-    free(error);
-    return false;
-}
-
-static bool
-action_get_int(struct action_context *ctx, int *value)
-{
-    bool ok = lexer_get_int(ctx->lexer, value);
-    if (!ok) {
-        action_syntax_error(ctx, "expecting small integer");
-    }
-    return ok;
+    return true;
 }
 
 static bool
@@ -302,7 +210,7 @@ action_parse_port(struct action_context *ctx, uint16_t *port)
             return true;
         }
     }
-    action_syntax_error(ctx, "expecting port number");
+    lexer_syntax_error(ctx->lexer, "expecting port number");
     return false;
 }
 
@@ -350,20 +258,18 @@ static void
 parse_NEXT(struct action_context *ctx)
 {
     if (!ctx->pp->n_tables) {
-        action_error(ctx, "\"next\" action not allowed here.");
+        lexer_error(ctx->lexer, "\"next\" action not allowed here.");
     } else if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
         int ltable;
 
-        if (!action_get_int(ctx, &ltable)) {
-            return;
-        }
-        if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
-            action_syntax_error(ctx, "expecting `)'");
+        if (!lexer_force_int(ctx->lexer, &ltable) ||
+            !lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
             return;
         }
 
         if (ltable >= ctx->pp->n_tables) {
-            action_error(ctx, "\"next\" argument must be in range 0 to %d.",
+            lexer_error(ctx->lexer,
+                        "\"next\" argument must be in range 0 to %d.",
                          ctx->pp->n_tables - 1);
             return;
         }
@@ -373,7 +279,8 @@ parse_NEXT(struct action_context *ctx)
         if (ctx->pp->cur_ltable < ctx->pp->n_tables) {
             ovnact_put_NEXT(ctx->ovnacts)->ltable = ctx->pp->cur_ltable + 1;
         } else {
-            action_error(ctx, "\"next\" action not allowed in last table.");
+            lexer_error(ctx->lexer,
+                        "\"next\" action not allowed in last table.");
         }
     }
 }
@@ -403,14 +310,17 @@ parse_LOAD(struct action_context *ctx, const struct expr_field *lhs)
     size_t ofs = ctx->ovnacts->size;
     struct ovnact_load *load = ovnact_put_LOAD(ctx->ovnacts);
     load->dst = *lhs;
+
     char *error = expr_type_check(lhs, lhs->n_bits, true);
-    if (!error) {
-        error = expr_constant_parse(ctx->lexer, lhs, &load->imm);
-    }
     if (error) {
         ctx->ovnacts->size = ofs;
-        action_error(ctx, "%s", error);
+        lexer_error(ctx->lexer, "%s", error);
         free(error);
+        return;
+    }
+    if (!expr_constant_parse(ctx->lexer, lhs, &load->imm)) {
+        ctx->ovnacts->size = ofs;
+        return;
     }
 }
 
@@ -497,11 +407,7 @@ parse_assignment_action(struct action_context *ctx, bool exchange,
                         const struct expr_field *lhs)
 {
     struct expr_field rhs;
-    char *error = expr_field_parse(ctx->lexer, ctx->pp->symtab, &rhs,
-                                   &ctx->prereqs);
-    if (error) {
-        action_error(ctx, "%s", error);
-        free(error);
+    if (!expr_field_parse(ctx->lexer, ctx->pp->symtab, &rhs, &ctx->prereqs)) {
         return;
     }
 
@@ -509,47 +415,48 @@ parse_assignment_action(struct action_context *ctx, bool exchange,
     const struct expr_symbol *rs = rhs.symbol;
     if ((ls->width != 0) != (rs->width != 0)) {
         if (exchange) {
-            action_error(ctx,
-                         "Can't exchange %s field (%s) with %s field (%s).",
-                         ls->width ? "integer" : "string",
-                         ls->name,
-                         rs->width ? "integer" : "string",
-                         rs->name);
+            lexer_error(ctx->lexer,
+                        "Can't exchange %s field (%s) with %s field (%s).",
+                        ls->width ? "integer" : "string",
+                        ls->name,
+                        rs->width ? "integer" : "string",
+                        rs->name);
         } else {
-            action_error(ctx,
-                         "Can't assign %s field (%s) to %s field (%s).",
-                         rs->width ? "integer" : "string",
-                         rs->name,
-                         ls->width ? "integer" : "string",
-                         ls->name);
+            lexer_error(ctx->lexer,
+                        "Can't assign %s field (%s) to %s field (%s).",
+                        rs->width ? "integer" : "string",
+                        rs->name,
+                        ls->width ? "integer" : "string",
+                        ls->name);
         }
         return;
     }
 
     if (lhs->n_bits != rhs.n_bits) {
         if (exchange) {
-            action_error(ctx, "Can't exchange %d-bit field with %d-bit field.",
-                         lhs->n_bits, rhs.n_bits);
+            lexer_error(ctx->lexer,
+                        "Can't exchange %d-bit field with %d-bit field.",
+                        lhs->n_bits, rhs.n_bits);
         } else {
-            action_error(ctx,
-                         "Can't assign %d-bit value to %d-bit destination.",
-                         rhs.n_bits, lhs->n_bits);
+            lexer_error(ctx->lexer,
+                        "Can't assign %d-bit value to %d-bit destination.",
+                        rhs.n_bits, lhs->n_bits);
         }
         return;
     } else if (!lhs->n_bits &&
                ls->field->n_bits != rs->field->n_bits) {
-        action_error(ctx, "String fields %s and %s are incompatible for "
-                     "%s.", ls->name, rs->name,
-                     exchange ? "exchange" : "assignment");
+        lexer_error(ctx->lexer, "String fields %s and %s are incompatible for "
+                    "%s.", ls->name, rs->name,
+                    exchange ? "exchange" : "assignment");
         return;
     }
 
-    error = expr_type_check(lhs, lhs->n_bits, true);
+    char *error = expr_type_check(lhs, lhs->n_bits, true);
     if (!error) {
         error = expr_type_check(&rhs, rhs.n_bits, true);
     }
     if (error) {
-        action_error(ctx, "%s", error);
+        lexer_error(ctx->lexer, "%s", error);
         free(error);
         return;
     }
@@ -596,7 +503,7 @@ free_EXCHANGE(struct ovnact_move *xchg OVS_UNUSED)
 static void
 parse_DEC_TTL(struct action_context *ctx)
 {
-    action_force_match(ctx, LEX_T_DECREMENT);
+    lexer_force_match(ctx->lexer, LEX_T_DECREMENT);
     ovnact_put_DEC_TTL(ctx->ovnacts);
     add_prerequisite(ctx, "ip");
 }
@@ -624,7 +531,8 @@ static void
 parse_CT_NEXT(struct action_context *ctx)
 {
     if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
-        action_error(ctx, "\"ct_next\" action not allowed in last table.");
+        lexer_error(ctx->lexer,
+                    "\"ct_next\" action not allowed in last table.");
         return;
     }
 
@@ -661,7 +569,7 @@ parse_ct_commit_arg(struct action_context *ctx,
                     struct ovnact_ct_commit *cc)
 {
     if (lexer_match_id(ctx->lexer, "ct_mark")) {
-        if (!action_force_match(ctx, LEX_T_EQUALS)) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
             return;
         }
         if (ctx->lexer->token.type == LEX_T_INTEGER) {
@@ -671,12 +579,12 @@ parse_ct_commit_arg(struct action_context *ctx,
             cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
             cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
         } else {
-            action_syntax_error(ctx, "expecting integer");
+            lexer_syntax_error(ctx->lexer, "expecting integer");
             return;
         }
         lexer_get(ctx->lexer);
     } else if (lexer_match_id(ctx->lexer, "ct_label")) {
-        if (!action_force_match(ctx, LEX_T_EQUALS)) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
             return;
         }
         if (ctx->lexer->token.type == LEX_T_INTEGER) {
@@ -686,12 +594,12 @@ parse_ct_commit_arg(struct action_context *ctx,
             cc->ct_label = ctx->lexer->token.value.be128_int;
             cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
         } else {
-            action_syntax_error(ctx, "expecting integer");
+            lexer_syntax_error(ctx->lexer, "expecting integer");
             return;
         }
         lexer_get(ctx->lexer);
     } else {
-        action_syntax_error(ctx, NULL);
+        lexer_syntax_error(ctx->lexer, NULL);
     }
 }
 
@@ -704,7 +612,7 @@ parse_CT_COMMIT(struct action_context *ctx)
     if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
         while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
             parse_ct_commit_arg(ctx, ct_commit);
-            if (ctx->error) {
+            if (ctx->lexer->error) {
                 return;
             }
             lexer_match(ctx->lexer, LEX_T_COMMA);
@@ -786,7 +694,8 @@ parse_ct_nat(struct action_context *ctx, const char *name,
     add_prerequisite(ctx, "ip");
 
     if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
-        action_error(ctx, "\"%s\" action not allowed in last table.", name);
+        lexer_error(ctx->lexer,
+                    "\"%s\" action not allowed in last table.", name);
         return;
     }
     cn->ltable = ctx->pp->cur_ltable + 1;
@@ -794,13 +703,13 @@ parse_ct_nat(struct action_context *ctx, const char *name,
     if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
         if (ctx->lexer->token.type != LEX_T_INTEGER
             || ctx->lexer->token.format != LEX_F_IPV4) {
-            action_syntax_error(ctx, "expecting IPv4 address");
+            lexer_syntax_error(ctx->lexer, "expecting IPv4 address");
             return;
         }
         cn->ip = ctx->lexer->token.value.ipv4;
         lexer_get(ctx->lexer);
 
-        if (!action_force_match(ctx, LEX_T_RPAREN)) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
             return;
         }
     }
@@ -926,7 +835,7 @@ static void
 parse_ct_lb_action(struct action_context *ctx)
 {
     if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
-        action_error(ctx, "\"ct_lb\" action not allowed in last table.");
+        lexer_error(ctx->lexer, "\"ct_lb\" action not allowed in last table.");
         return;
     }
 
@@ -940,7 +849,7 @@ parse_ct_lb_action(struct action_context *ctx)
         while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
             if (ctx->lexer->token.type != LEX_T_INTEGER
                 || mf_subvalue_width(&ctx->lexer->token.value) > 32) {
-                action_syntax_error(ctx, "expecting IPv4 address");
+                lexer_syntax_error(ctx->lexer, "expecting IPv4 address");
                 return;
             }
 
@@ -1113,7 +1022,7 @@ static void
 parse_nested_action(struct action_context *ctx, enum ovnact_type type,
                     const char *prereq)
 {
-    if (!action_force_match(ctx, LEX_T_LCURLY)) {
+    if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) {
         return;
     }
 
@@ -1123,7 +1032,6 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
     struct action_context inner_ctx = {
         .pp = ctx->pp,
         .lexer = ctx->lexer,
-        .error = NULL,
         .ovnacts = &nested,
         .prereqs = NULL
     };
@@ -1134,8 +1042,7 @@ parse_nested_action(struct action_context *ctx, enum ovnact_type type,
     }
 
     expr_destroy(inner_ctx.prereqs); /* XXX what is the correct behavior? */
-    if (inner_ctx.error) {
-        ctx->error = inner_ctx.error;
+    if (inner_ctx.lexer->error) {
         ovnacts_free(nested.data, nested.size);
         ofpbuf_uninit(&nested);
         return;
@@ -1244,11 +1151,11 @@ static void
 parse_get_mac_bind(struct action_context *ctx, int width,
                    struct ovnact_get_mac_bind *get_mac)
 {
-    action_force_match(ctx, LEX_T_LPAREN);
+    lexer_force_match(ctx->lexer, LEX_T_LPAREN);
     action_parse_field(ctx, 0, false, &get_mac->port);
-    action_force_match(ctx, LEX_T_COMMA);
+    lexer_force_match(ctx->lexer, LEX_T_COMMA);
     action_parse_field(ctx, width, false, &get_mac->ip);
-    action_force_match(ctx, LEX_T_RPAREN);
+    lexer_force_match(ctx->lexer, LEX_T_RPAREN);
 }
 
 static void
@@ -1322,13 +1229,13 @@ static void
 parse_put_mac_bind(struct action_context *ctx, int width,
                    struct ovnact_put_mac_bind *put_mac)
 {
-    action_force_match(ctx, LEX_T_LPAREN);
+    lexer_force_match(ctx->lexer, LEX_T_LPAREN);
     action_parse_field(ctx, 0, false, &put_mac->port);
-    action_force_match(ctx, LEX_T_COMMA);
+    lexer_force_match(ctx->lexer, LEX_T_COMMA);
     action_parse_field(ctx, width, false, &put_mac->ip);
-    action_force_match(ctx, LEX_T_COMMA);
+    lexer_force_match(ctx->lexer, LEX_T_COMMA);
     action_parse_field(ctx, 48, false, &put_mac->mac);
-    action_force_match(ctx, LEX_T_RPAREN);
+    lexer_force_match(ctx->lexer, LEX_T_RPAREN);
 }
 
 static void
@@ -1401,38 +1308,35 @@ static void
 parse_dhcp_opt(struct action_context *ctx, struct ovnact_dhcp_option *o)
 {
     if (ctx->lexer->token.type != LEX_T_ID) {
-        action_syntax_error(ctx, NULL);
+        lexer_syntax_error(ctx->lexer, NULL);
         return;
     }
     o->option = dhcp_opts_find(ctx->pp->dhcp_opts, ctx->lexer->token.s);
     if (!o->option) {
-        action_syntax_error(ctx, "expecting DHCP option name");
+        lexer_syntax_error(ctx->lexer, "expecting DHCP option name");
         return;
     }
     lexer_get(ctx->lexer);
 
-    if (!action_force_match(ctx, LEX_T_EQUALS)) {
+    if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
         return;
     }
 
-    char *error = expr_constant_set_parse(ctx->lexer, &o->value);
-    if (error) {
-        action_error(ctx, "%s", error);
-        free(error);
+    if (!expr_constant_set_parse(ctx->lexer, &o->value)) {
         return;
     }
 
     if (!strcmp(o->option->type, "str")) {
         if (o->value.type != EXPR_C_STRING) {
-            action_error(ctx, "DHCP option %s requires string value.",
-                         o->option->name);
+            lexer_error(ctx->lexer, "DHCP option %s requires string value.",
+                        o->option->name);
             expr_constant_set_destroy(&o->value);
             return;
         }
     } else {
         if (o->value.type != EXPR_C_INTEGER) {
-            action_error(ctx, "DHCP option %s requires numeric value.",
-                         o->option->name);
+            lexer_error(ctx->lexer, "DHCP option %s requires numeric value.",
+                        o->option->name);
             expr_constant_set_destroy(&o->value);
             return;
         }
@@ -1469,7 +1373,7 @@ parse_PUT_DHCP_OPTS(struct action_context *ctx, const struct expr_field *dst)
     /* Validate that the destination is a 1-bit, modifiable field. */
     char *error = expr_type_check(dst, 1, true);
     if (error) {
-        action_error(ctx, "%s", error);
+        lexer_error(ctx->lexer, "%s", error);
         free(error);
         return;
     }
@@ -1485,7 +1389,7 @@ parse_PUT_DHCP_OPTS(struct action_context *ctx, const struct expr_field *dst)
 
         struct ovnact_dhcp_option *o = &options[n_options];
         parse_dhcp_opt(ctx, o);
-        if (ctx->error) {
+        if (ctx->lexer->error) {
             free_dhcp_options(options, n_options);
             return;
         }
@@ -1495,7 +1399,8 @@ parse_PUT_DHCP_OPTS(struct action_context *ctx, const struct expr_field *dst)
     } while (!lexer_match(ctx->lexer, LEX_T_RPAREN));
 
     if (!find_offerip(options, n_options)) {
-        action_error(ctx, "put_dhcp_opts requires offerip to be specified.");
+        lexer_error(ctx->lexer,
+                    "put_dhcp_opts requires offerip to be specified.");
         free_dhcp_options(options, n_options);
         return;
     }
@@ -1639,37 +1544,26 @@ free_PUT_DHCP_OPTS(struct ovnact_put_dhcp_opts *pdo)
 static void
 parse_set_action(struct action_context *ctx)
 {
-    struct expr *prereqs = NULL;
     struct expr_field lhs;
-    char *error;
+    if (!expr_field_parse(ctx->lexer, ctx->pp->symtab, &lhs, &ctx->prereqs)) {
+        return;
+    }
 
-    error = expr_field_parse(ctx->lexer, ctx->pp->symtab, &lhs, &ctx->prereqs);
-    if (!error) {
-        if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
-            parse_assignment_action(ctx, true, &lhs);
-        } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
-            if (ctx->lexer->token.type != LEX_T_ID) {
-                parse_LOAD(ctx, &lhs);
-            } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts")
-                && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
-                lexer_get(ctx->lexer); /* Skip put_dhcp_opts. */
-                lexer_get(ctx->lexer); /* Skip '('. */
-                parse_PUT_DHCP_OPTS(ctx, &lhs);
-            } else {
-                parse_assignment_action(ctx, false, &lhs);
-            }
+    if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
+        parse_assignment_action(ctx, true, &lhs);
+    } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
+        if (ctx->lexer->token.type != LEX_T_ID) {
+            parse_LOAD(ctx, &lhs);
+        } else if (!strcmp(ctx->lexer->token.s, "put_dhcp_opts")
+                   && lexer_lookahead(ctx->lexer) == LEX_T_LPAREN) {
+            lexer_get(ctx->lexer); /* Skip put_dhcp_opts. */
+            lexer_get(ctx->lexer); /* Skip '('. */
+            parse_PUT_DHCP_OPTS(ctx, &lhs);
         } else {
-            action_syntax_error(ctx, "expecting `=' or `<->'");
-        }
-        if (!error) {
-            ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
+            parse_assignment_action(ctx, false, &lhs);
         }
-    }
-
-    if (error) {
-        expr_destroy(prereqs);
-        action_error(ctx, "%s", error);
-        free(error);
+    } else {
+        lexer_syntax_error(ctx->lexer, "expecting `=' or `<->'");
     }
 }
 
@@ -1677,7 +1571,7 @@ static bool
 parse_action(struct action_context *ctx)
 {
     if (ctx->lexer->token.type != LEX_T_ID) {
-        action_syntax_error(ctx, NULL);
+        lexer_syntax_error(ctx->lexer, NULL);
         return false;
     }
 
@@ -1714,12 +1608,10 @@ parse_action(struct action_context *ctx)
     } else if (lexer_match_id(ctx->lexer, "put_nd")) {
         parse_put_mac_bind(ctx, 128, ovnact_put_PUT_ND(ctx->ovnacts));
     } else {
-        action_syntax_error(ctx, "expecting action");
+        lexer_syntax_error(ctx->lexer, "expecting action");
     }
-    if (!lexer_match(ctx->lexer, LEX_T_SEMICOLON)) {
-        action_syntax_error(ctx, "expecting ';'");
-    }
-    return !ctx->error;
+    lexer_force_match(ctx->lexer, LEX_T_SEMICOLON);
+    return !ctx->lexer->error;
 }
 
 static void
@@ -1732,9 +1624,7 @@ parse_actions(struct action_context *ctx)
         && lexer_lookahead(ctx->lexer) == LEX_T_SEMICOLON) {
         lexer_get(ctx->lexer);  /* Skip "drop". */
         lexer_get(ctx->lexer);  /* Skip ";". */
-        if (ctx->lexer->token.type != LEX_T_END) {
-            action_syntax_error(ctx, "expecting end of input");
-        }
+        lexer_force_end(ctx->lexer);
         return;
     }
 
@@ -1762,7 +1652,7 @@ parse_actions(struct action_context *ctx)
  * '*prereqsp' is set to NULL, but some tokens may have been consumed from
  * 'lexer'.
   */
-char * OVS_WARN_UNUSED_RESULT
+bool
 ovnacts_parse(struct lexer *lexer, const struct ovnact_parse_params *pp,
               struct ofpbuf *ovnacts, struct expr **prereqsp)
 {
@@ -1771,15 +1661,16 @@ ovnacts_parse(struct lexer *lexer, const struct ovnact_parse_params *pp,
     struct action_context ctx = {
         .pp = pp,
         .lexer = lexer,
-        .error = NULL,
         .ovnacts = ovnacts,
         .prereqs = NULL,
     };
-    parse_actions(&ctx);
+    if (!lexer->error) {
+        parse_actions(&ctx);
+    }
 
-    if (!ctx.error) {
+    if (!lexer->error) {
         *prereqsp = ctx.prereqs;
-        return NULL;
+        return true;
     } else {
         ofpbuf_pull(ovnacts, ovnacts_start);
         ovnacts_free(ovnacts->data, ovnacts->size);
@@ -1788,7 +1679,7 @@ ovnacts_parse(struct lexer *lexer, const struct ovnact_parse_params *pp,
         ovnacts->size = ovnacts_start;
         expr_destroy(ctx.prereqs);
         *prereqsp = NULL;
-        return ctx.error;
+        return false;
     }
 }
 
@@ -1798,11 +1689,11 @@ ovnacts_parse_string(const char *s, const struct ovnact_parse_params *pp,
                      struct ofpbuf *ofpacts, struct expr **prereqsp)
 {
     struct lexer lexer;
-    char *error;
 
     lexer_init(&lexer, s);
     lexer_get(&lexer);
-    error = ovnacts_parse(&lexer, pp, ofpacts, prereqsp);
+    ovnacts_parse(&lexer, pp, ofpacts, prereqsp);
+    char *error = lexer_steal_error(&lexer);
     lexer_destroy(&lexer);
 
     return error;
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 25281e0..c3a26f5 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -417,7 +417,6 @@ struct expr_context {
     struct lexer *lexer;        /* Lexer for pulling more tokens. */
     const struct shash *symtab; /* Symbol table. */
     const struct shash *macros; /* Table of macros. */
-    char *error;                /* Error, if any, otherwise NULL. */
     bool not;                   /* True inside odd number of NOT operators. */
 };
 
@@ -425,64 +424,6 @@ struct expr *expr_parse__(struct expr_context *);
 static void expr_not(struct expr *);
 static bool parse_field(struct expr_context *, struct expr_field *);
 
-static bool
-expr_error_handle_common(struct expr_context *ctx)
-{
-    if (ctx->error) {
-        /* Already have an error, suppress this one since the cascade seems
-         * unlikely to be useful. */
-        return true;
-    } else if (ctx->lexer->token.type == LEX_T_ERROR) {
-        /* The lexer signaled an error.  Nothing at the expression level
-         * accepts an error token, so we'll inevitably end up here with some
-         * meaningless parse error.  Report the lexical error instead. */
-        ctx->error = xstrdup(ctx->lexer->token.s);
-        return true;
-    } else {
-        return false;
-    }
-}
-
-static void OVS_PRINTF_FORMAT(2, 3)
-expr_error(struct expr_context *ctx, const char *message, ...)
-{
-    if (expr_error_handle_common(ctx)) {
-        return;
-    }
-
-    va_list args;
-    va_start(args, message);
-    ctx->error = xvasprintf(message, args);
-    va_end(args);
-}
-
-static void OVS_PRINTF_FORMAT(2, 3)
-expr_syntax_error(struct expr_context *ctx, const char *message, ...)
-{
-    if (expr_error_handle_common(ctx)) {
-        return;
-    }
-
-    struct ds s;
-
-    ds_init(&s);
-    ds_put_cstr(&s, "Syntax error ");
-    if (ctx->lexer->token.type == LEX_T_END) {
-        ds_put_cstr(&s, "at end of input ");
-    } else if (ctx->lexer->start) {
-        ds_put_format(&s, "at `%.*s' ",
-                      (int) (ctx->lexer->input - ctx->lexer->start),
-                      ctx->lexer->start);
-    }
-
-    va_list args;
-    va_start(args, message);
-    ds_put_format_valist(&s, message, args);
-    va_end(args);
-
-    ctx->error = ds_steal_cstr(&s);
-}
-
 static struct expr *
 make_cmp__(const struct expr_field *f, enum expr_relop r,
              const union expr_constant *c)
@@ -541,10 +482,11 @@ type_check(struct expr_context *ctx, const struct expr_field *f,
            struct expr_constant_set *cs)
 {
     if (cs->type != (f->symbol->width ? EXPR_C_INTEGER : EXPR_C_STRING)) {
-        expr_error(ctx, "%s field %s is not compatible with %s constant.",
-                   f->symbol->width ? "Integer" : "String",
-                   f->symbol->name,
-                   cs->type == EXPR_C_INTEGER ? "integer" : "string");
+        lexer_error(ctx->lexer,
+                    "%s field %s is not compatible with %s constant.",
+                    f->symbol->width ? "Integer" : "String",
+                    f->symbol->name,
+                    cs->type == EXPR_C_INTEGER ? "integer" : "string");
         return false;
     }
 
@@ -552,9 +494,9 @@ type_check(struct expr_context *ctx, const struct expr_field *f,
         for (size_t i = 0; i < cs->n_values; i++) {
             int w = expr_constant_width(&cs->values[i]);
             if (w > f->symbol->width) {
-                expr_error(ctx, "%d-bit constant is not compatible with "
-                           "%d-bit field %s.",
-                           w, f->symbol->width, f->symbol->name);
+                lexer_error(ctx->lexer,
+                            "%d-bit constant is not compatible with %d-bit "
+                            "field %s.", w, f->symbol->width, f->symbol->name);
                 return false;
             }
         }
@@ -576,23 +518,23 @@ make_cmp(struct expr_context *ctx,
 
     if (r != EXPR_R_EQ && r != EXPR_R_NE) {
         if (cs->in_curlies) {
-            expr_error(ctx, "Only == and != operators may be used "
-                       "with value sets.");
+            lexer_error(ctx->lexer, "Only == and != operators may be used "
+                        "with value sets.");
             goto exit;
         }
         if (f->symbol->level == EXPR_L_NOMINAL ||
             f->symbol->level == EXPR_L_BOOLEAN) {
-            expr_error(ctx, "Only == and != operators may be used "
-                       "with %s field %s.",
-                       expr_level_to_string(f->symbol->level),
-                       f->symbol->name);
+            lexer_error(ctx->lexer, "Only == and != operators may be used "
+                        "with %s field %s.",
+                        expr_level_to_string(f->symbol->level),
+                        f->symbol->name);
             goto exit;
         }
         if (cs->values[0].masked) {
-            expr_error(ctx, "Only == and != operators may be used with "
-                       "masked constants.  Consider using subfields instead "
-                       "(e.g. eth.src[0..15] > 0x1111 in place of "
-                       "eth.src > 00:00:00:00:11:11/00:00:00:00:ff:ff).");
+            lexer_error(ctx->lexer, "Only == and != operators may be used "
+                        "with masked constants.  Consider using subfields "
+                        "instead (e.g. eth.src[0..15] > 0x1111 in place of "
+                        "eth.src > 00:00:00:00:11:11/00:00:00:00:ff:ff).");
             goto exit;
         }
     }
@@ -607,17 +549,18 @@ make_cmp(struct expr_context *ctx,
                 positive ^= ctx->not;
                 if (!positive) {
                     const char *name = f->symbol->name;
-                    expr_error(ctx, "Nominal predicate %s may only be tested "
-                               "positively, e.g. `%s' or `%s == 1' but not "
-                               "`!%s' or `%s == 0'.",
-                               name, name, name, name, name);
+                    lexer_error(ctx->lexer,
+                                "Nominal predicate %s may only be tested "
+                                "positively, e.g. `%s' or `%s == 1' but not "
+                                "`!%s' or `%s == 0'.",
+                                name, name, name, name, name);
                     goto exit;
                 }
             }
         } else if (r != (ctx->not ? EXPR_R_NE : EXPR_R_EQ)) {
-            expr_error(ctx, "Nominal field %s may only be tested for "
-                       "equality (taking enclosing `!' operators into "
-                       "account).", f->symbol->name);
+            lexer_error(ctx->lexer, "Nominal field %s may only be tested for "
+                        "equality (taking enclosing `!' operators into "
+                        "account).", f->symbol->name);
             goto exit;
         }
     }
@@ -633,28 +576,18 @@ exit:
 }
 
 static bool
-expr_get_int(struct expr_context *ctx, int *value)
-{
-    bool ok = lexer_get_int(ctx->lexer, value);
-    if (!ok) {
-        expr_syntax_error(ctx, "expecting small integer.");
-    }
-    return ok;
-}
-
-static bool
 parse_field(struct expr_context *ctx, struct expr_field *f)
 {
     const struct expr_symbol *symbol;
 
     if (ctx->lexer->token.type != LEX_T_ID) {
-        expr_syntax_error(ctx, "expecting field name.");
+        lexer_syntax_error(ctx->lexer, "expecting field name");
         return false;
     }
 
     symbol = shash_find_data(ctx->symtab, ctx->lexer->token.s);
     if (!symbol) {
-        expr_syntax_error(ctx, "expecting field name.");
+        lexer_syntax_error(ctx->lexer, "expecting field name");
         return false;
     }
     lexer_get(ctx->lexer);
@@ -664,38 +597,40 @@ parse_field(struct expr_context *ctx, struct expr_field *f)
         int low, high;
 
         if (!symbol->width) {
-            expr_error(ctx, "Cannot select subfield of string field %s.",
-                       symbol->name);
+            lexer_error(ctx->lexer,
+                        "Cannot select subfield of string field %s.",
+                        symbol->name);
             return false;
         }
 
-        if (!expr_get_int(ctx, &low)) {
+        if (!lexer_force_int(ctx->lexer, &low)) {
             return false;
         }
         if (lexer_match(ctx->lexer, LEX_T_ELLIPSIS)) {
-            if (!expr_get_int(ctx, &high)) {
+            if (!lexer_force_int(ctx->lexer, &high)) {
                 return false;
             }
         } else {
             high = low;
         }
 
-        if (!lexer_match(ctx->lexer, LEX_T_RSQUARE)) {
-            expr_syntax_error(ctx, "expecting `]'.");
+        if (!lexer_force_match(ctx->lexer, LEX_T_RSQUARE)) {
             return false;
         }
 
         if (low > high) {
-            expr_error(ctx, "Invalid bit range %d to %d.", low, high);
+            lexer_error(ctx->lexer, "Invalid bit range %d to %d.", low, high);
             return false;
         } else if (high >= symbol->width) {
-            expr_error(ctx, "Cannot select bits %d to %d of %d-bit field %s.",
-                       low, high, symbol->width, symbol->name);
+            lexer_error(ctx->lexer,
+                        "Cannot select bits %d to %d of %d-bit field %s.",
+                        low, high, symbol->width, symbol->name);
             return false;
         } else if (symbol->level == EXPR_L_NOMINAL
                    && (low != 0 || high != symbol->width - 1)) {
-            expr_error(ctx, "Cannot select subfield of nominal field %s.",
-                       symbol->name);
+            lexer_error(ctx->lexer,
+                        "Cannot select subfield of nominal field %s.",
+                        symbol->name);
             return false;
         }
 
@@ -716,7 +651,7 @@ parse_relop(struct expr_context *ctx, enum expr_relop *relop)
         lexer_get(ctx->lexer);
         return true;
     } else {
-        expr_syntax_error(ctx, "expecting relational operator.");
+        lexer_syntax_error(ctx->lexer, "expecting relational operator");
         return false;
     }
 }
@@ -730,8 +665,8 @@ assign_constant_set_type(struct expr_context *ctx,
         cs->type = type;
         return true;
     } else {
-        expr_syntax_error(ctx, "expecting %s.",
-                          cs->type == EXPR_C_INTEGER ? "integer" : "string");
+        lexer_syntax_error(ctx->lexer, "expecting %s",
+                           cs->type == EXPR_C_INTEGER ? "integer" : "string");
         return false;
     }
 }
@@ -745,7 +680,7 @@ parse_macros(struct expr_context *ctx, struct expr_constant_set *cs,
            ? shash_find_data(ctx->macros, ctx->lexer->token.s)
            : NULL);
     if (!addr_set) {
-        expr_syntax_error(ctx, "expecting address set name.");
+        lexer_syntax_error(ctx->lexer, "expecting address set name");
         return false;
     }
 
@@ -803,7 +738,7 @@ parse_constant(struct expr_context *ctx, struct expr_constant_set *cs,
         lexer_get(ctx->lexer);
         return true;
     } else {
-        expr_syntax_error(ctx, "expecting constant.");
+        lexer_syntax_error(ctx->lexer, "expecting constant");
         return false;
     }
 }
@@ -842,10 +777,14 @@ parse_constant_set(struct expr_context *ctx, struct expr_constant_set *cs)
  * type of 'f' into 'c'.  On success, returns NULL.  On failure, returns an
  * xmalloc()'ed error message that the caller must free and 'c' is
  * indeterminate. */
-char * OVS_WARN_UNUSED_RESULT
+bool
 expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
                     union expr_constant *c)
 {
+    if (lexer->error) {
+        return false;
+    }
+
     struct expr_context ctx = { .lexer = lexer };
 
     struct expr_constant_set cs;
@@ -858,7 +797,7 @@ expr_constant_parse(struct lexer *lexer, const struct expr_field *f,
     }
     expr_constant_set_destroy(&cs);
 
-    return ctx.error;
+    return !lexer->error;
 }
 
 /* Appends to 's' a re-parseable representation of constant 'c' with the given
@@ -900,12 +839,14 @@ expr_constant_destroy(const union expr_constant *c,
  * Returns NULL on success, in which case the caller owns 'cs', otherwise a
  * xmalloc()'ed error message owned by the caller, in which case 'cs' is
  * indeterminate. */
-char * OVS_WARN_UNUSED_RESULT
+bool
 expr_constant_set_parse(struct lexer *lexer, struct expr_constant_set *cs)
 {
-    struct expr_context ctx = { .lexer = lexer };
-    parse_constant_set(&ctx, cs);
-    return ctx.error;
+    if (!lexer->error) {
+        struct expr_context ctx = { .lexer = lexer };
+        parse_constant_set(&ctx, cs);
+    }
+    return !lexer->error;
 }
 
 /* Appends to 's' a re-parseable representation of 'cs'. */
@@ -1013,9 +954,8 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic)
     *atomic = false;
     if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
         struct expr *e = expr_parse__(ctx);
-        if (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_RPAREN)) {
             expr_destroy(e);
-            expr_syntax_error(ctx, "expecting `)'.");
             return NULL;
         }
         *atomic = true;
@@ -1033,10 +973,12 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic)
 
         if (!expr_relop_from_token(ctx->lexer->token.type, &r)) {
             if (!f.n_bits || ctx->lexer->token.type == LEX_T_EQUALS) {
-                expr_syntax_error(ctx, "expecting relational operator.");
+                lexer_syntax_error(ctx->lexer,
+                                   "expecting relational operator");
                 return NULL;
             } else if (f.n_bits > 1 && !ctx->not) {
-                expr_error(ctx, "Explicit `!= 0' is required for inequality "
+                lexer_error(ctx->lexer,
+                            "Explicit `!= 0' is required for inequality "
                             "test of multibit field against 0.");
                 return NULL;
             }
@@ -1099,9 +1041,10 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic)
                    (r2 == EXPR_R_LT || r2 == EXPR_R_LE)) ||
                   ((r1 == EXPR_R_GT || r1 == EXPR_R_GE) &&
                    (r2 == EXPR_R_GT || r2 == EXPR_R_GE)))) {
-                expr_error(ctx, "Range expressions must have the form "
-                           "`x < field < y' or `x > field > y', with each "
-                           "`<' optionally replaced by `<=' or `>' by `>=').");
+                lexer_error(ctx->lexer, "Range expressions must have the "
+                            "form `x < field < y' or `x > field > y', with "
+                            "each `<' optionally replaced by `<=' or `>' by "
+                            "`>=').");
                 expr_constant_set_destroy(&c1);
                 expr_constant_set_destroy(&c2);
                 return NULL;
@@ -1109,7 +1052,7 @@ expr_parse_primary(struct expr_context *ctx, bool *atomic)
 
             struct expr *e1 = make_cmp(ctx, &f, expr_relop_turn(r1), &c1);
             struct expr *e2 = make_cmp(ctx, &f, r2, &c2);
-            if (ctx->error) {
+            if (ctx->lexer->error) {
                 expr_destroy(e1);
                 expr_destroy(e2);
                 return NULL;
@@ -1131,7 +1074,8 @@ expr_parse_not(struct expr_context *ctx)
 
         if (expr) {
             if (!atomic) {
-                expr_error(ctx, "Missing parentheses around operand of !.");
+                lexer_error(ctx->lexer,
+                            "Missing parentheses around operand of !.");
                 expr_destroy(expr);
                 return NULL;
             }
@@ -1168,8 +1112,8 @@ expr_parse__(struct expr_context *ctx)
         if (ctx->lexer->token.type == LEX_T_LOG_AND
             || ctx->lexer->token.type == LEX_T_LOG_OR) {
             expr_destroy(e);
-            expr_error(ctx,
-                       "&& and || must be parenthesized when used together.");
+            lexer_error(ctx->lexer,
+                        "&& and || must be parenthesized when used together.");
             return NULL;
         }
     }
@@ -1183,15 +1127,12 @@ expr_parse__(struct expr_context *ctx)
  * expr_destroy()) or error (with free()). */
 struct expr *
 expr_parse(struct lexer *lexer, const struct shash *symtab,
-           const struct shash *macros, char **errorp)
+           const struct shash *macros)
 {
     struct expr_context ctx = { .lexer = lexer,
                                 .symtab = symtab,
                                 .macros = macros };
-    struct expr *e = expr_parse__(&ctx);
-    *errorp = ctx.error;
-    ovs_assert((ctx.error != NULL) != (e != NULL));
-    return e;
+    return lexer->error ? NULL : expr_parse__(&ctx);
 }
 
 /* Like expr_parse(), but the expression is taken from 's'. */
@@ -1200,13 +1141,13 @@ expr_parse_string(const char *s, const struct shash *symtab,
                   const struct shash *macros, char **errorp)
 {
     struct lexer lexer;
-    struct expr *expr;
 
     lexer_init(&lexer, s);
     lexer_get(&lexer);
-    expr = expr_parse(&lexer, symtab, macros, errorp);
-    if (!*errorp && lexer.token.type != LEX_T_END) {
-        *errorp = xstrdup("Extra tokens at end of input.");
+    struct expr *expr = expr_parse(&lexer, symtab, macros);
+    lexer_force_end(&lexer);
+    *errorp = lexer_steal_error(&lexer);
+    if (*errorp) {
         expr_destroy(expr);
         expr = NULL;
     }
@@ -1218,23 +1159,25 @@ expr_parse_string(const char *s, const struct shash *symtab,
 /* 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
+bool
 expr_field_parse(struct lexer *lexer, const struct shash *symtab,
                  struct expr_field *field, struct expr **prereqsp)
 {
     struct expr_context ctx = { .lexer = lexer, .symtab = symtab };
     if (parse_field(&ctx, field) && field->symbol->predicate) {
-        ctx.error = xasprintf("Predicate symbol %s used where lvalue "
-                              "required.", field->symbol->name);
+        lexer_error(lexer, "Predicate symbol %s used where lvalue required.",
+                    field->symbol->name);
     }
-    if (!ctx.error) {
+    if (!lexer->error) {
         const struct expr_symbol *symbol = field->symbol;
         while (symbol) {
             if (symbol->prereqs) {
+                char *error;
                 struct ovs_list nesting = OVS_LIST_INITIALIZER(&nesting);
                 struct expr *e = parse_and_annotate(symbol->prereqs, symtab,
-                                                    &nesting, &ctx.error);
-                if (ctx.error) {
+                                                    &nesting, &error);
+                if (error) {
+                    lexer_error(lexer, "%s", error);
                     break;
                 }
                 *prereqsp = expr_combine(EXPR_T_AND, *prereqsp, e);
@@ -1246,10 +1189,11 @@ expr_field_parse(struct lexer *lexer, const struct shash *symtab,
             symbol = symbol->parent;
         }
     }
-    if (ctx.error) {
-        memset(field, 0, sizeof *field);
+    if (!lexer->error) {
+        return true;
     }
-    return ctx.error;
+    memset(field, 0, sizeof *field);
+    return false;
 }
 
 /* Appends to 's' a re-parseable representation of 'field'. */
@@ -1335,17 +1279,12 @@ parse_field_from_string(const char *s, const struct shash *symtab,
     lexer_get(&lexer);
 
     struct expr_context ctx = { .lexer = &lexer, .symtab = symtab };
-    bool ok = parse_field(&ctx, field);
-    if (!ok) {
-        *errorp = ctx.error;
-    } else if (lexer.token.type != LEX_T_END) {
-        *errorp = xstrdup("Extra tokens at end of input.");
-        ok = false;
-    }
-
+    parse_field(&ctx, field);
+    lexer_force_end(&lexer);
+    *errorp = lexer_steal_error(&lexer);
     lexer_destroy(&lexer);
 
-    return ok;
+    return !*errorp;
 }
 
 /* Adds 'name' as a subfield of a larger field in 'symtab'.  Whenever
diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c
index 95edeaf..a05edfa 100644
--- a/ovn/lib/lex.c
+++ b/ovn/lib/lex.c
@@ -803,6 +803,7 @@ lexer_init(struct lexer *lexer, const char *input)
     lexer->input = input;
     lexer->start = NULL;
     lex_token_init(&lexer->token);
+    lexer->error = NULL;
 }
 
 /* Frees storage associated with 'lexer'. */
@@ -810,6 +811,7 @@ void
 lexer_destroy(struct lexer *lexer)
 {
     lex_token_destroy(&lexer->token);
+    free(lexer->error);
 }
 
 /* Obtains the next token from 'lexer' into 'lexer->token', and returns the
@@ -851,6 +853,24 @@ lexer_match(struct lexer *lexer, enum lex_type type)
     }
 }
 
+bool
+lexer_force_match(struct lexer *lexer, enum lex_type t)
+{
+    if (lexer_match(lexer, t)) {
+        return true;
+    } else {
+        struct lex_token token = { .type = t };
+        struct ds s = DS_EMPTY_INITIALIZER;
+        lex_token_format(&token, &s);
+
+        lexer_syntax_error(lexer, "expecting `%s'", ds_cstr(&s));
+
+        ds_destroy(&s);
+
+        return false;
+    }
+}
+
 /* If 'lexer''s current token is the identifier given in 'id', advances 'lexer'
  * to the next token and returns true.  Otherwise returns false.  */
 bool
@@ -884,3 +904,92 @@ lexer_get_int(struct lexer *lexer, int *value)
         return false;
     }
 }
+
+bool
+lexer_force_int(struct lexer *lexer, int *value)
+{
+    bool ok = lexer_get_int(lexer, value);
+    if (!ok) {
+        lexer_syntax_error(lexer, "expecting small integer");
+    }
+    return ok;
+}
+
+void
+lexer_force_end(struct lexer *lexer)
+{
+    if (lexer->token.type != LEX_T_END) {
+        lexer_syntax_error(lexer, "expecting end of input");
+    }
+}
+
+static bool
+lexer_error_handle_common(struct lexer *lexer)
+{
+    if (lexer->error) {
+        /* Already have an error, suppress this one since the cascade seems
+         * unlikely to be useful. */
+        return true;
+    } else if (lexer->token.type == LEX_T_ERROR) {
+        /* The lexer signaled an error.  Nothing at a higher level accepts an
+         * error token, so we'll inevitably end up here with some meaningless
+         * parse error.  Report the lexical error instead. */
+        lexer->error = xstrdup(lexer->token.s);
+        return true;
+    } else {
+        return false;
+    }
+}
+
+void OVS_PRINTF_FORMAT(2, 3)
+lexer_error(struct lexer *lexer, const char *message, ...)
+{
+    if (lexer_error_handle_common(lexer)) {
+        return;
+    }
+
+    va_list args;
+    va_start(args, message);
+    lexer->error = xvasprintf(message, args);
+    va_end(args);
+}
+
+void OVS_PRINTF_FORMAT(2, 3)
+lexer_syntax_error(struct lexer *lexer, const char *message, ...)
+{
+    if (lexer_error_handle_common(lexer)) {
+        return;
+    }
+
+    struct ds s;
+
+    ds_init(&s);
+    ds_put_cstr(&s, "Syntax error");
+    if (lexer->token.type == LEX_T_END) {
+        ds_put_cstr(&s, " at end of input");
+    } else if (lexer->start) {
+        ds_put_format(&s, " at `%.*s'",
+                      (int) (lexer->input - lexer->start),
+                      lexer->start);
+    }
+
+    if (message) {
+        ds_put_char(&s, ' ');
+
+        va_list args;
+        va_start(args, message);
+        ds_put_format_valist(&s, message, args);
+        va_end(args);
+    }
+    ds_put_char(&s, '.');
+
+    lexer->error = ds_steal_cstr(&s);
+}
+
+char *
+lexer_steal_error(struct lexer *lexer)
+{
+    char *error = lexer->error;
+    lexer->error = NULL;
+    return error;
+}
diff --git a/tests/ovn.at b/tests/ovn.at
index b187641..2f91838 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -341,7 +341,7 @@ ip4.src == ::1 => 128-bit constant is not compatible with 32-bit field ip4.src.
 
 1 == eth.type == 2 => Range expressions must have the form `x < field < y' or `x > field > y', with each `<' optionally replaced by `<=' or `>' by `>=').
 
-eth.dst[40] x => Extra tokens at end of input.
+eth.dst[40] x => Syntax error at `x' expecting end of input.
 
 ip4.src == {1.2.3.4, $set1, $unknownset} => Syntax error at `$unknownset' expecting address set name.
 eth.src == {$set3, badmac, 00:00:00:00:00:01} => Syntax error at `badmac' expecting constant.
@@ -931,7 +931,7 @@ next; 123;
 next; xyzzy;
     Syntax error at `xyzzy' expecting action.
 next
-    Syntax error at end of input expecting ';'.
+    Syntax error at end of input expecting `;'.
 ]])
 sed '/^[[ 	]]/d' test-cases.txt > input.txt
 cp test-cases.txt expout
-- 
2.1.3




More information about the dev mailing list