From 34bfa9790e6e7ed1ba9f1d4ed17fa34a73a1b064 Mon Sep 17 00:00:00 2001 From: Sebastian Harl Date: Wed, 5 Nov 2014 22:15:15 +0100 Subject: [PATCH] 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. --- src/frontend/grammar.y | 8 ++++- src/frontend/parser.c | 49 ++++++++++++++++++------------- src/frontend/query.c | 13 ++++---- src/frontend/scanner.l | 2 +- src/include/frontend/connection.h | 5 ++-- src/include/frontend/parser.h | 8 +++-- t/unit/core/store_lookup_test.c | 15 ++++++---- t/unit/frontend/parser_test.c | 34 +++++++++++++++------ 8 files changed, 87 insertions(+), 47 deletions(-) diff --git a/src/frontend/grammar.y b/src/frontend/grammar.y index 4dc6cb2..ab9345e 100644 --- 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 f8c82d3..936f31e 100644 --- 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 #include /* @@ -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 e5a1fc4..338d95a 100644 --- 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 c8d0dcb..f17b689 100644 --- 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 cbaa43e..b329fa1 100644 --- 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 e1c9c6f..e763afc 100644 --- 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 698226e..c256b23 100644 --- 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: ", - golden_data[i].query); + "sdb_fe_parse_matcher(%s, -1) = NULL; expected: " + "(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: ", golden_data[i].filter); + "expected: (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 ec8264c..8af046a 100644 --- 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: ", golden_data[i].expr); + "expected: (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: ", golden_data[i].expr); + "expected: (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 -- 2.30.2