frontend: Improved parser error reporting.

All error messages are now written to a string buffer, allowing them to be
displayed as part of the main error messages associated with failed commands.

diff --git a/src/frontend/grammar.y b/src/frontend/grammar.y
index 4dc6cb22411ce756991d07c0a01cd078d6949abd..ab9345e22c80272b6cbd4b84b0fd7ce4472fda87 100644 (file)
--- a/src/frontend/grammar.y
+++ b/src/frontend/grammar.y
@@ -72,6 +72,9 @@ sdb_fe_yyerrorf(YYLTYPE *lval, sdb_fe_yyscan_t scanner, const char *fmt, ...);
 /* quick access to the parser mode */
 #define parser_mode sdb_fe_yyget_extra(scanner)->mode
 
+/* quick access to the parser's error buffer */
+#define errbuf sdb_fe_yyget_extra(scanner)->errbuf
+
 #define MODE_TO_STRING(m) \
        (((m) == SDB_PARSE_DEFAULT) ? "statement" \
                : ((m) == SDB_PARSE_COND) ? "condition" \
@@ -653,14 +656,17 @@ void
 sdb_fe_yyerror(YYLTYPE *lval, sdb_fe_yyscan_t scanner, const char *msg)
 {
        sdb_log(SDB_LOG_ERR, "frontend: parse error: %s", msg);
+       sdb_strbuf_sprintf(errbuf, "%s", msg);
 } /* sdb_fe_yyerror */
 
 void
 sdb_fe_yyerrorf(YYLTYPE *lval, sdb_fe_yyscan_t scanner, const char *fmt, ...)
 {
-       va_list ap;
+       va_list ap, aq;
        va_start(ap, fmt);
+       va_copy(aq, ap);
        sdb_vlog(SDB_LOG_ERR, fmt, ap);
+       sdb_strbuf_vsprintf(errbuf, "%s", aq);
        va_end(ap);
 } /* sdb_fe_yyerrorf */
 

diff --git a/src/frontend/parser.c b/src/frontend/parser.c
index f8c82d3e234b856506bb3461b0563ab1f38505fc..936f31e6c2e12ecc4658c94d07b69e1d2cc03d94 100644 (file)
--- a/src/frontend/parser.c
+++ b/src/frontend/parser.c
@@ -34,7 +34,9 @@
 #include "core/store.h"
 
 #include "utils/llist.h"
+#include "utils/strbuf.h"
 
+#include <assert.h>
 #include <string.h>
 
 /*
@@ -43,17 +45,23 @@
 
 static int
 scanner_init(const char *input, int len,
-               sdb_fe_yyscan_t *scanner, sdb_fe_yyextra_t *extra)
+               sdb_fe_yyscan_t *scanner, sdb_fe_yyextra_t *extra,
+               sdb_strbuf_t *errbuf)
 {
-       if (! input)
+       if (! input) {
+               sdb_strbuf_sprintf(errbuf, "Missing scanner input");
                return -1;
+       }
 
        memset(extra, 0, sizeof(*extra));
        extra->parsetree = sdb_llist_create();
        extra->mode = SDB_PARSE_DEFAULT;
+       extra->errbuf = errbuf;
 
-       if (! extra->parsetree)
+       if (! extra->parsetree) {
+               sdb_strbuf_sprintf(errbuf, "Failed to allocate parse-tree");
                return -1;
+       }
 
        *scanner = sdb_fe_scanner_init(input, len, extra);
        if (! scanner) {
@@ -68,14 +76,14 @@ scanner_init(const char *input, int len,
  */
 
 sdb_llist_t *
-sdb_fe_parse(const char *query, int len)
+sdb_fe_parse(const char *query, int len, sdb_strbuf_t *errbuf)
 {
        sdb_fe_yyscan_t scanner;
        sdb_fe_yyextra_t yyextra;
        sdb_llist_iter_t *iter;
        int yyres;
 
-       if (scanner_init(query, len, &scanner, &yyextra))
+       if (scanner_init(query, len, &scanner, &yyextra, errbuf))
                return NULL;
 
        yyres = sdb_fe_yyparse(scanner);
@@ -91,6 +99,9 @@ sdb_fe_parse(const char *query, int len)
                sdb_conn_node_t *node;
                node = SDB_CONN_NODE(sdb_llist_iter_get_next(iter));
                if (sdb_fe_analyze(node)) {
+                       /* TODO: pass on errbuf to the analyzer */
+                       sdb_strbuf_sprintf(errbuf, "Failed to verify "
+                                       "query '%s'", query);
                        sdb_llist_iter_destroy(iter);
                        sdb_llist_destroy(yyextra.parsetree);
                        return NULL;
@@ -101,7 +112,7 @@ sdb_fe_parse(const char *query, int len)
 } /* sdb_fe_parse */
 
 sdb_store_matcher_t *
-sdb_fe_parse_matcher(const char *cond, int len)
+sdb_fe_parse_matcher(const char *cond, int len, sdb_strbuf_t *errbuf)
 {
        sdb_fe_yyscan_t scanner;
        sdb_fe_yyextra_t yyextra;
@@ -111,7 +122,7 @@ sdb_fe_parse_matcher(const char *cond, int len)
 
        int yyres;
 
-       if (scanner_init(cond, len, &scanner, &yyextra))
+       if (scanner_init(cond, len, &scanner, &yyextra, errbuf))
                return NULL;
 
        yyextra.mode = SDB_PARSE_COND;
@@ -126,16 +137,14 @@ sdb_fe_parse_matcher(const char *cond, int len)
 
        node = SDB_CONN_NODE(sdb_llist_get(yyextra.parsetree, 0));
        if (! node) {
+               sdb_strbuf_sprintf(errbuf, "Empty matcher expression '%s'", cond);
                sdb_llist_destroy(yyextra.parsetree);
                return NULL;
        }
 
-       if (node->cmd == CONNECTION_MATCHER) {
-               m = CONN_MATCHER(node)->matcher;
-               CONN_MATCHER(node)->matcher = NULL;
-       }
-       else
-               m = NULL;
+       assert(node->cmd == CONNECTION_MATCHER);
+       m = CONN_MATCHER(node)->matcher;
+       CONN_MATCHER(node)->matcher = NULL;
 
        sdb_llist_destroy(yyextra.parsetree);
        sdb_object_deref(SDB_OBJ(node));
@@ -143,7 +152,7 @@ sdb_fe_parse_matcher(const char *cond, int len)
 } /* sdb_fe_parse_matcher */
 
 sdb_store_expr_t *
-sdb_fe_parse_expr(const char *expr, int len)
+sdb_fe_parse_expr(const char *expr, int len, sdb_strbuf_t *errbuf)
 {
        sdb_fe_yyscan_t scanner;
        sdb_fe_yyextra_t yyextra;
@@ -153,7 +162,7 @@ sdb_fe_parse_expr(const char *expr, int len)
 
        int yyres;
 
-       if (scanner_init(expr, len, &scanner, &yyextra))
+       if (scanner_init(expr, len, &scanner, &yyextra, errbuf))
                return NULL;
 
        yyextra.mode = SDB_PARSE_EXPR;
@@ -168,16 +177,14 @@ sdb_fe_parse_expr(const char *expr, int len)
 
        node = SDB_CONN_NODE(sdb_llist_get(yyextra.parsetree, 0));
        if (! node) {
+               sdb_strbuf_sprintf(errbuf, "Empty expression '%s'", expr);
                sdb_llist_destroy(yyextra.parsetree);
                return NULL;
        }
 
-       if (node->cmd == CONNECTION_EXPR) {
-               e = CONN_EXPR(node)->expr;
-               CONN_EXPR(node)->expr = NULL;
-       }
-       else
-               e = NULL;
+       assert(node->cmd == CONNECTION_EXPR);
+       e = CONN_EXPR(node)->expr;
+       CONN_EXPR(node)->expr = NULL;
 
        sdb_llist_destroy(yyextra.parsetree);
        sdb_object_deref(SDB_OBJ(node));

diff --git a/src/frontend/query.c b/src/frontend/query.c
index e5a1fc43263b925653f71539ca369a5f677646b7..338d95a75c6351343a6a2b248b3f5bc2a61cdafa 100644 (file)
--- a/src/frontend/query.c
+++ b/src/frontend/query.c
@@ -73,13 +73,13 @@ sdb_fe_query(sdb_conn_t *conn)
                return -1;
 
        parsetree = sdb_fe_parse(sdb_strbuf_string(conn->buf),
-                       (int)conn->cmd_len);
+                       (int)conn->cmd_len, conn->errbuf);
        if (! parsetree) {
                char query[conn->cmd_len + 1];
                strncpy(query, sdb_strbuf_string(conn->buf), conn->cmd_len);
                query[sizeof(query) - 1] = '\0';
-               sdb_log(SDB_LOG_ERR, "frontend: Failed to parse query '%s'",
-                               query);
+               sdb_log(SDB_LOG_ERR, "frontend: Failed to parse query '%s': %s",
+                               query, sdb_strbuf_string(conn->errbuf));
                return -1;
        }
 
@@ -192,13 +192,14 @@ sdb_fe_lookup(sdb_conn_t *conn)
 
        matcher = sdb_strbuf_string(conn->buf) + sizeof(uint32_t);
        matcher_len = conn->cmd_len - sizeof(uint32_t);
-       m = sdb_fe_parse_matcher(matcher, (int)matcher_len);
+       m = sdb_fe_parse_matcher(matcher, (int)matcher_len, conn->errbuf);
        if (! m) {
                char expr[matcher_len + 1];
                strncpy(expr, matcher, sizeof(expr));
                expr[sizeof(expr) - 1] = '\0';
                sdb_log(SDB_LOG_ERR, "frontend: Failed to parse "
-                               "lookup condition '%s'", expr);
+                               "lookup condition '%s': %s", expr,
+                               sdb_strbuf_string(conn->errbuf));
                return -1;
        }
 
@@ -211,7 +212,7 @@ sdb_fe_lookup(sdb_conn_t *conn)
                char expr[matcher_len + 1];
                strncpy(expr, matcher, sizeof(expr));
                expr[sizeof(expr) - 1] = '\0';
-               sdb_log(SDB_LOG_ERR, "frontend: Failed to verify "
+               sdb_strbuf_sprintf(conn->errbuf, "Failed to verify "
                                "lookup condition '%s'", expr);
                status = -1;
        }

diff --git a/src/frontend/scanner.l b/src/frontend/scanner.l
index c8d0dcbca25de94fe27196f7b1d65ff034370522..f17b689c97a5b98f03a47ac341981ac07e832f66 100644 (file)
--- a/src/frontend/scanner.l
+++ b/src/frontend/scanner.l
@@ -275,7 +275,7 @@ sdb_fe_scanner_init(const char *str, int len, sdb_fe_yyextra_t *yyext)
 
        if (sdb_fe_yylex_init(&scanner)) {
                char errbuf[1024];
-               sdb_log(SDB_LOG_ERR, "frontend: yylex_init failed: %s",
+               sdb_strbuf_sprintf(yyext->errbuf, "yylex_init_failed: %s",
                        sdb_strerror(errno, errbuf, sizeof(errbuf)));
                return NULL;
        }

diff --git a/src/include/frontend/connection.h b/src/include/frontend/connection.h
index cbaa43ef459d1810a2bd2f84822a41487c7c33e7..b329fa1ec9d8b3c003207b0c197085aeed6c29d6 100644 (file)
--- a/src/include/frontend/connection.h
+++ b/src/include/frontend/connection.h
@@ -126,14 +126,15 @@ sdb_connection_ping(sdb_conn_t *conn);
  * Parse the query text specified in 'query' of length 'len' and return a list
  * of parse trees (for each command) to be executed by sdb_fe_exec. The list
  * has to be freed by the caller. If 'len' is less than zero, parse the whole
- * (nul-terminated) string.
+ * (nul-terminated) string. If specified, errbuf will be used to record parse
+ * errors.
  *
  * Returns:
  *  - an sdb_llist_t object of sdb_conn_node_t on success
  *  - NULL in case of an error
  */
 sdb_llist_t *
-sdb_fe_parse(const char *query, int len);
+sdb_fe_parse(const char *query, int len, sdb_strbuf_t *errbuf);
 
 /*
  * sdb_fe_exec:

diff --git a/src/include/frontend/parser.h b/src/include/frontend/parser.h
index e1c9c6fa81e5d3b4e2b6eef91d14b8ed0fc3b3f9..e763afc805a68081493baf295149b6b1c7221e22 100644 (file)
--- a/src/include/frontend/parser.h
+++ b/src/include/frontend/parser.h
@@ -31,6 +31,7 @@
 #include "core/store.h"
 #include "frontend/connection.h"
 #include "utils/llist.h"
+#include "utils/strbuf.h"
 
 #ifdef __cplusplus
 extern "C" {
@@ -50,6 +51,9 @@ typedef struct {
 
        /* parser mode */
        int mode;
+
+       /* buffer for parser error messages */
+       sdb_strbuf_t *errbuf;
 } sdb_fe_yyextra_t;
 
 /* see yyscan_t */
@@ -65,10 +69,10 @@ int
 sdb_fe_yyparse(sdb_fe_yyscan_t scanner);
 
 sdb_store_matcher_t *
-sdb_fe_parse_matcher(const char *cond, int len);
+sdb_fe_parse_matcher(const char *cond, int len, sdb_strbuf_t *errbuf);
 
 sdb_store_expr_t *
-sdb_fe_parse_expr(const char *expr, int len);
+sdb_fe_parse_expr(const char *expr, int len, sdb_strbuf_t *errbuf);
 
 /*
  * sdb_fe_analyze:

diff --git a/t/unit/core/store_lookup_test.c b/t/unit/core/store_lookup_test.c
index 698226e23db360ac792a9a4776821c71da20e0f6..c256b232ae049096eaace1a86652ff84a77b6e5d 100644 (file)
--- a/t/unit/core/store_lookup_test.c
+++ b/t/unit/core/store_lookup_test.c
@@ -596,6 +596,7 @@ START_TEST(test_scan)
                  "AND attribute['y'] !~ 'x'", NULL,   2 },
        };
 
+       sdb_strbuf_t *errbuf = sdb_strbuf_create(64);
        int check, n;
        size_t i;
 
@@ -610,16 +611,18 @@ START_TEST(test_scan)
        for (i = 0; i < SDB_STATIC_ARRAY_LEN(golden_data); ++i) {
                sdb_store_matcher_t *m, *filter = NULL;
 
-               m = sdb_fe_parse_matcher(golden_data[i].query, -1);
+               m = sdb_fe_parse_matcher(golden_data[i].query, -1, errbuf);
                fail_unless(m != NULL,
-                               "sdb_fe_parse_matcher(%s, -1) = NULL; expected: <matcher>",
-                               golden_data[i].query);
+                               "sdb_fe_parse_matcher(%s, -1) = NULL; expected: <matcher> "
+                               "(parser error: %s)", golden_data[i].query,
+                               sdb_strbuf_string(errbuf));
 
                if (golden_data[i].filter) {
-                       filter = sdb_fe_parse_matcher(golden_data[i].filter, -1);
+                       filter = sdb_fe_parse_matcher(golden_data[i].filter, -1, errbuf);
                        fail_unless(filter != NULL,
                                        "sdb_fe_parse_matcher(%s, -1) = NULL; "
-                                       "expected: <matcher>", golden_data[i].filter);
+                                       "expected: <matcher> (parser error: %s)",
+                                       golden_data[i].filter, sdb_strbuf_string(errbuf));
                }
 
                n = 0;
@@ -631,6 +634,8 @@ START_TEST(test_scan)
                sdb_object_deref(SDB_OBJ(filter));
                sdb_object_deref(SDB_OBJ(m));
        }
+
+       sdb_strbuf_destroy(errbuf);
 }
 END_TEST
 

diff --git a/t/unit/frontend/parser_test.c b/t/unit/frontend/parser_test.c
index ec8264c82b6c495bd1e9ff2f0a250d7e32cc8280..8af046a1de93c4ece48f5dbc6c3d3d3bcf983579 100644 (file)
--- a/t/unit/frontend/parser_test.c
+++ b/t/unit/frontend/parser_test.c
@@ -264,22 +264,26 @@ START_TEST(test_parse)
                  "'f' || oo",           -1, -1, 0 },
        };
 
-       size_t i;
+       sdb_strbuf_t *errbuf = sdb_strbuf_create(64);
        sdb_llist_t *check;
 
+       size_t i;
+
        for (i = 0; i < SDB_STATIC_ARRAY_LEN(golden_data); ++i) {
                sdb_object_t *obj;
                _Bool ok;
 
-               check = sdb_fe_parse(golden_data[i].query, golden_data[i].len);
+               check = sdb_fe_parse(golden_data[i].query,
+                               golden_data[i].len, errbuf);
                if (golden_data[i].expected < 0)
                        ok = check == 0;
                else
                        ok = sdb_llist_len(check) == (size_t)golden_data[i].expected;
 
-               fail_unless(ok, "sdb_fe_parse(%s) = %p (len: %zu); expected: %d",
-                               golden_data[i].query, check, sdb_llist_len(check),
-                               golden_data[i].expected);
+               fail_unless(ok, "sdb_fe_parse(%s) = %p (len: %zu); expected: %d "
+                               "(parser error: %s)", golden_data[i].query, check,
+                               sdb_llist_len(check), golden_data[i].expected,
+                               sdb_strbuf_string(errbuf));
 
                if (! check)
                        continue;
@@ -298,6 +302,8 @@ START_TEST(test_parse)
                sdb_object_deref(obj);
                sdb_llist_destroy(check);
        }
+
+       sdb_strbuf_destroy(errbuf);
 }
 END_TEST
 
@@ -447,11 +453,13 @@ START_TEST(test_parse_matcher)
                { "invalid",                      -1, -1 },
        };
 
+       sdb_strbuf_t *errbuf = sdb_strbuf_create(64);
        size_t i;
 
        for (i = 0; i < SDB_STATIC_ARRAY_LEN(golden_data); ++i) {
                sdb_store_matcher_t *m;
-               m = sdb_fe_parse_matcher(golden_data[i].expr, golden_data[i].len);
+               m = sdb_fe_parse_matcher(golden_data[i].expr,
+                               golden_data[i].len, errbuf);
 
                if (golden_data[i].expected < 0) {
                        fail_unless(m == NULL,
@@ -461,7 +469,8 @@ START_TEST(test_parse_matcher)
                }
 
                fail_unless(m != NULL, "sdb_fe_parse_matcher(%s) = NULL; "
-                               "expected: <matcher>", golden_data[i].expr);
+                               "expected: <matcher> (parser error: %s)",
+                               golden_data[i].expr, sdb_strbuf_string(errbuf));
                fail_unless(M(m)->type == golden_data[i].expected,
                                "sdb_fe_parse_matcher(%s) returned matcher of type %d; "
                                "expected: %d", golden_data[i].expr, M(m)->type,
@@ -469,6 +478,8 @@ START_TEST(test_parse_matcher)
 
                sdb_object_deref(SDB_OBJ(m));
        }
+
+       sdb_strbuf_destroy(errbuf);
 }
 END_TEST
 
@@ -536,11 +547,13 @@ START_TEST(test_parse_expr)
                { "invalid",              -1, INT_MAX },
        };
 
+       sdb_strbuf_t *errbuf = sdb_strbuf_create(64);
        size_t i;
 
        for (i = 0; i < SDB_STATIC_ARRAY_LEN(golden_data); ++i) {
                sdb_store_expr_t *e;
-               e = sdb_fe_parse_expr(golden_data[i].expr, golden_data[i].len);
+               e = sdb_fe_parse_expr(golden_data[i].expr,
+                               golden_data[i].len, errbuf);
 
                if (golden_data[i].expected == INT_MAX) {
                        fail_unless(e == NULL,
@@ -550,7 +563,8 @@ START_TEST(test_parse_expr)
                }
 
                fail_unless(e != NULL, "sdb_fe_parse_expr(%s) = NULL; "
-                               "expected: <expr>", golden_data[i].expr);
+                               "expected: <expr> (parser error: %s)",
+                               golden_data[i].expr, sdb_strbuf_string(errbuf));
                fail_unless(e->type == golden_data[i].expected,
                                "sdb_fe_parse_expr(%s) returned expression of type %d; "
                                "expected: %d", golden_data[i].expr, e->type,
@@ -558,6 +572,8 @@ START_TEST(test_parse_expr)
 
                sdb_object_deref(SDB_OBJ(e));
        }
+
+       sdb_strbuf_destroy(errbuf);
 }
 END_TEST