From: Sebastian Harl <sh@tokkee.org>
Date: Wed, 5 Nov 2014 21:15:15 +0000 (+0100)
Subject: frontend: Improved parser error reporting.
X-Git-Tag: sysdb-0.6.0~27
X-Git-Url: https://git.tokkee.org/?a=commitdiff_plain;h=34bfa9790e6e7ed1ba9f1d4ed17fa34a73a1b064;p=sysdb.git

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 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 <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 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: <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 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: <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