Improve line/error reporting in server config parsing

2 files changed, 86 insertions, 148 deletions

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);