Improve line/error reporting in server config parsing

7 files changed, 112 insertions, 157 deletions

diff --git a/minecctl/misc-commands.c b/minecctl/misc-commands.c
index f7f2dea..a4188f9 100644
--- a/minecctl/misc-commands.c
+++ b/minecctl/misc-commands.c
@@ -20,6 +20,7 @@ bool do_list(struct cfg *cfg)
 bool do_lint(struct cfg *cfg)
 {
 	struct server *server;
+	const char *error;
 	bool rv = true;
 
 	/* server->scfg.filename check excludes servers created from cmdline */
@@ -32,8 +33,8 @@ bool do_lint(struct cfg *cfg)
 		/* FIXME: should return bool */
 		server_read_config(cfg, server);
 
-		if (!scfg_validate(&server->scfg)) {
-			error("%s: invalid", server->name);
+		if (!scfg_validate(&server->scfg, &error)) {
+			error("%s: invalid (%s)", server->name, error);
 			rv = false;
 		}
 	}
diff --git a/minecctl/server.c b/minecctl/server.c
index 21df7f4..658254f 100644
--- a/minecctl/server.c
+++ b/minecctl/server.c
@@ -10,6 +10,8 @@
 
 void server_read_config(struct cfg *cfg, struct server *server)
 {
+	unsigned lineno;
+	const char *error;
 	char buf[4096];
 	size_t off = 0;
 	ssize_t r;
@@ -47,8 +49,9 @@ void server_read_config(struct cfg *cfg, struct server *server)
 	buf[off] = '\0';
 	close(fd);
 
-	if (!scfg_parse(&server->scfg, buf, NULL))
-		die("Unable to parse %s", server->scfg.filename);
+	if (!scfg_parse(&server->scfg, buf, NULL, &lineno, &error))
+		die("Unable to parse %s, line %u: %s", server->scfg.filename,
+		    lineno, error);
 
 	if (!server->scfg.rcon_password)
 		verbose("rcon password not found in %s", server->scfg.filename);
diff --git a/minecproxy/server-config.c b/minecproxy/server-config.c
index de9265d..1bc3d38 100644
--- a/minecproxy/server-config.c
+++ b/minecproxy/server-config.c
@@ -19,6 +19,8 @@
 static void scfg_read_cb(struct uring_task *task, int res)
 {
 	struct server *server = container_of(task, struct server, task);
+	const char *error;
+	unsigned lineno;
 
 	assert_return(task);
 	assert_task_alive(DBG_CFG, task);
@@ -33,8 +35,9 @@ static void scfg_read_cb(struct uring_task *task, int res)
 	uring_task_close_fd(&server->task);
 	
 	if (!scfg_parse(&server->scfg, server->tbuf.buf,
-			server_async_dns_update)) {
-		error("%s: failed to parse config file", server->name);
+			server_async_dns_update, &lineno, &error)) {
+		error("%s: failed to parse config file, line %u: %s",
+		      server->name, lineno, error);
 		server_delete(server);
 		return;
 	}
diff --git a/minecproxy/server.c b/minecproxy/server.c
index b06286b..f1f0006 100644
--- a/minecproxy/server.c
+++ b/minecproxy/server.c
@@ -423,6 +423,7 @@ bool server_announce(struct server *server, int fd)
 
 bool server_commit(struct server *server)
 {
+	const char *error;
 	struct saddr *saddr, *tmp;
 	int r;
 
@@ -445,8 +446,9 @@ bool server_commit(struct server *server)
 		return false;
 	}
 
-	if (!scfg_validate(&server->scfg)) {
-		error("%s: failed to validate config file", server->name);
+	if (!scfg_validate(&server->scfg, &error)) {
+		error("%s: failed to validate config file (%s)",
+		      server->name, error);
 		server_delete(server);
 		return false;
 	}
@@ -468,7 +470,6 @@ bool server_commit(struct server *server)
 	list_for_each_entry_safe(saddr, tmp, &server->scfg.locals, list) {
 		struct server_local *local;
 
-		/* FIXME: error checks */
 		list_del(&saddr->list);
 		local = local_new(server, saddr);
 		if (!local) {
diff --git a/minecproxy/systemd.c b/minecproxy/systemd.c
index 96ff50f..3351f1c 100644
--- a/minecproxy/systemd.c
+++ b/minecproxy/systemd.c
@@ -1,6 +1,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <systemd/sd-bus.h>
+#include <errno.h>
 
 #include "main.h"
 #include "server.h"
@@ -58,11 +59,15 @@ bool systemd_service_running(struct server *server)
 			      server->scfg.systemd_obj,
 		      false);
 
+again:
 	r = sd_bus_get_property_string(bus, SYSTEMD_DBUS_SERVICE,
 				       server->scfg.systemd_obj,
 				       SYSTEMD_DBUS_INTERFACE, "ActiveState",
 				       &error, &status);
 	if (r < 0) {
+		if (sd_bus_error_get_errno(&error) == EINTR)
+			goto again;
+
 		error("failed to get status for service %s (%s): %s",
 		      server->scfg.systemd_service, server->scfg.systemd_obj,
 		      error.message);
@@ -96,10 +101,14 @@ static bool systemd_service_action(struct server *server, const char *action)
 			      server->scfg.systemd_obj && action,
 		      false);
 
+again:
 	r = sd_bus_call_method(bus, SYSTEMD_DBUS_SERVICE, server->scfg.systemd_obj,
 			       SYSTEMD_DBUS_INTERFACE, action, &error, &m, "s",
 			       "fail");
 	if (r < 0) {
+		if (sd_bus_error_get_errno(&error) == EINTR)
+			goto again;
+
 		error("failed to perform action %s on systemd service %s: %s",
 		      action, server->scfg.systemd_service, error.message);
 		goto out;
diff --git a/shared/config-parser.c b/shared/config-parser.c
index c238bb7..878262e 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -156,12 +156,13 @@ static bool handle_dns(struct server_config *scfg, enum scfg_keys type,
 			break;
 		default:
 			error("invalid DNS lookup type");
+			/* Note: we'll leak dns here, but not much we can do */
 			return false;
 		}
 
 		if (!notification_cb) {
 			error("async DNS lookup received but not requested");
-			xfree(dns);
+			/* Ditto */
 			return false;
 		}
 
@@ -225,169 +226,112 @@ static char *systemd_object_path(const char *service)
 	return r;
 }
 
-/* FIXME: Add a strict argument */
-bool scfg_validate(struct server_config *scfg)
+#define ERROR(msg) do { *error = (msg); return false; } while (0)
+
+bool scfg_validate(struct server_config *scfg, const char **error)
 {
 	struct saddr *saddr;
 	uint16_t port;
 
-	if (!scfg) {
-		error("invalid arguments");
-		return false;
-	}
+	assert_return(scfg && error, false);
 
-	if (!scfg->filename) {
-		error("filename not set");
-		return false;
-	}
+	if (!scfg)
+		ERROR("invalid arguments");
 
-	if (!list_empty(&scfg->dnslookups)) {
-		error("%s: pending DNS requests",
-		      scfg->filename);
-		return false;
-	}
+	if (!scfg->filename)
+		ERROR("filename not set");
+
+	if (!list_empty(&scfg->dnslookups))
+		ERROR("pending DNS requests");
 
 	if (scfg->stop_method == SERVER_STOP_METHOD_RCON &&
-	    list_empty(&scfg->rcons)) {
-		error("%s: rcon stop method missing rcon address",
-		      scfg->filename);
-		return false;
-	}
+	    list_empty(&scfg->rcons))
+		ERROR("rcon stop method missing rcon address");
 
 	if (scfg->stop_method == SERVER_STOP_METHOD_RCON &&
-	    !scfg->rcon_password) {
-		error("%s: rcon stop method missing rcon password",
-		      scfg->filename);
-		return false;
-	}
+	    !scfg->rcon_password)
+		ERROR("rcon stop method missing rcon password");
 
 	if ((scfg->start_method == SERVER_START_METHOD_SYSTEMD ||
 	     scfg->stop_method == SERVER_STOP_METHOD_SYSTEMD)) {
-		if (!scfg->systemd_service || !scfg->systemd_obj) {
-			error("%s: systemd start/stop method missing "
-			      "systemd service", scfg->filename);
-			return false;
-		}
+		if (!scfg->systemd_service || !scfg->systemd_obj)
+			ERROR("systemd start/stop missing service");
 	} else {
-		if (scfg->systemd_service || scfg->systemd_obj) {
-			error("%s: systemd service set but not used",
-			      scfg->filename);
-			return false;
-		}
+		if (scfg->systemd_service || scfg->systemd_obj)
+			ERROR("systemd service set but not used");
 	}
 
 	if (scfg->idle_timeout > 0 &&
-	    scfg->stop_method == SERVER_STOP_METHOD_UNDEFINED) {
-		error("%s: idle_timeout set but missing stop method",
-		      scfg->filename);
-		return false;
-	}
+	    scfg->stop_method == SERVER_STOP_METHOD_UNDEFINED)
+		ERROR("idle_timeout set but missing stop method");
 
 	switch (scfg->type) {
 	case SERVER_TYPE_ANNOUNCE:
-		if (scfg->announce_port == 0) {
-			error("%s: missing announce port", scfg->filename);
-			return false;
-		}
+		if (scfg->announce_port == 0)
+			ERROR("missing announce port");
 
-		if (scfg->start_method != SERVER_START_METHOD_UNDEFINED) {
-			error("%s: can't set start_method for announce server",
-			      scfg->filename);
-			return false;
-		}
+		if (scfg->start_method != SERVER_START_METHOD_UNDEFINED)
+			ERROR("can't set start_method for announce server");
 
-		if (scfg->start_exec) {
-			error("%s: can't set start_exec for announce server",
-			      scfg->filename);
-			return false;
-		}
+		if (scfg->start_exec)
+			ERROR("can't set start_exec for announce server");
 
-		if (!list_empty(&scfg->locals)) {
-			error("%s: can't set local addresses for announce server",
-			      scfg->filename);
-			return false;
-		}
+		if (!list_empty(&scfg->locals))
+			ERROR("can't set local addresses for announce server");
 
-		if (!list_empty(&scfg->remotes)) {
-			error("%s: can't set remote addresses for announce server",
-			      scfg->filename);
-			return false;
-		}
+		if (!list_empty(&scfg->remotes))
+			ERROR("can't set remote addresses for announce server");
 
 		break;
 
 	case SERVER_TYPE_PROXY:
-		if (scfg->announce_port != 0) {
-			error("%s: can't set announce port for proxy server",
-			      scfg->filename);
-			return false;
-		}
+		if (scfg->announce_port != 0)
+			ERROR("can't set announce port for proxy server");
 
-		if (list_empty(&scfg->locals)) {
-			error("%s: missing local addresses for proxy server",
-			      scfg->filename);
-			return false;
-		}
+		if (list_empty(&scfg->locals))
+			ERROR("missing local addresses for proxy server");
 
-		if (list_empty(&scfg->remotes)) {
-			error("%s: missing remote addresses for proxy server",
-			      scfg->filename);
-			return false;
-		}
+		if (list_empty(&scfg->remotes))
+			ERROR("missing remote addresses for proxy server");
 
 		list_for_each_entry(saddr, &scfg->locals, list) {
 			port = saddr_port(saddr);
 
-			if (port == 0) {
-				error("%s: invalid local port", scfg->filename);
-				return false;
-			}
+			if (port == 0)
+				ERROR("invalid local port");
 
 			if (scfg->announce_port == 0)
 				scfg->announce_port = port;
-			else if (scfg->announce_port != port) {
-				error("%s: multiple local ports", scfg->filename);
-				return false;
-			}
+			else if (scfg->announce_port != port)
+				ERROR("multiple local ports");
 		}
 
-		if (scfg->announce_port == 0) {
-			error("%s: can't determine which port to announce",
-			      scfg->filename);
-			return false;
-		}
+		if (scfg->announce_port == 0)
+			ERROR("can't determine which port to announce");
 
 		break;
 
 	default:
-		error("%s: can't determine server type", scfg->filename);
-		return false;
+		ERROR("can't determine server type");
 	}
 
 	return true;
 }
 
-#define INVALID(fmt, ...) \
-do { \
-	verbose("%s: " fmt, scfg->filename __VA_OPT__(,) __VA_ARGS__); \
-	valid = false; \
-} while(0)
-
 bool scfg_parse(struct server_config *scfg, char *buf,
-		notification_cb_t notification_cb)
+		notification_cb_t notification_cb, unsigned *lineno,
+		const char **error)
 {
-	unsigned lineno;
 	char *pos;
-	bool valid = true;
 
-	assert_return(scfg && buf, false);
+	assert_return(scfg && buf && lineno && error, false);
 
 	pos = buf;
 
-	if (!config_parse_header(SERVER_CFG_HEADER, &pos, &lineno)) {
-		INVALID("missing/invalid header");
-		goto out;
-	}
+	*lineno = 0;
+
+	if (!config_parse_header(SERVER_CFG_HEADER, &pos, lineno))
+		ERROR("missing/invalid header");
 
 	while (true) {
 		int key;
@@ -395,41 +339,38 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 		struct cfg_value value;
 
 		if (!config_parse_line(scfg->filename, &pos, scfg_key_map, &key,
-				       &keyname, &value, !!notification_cb, &lineno))
+				       &keyname, &value, !!notification_cb, lineno))
 			break;
 
-		/* FIXME: Improve error reporting */
-		if (key == SCFG_KEY_INVALID) {
-			INVALID("invalid line in config file");
-			continue;
-		}
+		if (key == SCFG_KEY_INVALID)
+			ERROR("invalid line in config file");
 
 		debug(DBG_CFG, "%s: key %s", scfg->filename, keyname);
 
 		switch (key) {
 		case SCFG_KEY_TYPE:
 			if (scfg->type != SERVER_TYPE_UNDEFINED)
-				INVALID("type defined multiple times");
+				ERROR("server type defined multiple times");
 			else if (streq(value.str, "proxy"))
 				scfg->type = SERVER_TYPE_PROXY;
 			else if (streq(value.str, "announce"))
 				scfg->type = SERVER_TYPE_ANNOUNCE;
 			else
-				INVALID("unknown type: %s", value.str);
+				ERROR("unknown server type");
 			break;
 
 		case SCFG_KEY_NAME:
 			if (scfg->pretty_name)
-				INVALID("name defined multiple times");
+				ERROR("server name defined multiple times");
 			else if (empty_str(value.str))
-				INVALID("name is an empty string");
+				ERROR("server name is an empty string");
 			else if (!(scfg->pretty_name = xstrdup(value.str)))
-				INVALID("strdup: %m");
+				ERROR("strdup failure");
 			break;
 
 		case SCFG_KEY_PORT:
 			if (scfg->announce_port != 0)
-				INVALID("port defined multiple times");
+				ERROR("port defined multiple times");
 			else
 				scfg->announce_port = value.uint16;
 			break;
@@ -437,25 +378,25 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 		case SCFG_KEY_LOCAL:
 			if (!handle_dns(scfg, SCFG_KEY_LOCAL, &value,
 					notification_cb))
-				INVALID("handle_dns failed");
+				ERROR("handle_dns failed");
 			break;
 
 		case SCFG_KEY_REMOTE:
 			if (!handle_dns(scfg, SCFG_KEY_REMOTE, &value,
 					notification_cb))
-				INVALID("handle_dns failed");
+				ERROR("handle_dns failed");
 			break;
 
 		case SCFG_KEY_IDLE_TIMEOUT:
 			if (scfg->idle_timeout != 0)
-				INVALID("idle_timeout already set");
+				ERROR("idle_timeout already set");
 			else
 				scfg->idle_timeout = value.uint16;
 			break;
 
 		case SCFG_KEY_STOP_METHOD:
 			if (scfg->stop_method != SERVER_STOP_METHOD_UNDEFINED)
-				INVALID("stop method defined multiple times");
+				ERROR("stop method defined multiple times");
 			else if (streq(value.str, "exec"))
 				scfg->stop_method = SERVER_STOP_METHOD_EXEC;
 			else if (streq(value.str, "rcon"))
@@ -463,50 +404,50 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 			else if (streq(value.str, "systemd"))
 				scfg->stop_method = SERVER_STOP_METHOD_SYSTEMD;
 			else
-				INVALID("unknown stop method: %s", value.str);
+				ERROR("unknown stop method");
 			break;
 
 		case SCFG_KEY_START_METHOD:
 			if (scfg->start_method != SERVER_START_METHOD_UNDEFINED)
-				INVALID("start method defined multiple times");
+				ERROR("start method defined multiple times");
 			else if (streq(value.str, "exec"))
 				scfg->start_method = SERVER_START_METHOD_EXEC;
 			else if (streq(value.str, "systemd"))
 				scfg->start_method = SERVER_START_METHOD_SYSTEMD;
 			else
-				INVALID("unknown start method: %s", value.str);
+				ERROR("unknown start method");
 			break;
 
 		case SCFG_KEY_STOP_EXEC:
 			if (scfg->stop_exec)
-				INVALID("stop exec defined multiple times");
+				ERROR("stop exec defined multiple times");
 			else if (!(scfg->stop_exec = xstrdup(value.str)))
-				INVALID("strdup: %m");
+				ERROR("strdup failure");
 			break;
 
 		case SCFG_KEY_START_EXEC:
 			if (scfg->start_exec)
-				INVALID("start exec defined multiple times");
+				ERROR("start exec defined multiple times");
 			else if (!(scfg->start_exec = xstrdup(value.str)))
-				INVALID("strdup: %m");
+				ERROR("strdup failure");
 			break;
 
 		case SCFG_KEY_RCON:
 			if (!handle_dns(scfg, SCFG_KEY_RCON, &value,
 					notification_cb))
-				INVALID("handle_dns failed");
+				ERROR("handle_dns failed");
 			break;
 
 		case SCFG_KEY_RCON_PASSWORD:
 			if (scfg->rcon_password)
-				INVALID("rcon_password defined multiple times");
+				ERROR("rcon_password defined multiple times");
 			else if (!(scfg->rcon_password = xstrdup(value.str)))
-				INVALID("strdup: %m");
+				ERROR("strdup failure");
 			break;
 
 		case SCFG_KEY_SYSTEMD_SERVICE:
 			if (scfg->systemd_service)
-				INVALID("systemd_service defined multiple times");
+				ERROR("systemd service defined multiple times");
 			else {
 				const char *suffix;
 				char *tmp;
@@ -521,29 +462,27 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 				} else
 					tmp = xstrdup(value.str);
 
-				if (!tmp) {
-					INVALID("malloc/strdup: %m");
-					break;
-				}
+				if (!tmp)
+					ERROR("malloc/strdup failure");
 
 				scfg->systemd_service = tmp;
 				scfg->systemd_obj = 
 					systemd_object_path(scfg->systemd_service);
 				if (!scfg->systemd_obj)
-					INVALID("object_path: %m");
+					ERROR("failed to create object_path");
 			}
 
 			break;
 
 		case SCFG_KEY_INVALID:
+			_fallthrough_;
 		default:
-			INVALID("invalid line");
+			ERROR("invalid line");
 			break;
 		}
 	}
 
-out:
-	return valid;
+	return true;
 }
 
 void scfg_delete(struct server_config *scfg)
@@ -1074,8 +1013,6 @@ bool config_parse_header(const char *title, char **buf, unsigned *lineno)
 
 	assert_return(!empty_str(title) && buf && *buf && lineno, false);
 
-	*lineno = 0;
-
 	line = get_line(buf, lineno);
 	if (line) {
 		char titlehdr[strlen(title) + 3];
diff --git a/shared/config-parser.h b/shared/config-parser.h
index 262fe09..ffce5d4 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -85,10 +85,11 @@ struct cfg_value {
 	};
 };
 
-bool scfg_validate(struct server_config *scfg);
+bool scfg_validate(struct server_config *scfg, const char **error);
 
 bool scfg_parse(struct server_config *scfg, char *buf,
-		notification_cb_t notification_cb);
+		notification_cb_t notification_cb, unsigned *lineno,
+		const char **error);
 
 void scfg_delete(struct server_config *scfg);