Centralize scfg validation and use in minecctl to implement a lint command

7 files changed, 193 insertions, 135 deletions

diff --git a/minecctl/minecctl-commands.h b/minecctl/minecctl-commands.h
index e8bb132..b1729f0 100644
--- a/minecctl/minecctl-commands.h
+++ b/minecctl/minecctl-commands.h
@@ -4,6 +4,7 @@
 enum commands {
 	CMD_INVALID = 0,
 	CMD_LIST,
+	CMD_LINT,
 	CMD_STATUS,
 	CMD_PING,
 	CMD_STOP,
@@ -22,6 +23,10 @@ static struct command_list {
 			     .cmd = CMD_LIST,
 		     },
 		     {
+			     .name = "lint",
+			     .cmd = CMD_LINT,
+		     },
+		     {
 			     .name = "status",
 			     .cmd = CMD_STATUS,
 		     },
diff --git a/minecctl/minecctl.c b/minecctl/minecctl.c
index 231b150..69286fc 100644
--- a/minecctl/minecctl.c
+++ b/minecctl/minecctl.c
@@ -69,7 +69,7 @@ _noreturn_ static void usage(bool no_error)
 	     "  -m, --mc-address=ADDR    connect to Minecraft server at ADDR\n"
 	     "                           (only relevant for some commands, can also\n"
 	     "                            use environment variable MC_ADDRESS)\n"
-	     "  -c, --cfgdir=DIR         look for server configurations in DIR\n"
+	     "  -c, --cfgdir=DIR         look for server configuration files in DIR\n"
 	     "                           (default: %s)\n"
 	     "  -f, --force              stop server even if it has players\n"
 	     "  -v, --verbose            enable extra logging\n"
@@ -77,6 +77,7 @@ _noreturn_ static void usage(bool no_error)
 	     "\n"
 	     "Valid commands:\n"
 	     "  list                     list known servers\n"
+	     "  lint                     check validity of server configuration files\n"
 	     "  status [SERVER]          show status of SERVER (or all known servers)\n"
 	     "  ping [SERVER]            check if SERVER is running\n"
 	     "  stop [SERVER]            stop SERVER\n"
@@ -225,6 +226,13 @@ static void parse_command(struct cfg *cfg, char *const *argv)
 		}
 		cfg->cmd = do_list;
 		break;
+	case CMD_LINT:
+		if (*argv) {
+			error("Too many arguments");
+			usage(false);
+		}
+		cfg->cmd = do_lint;
+		break;
 	case CMD_STOPALL:
 		if (*argv) {
 			error("Too many arguments");
@@ -300,7 +308,7 @@ static void parse_cmdline(struct cfg *cfg, int argc, char *const *argv)
 
 	cfg->cfgdir = DEFAULT_CFG_DIR;
 
-	/* FIXME: add lint and debug options */
+	/* FIXME: add debug option */
 
 	while (true) {
 		int option_index = 0;
diff --git a/minecctl/misc-commands.c b/minecctl/misc-commands.c
index f20cac5..1268138 100644
--- a/minecctl/misc-commands.c
+++ b/minecctl/misc-commands.c
@@ -17,6 +17,30 @@ bool do_list(struct cfg *cfg)
 	return true;
 }
 
+bool do_lint(struct cfg *cfg)
+{
+	struct server *server;
+	bool rv = true;
+
+	/* server->scfg.filename check excludes servers created from cmdline */
+	list_for_each_entry(server, &cfg->servers, list) {
+		if (!server->scfg.filename)
+			continue;
+
+		info("%s", server->name);
+
+		/* FIXME: should return bool */
+		server_read_config(cfg, server);
+
+		if (!scfg_validate(&server->scfg)) {
+			error("%s: invalid", server->name);
+			rv = false;
+		}
+	}
+
+	return rv;
+}
+
 bool do_pcount(struct cfg *cfg)
 {
 	unsigned x, y;
diff --git a/minecctl/misc-commands.h b/minecctl/misc-commands.h
index 1a4363f..eb03fd9 100644
--- a/minecctl/misc-commands.h
+++ b/minecctl/misc-commands.h
@@ -3,6 +3,8 @@
 
 bool do_list(struct cfg *cfg);
 
+bool do_lint(struct cfg *cfg);
+
 bool do_pcount(struct cfg *cfg);
 
 #endif
diff --git a/minecproxy/server.c b/minecproxy/server.c
index 81d148b..a2f8332 100644
--- a/minecproxy/server.c
+++ b/minecproxy/server.c
@@ -17,6 +17,7 @@
 #include "server-config.h"
 #include "server-proxy.h"
 #include "server-rcon.h"
+#include "config-parser.h"
 #include "idle.h"
 #include "systemd.h"
 
@@ -422,7 +423,6 @@ bool server_announce(struct server *server, int fd)
 bool server_commit(struct server *server)
 {
 	struct saddr *saddr, *tmp;
-	uint16_t port;
 	int r;
 
 	assert_return(server && server->name, false);
@@ -439,143 +439,19 @@ bool server_commit(struct server *server)
 	}
 
 	if (!list_empty(&server->scfg.dnslookups)) {
-		debug(DBG_SRV, "called with pending DNS requests");
-		return true;
-	}
-
-	if (server->scfg.stop_method == SERVER_STOP_METHOD_RCON &&
-	    list_empty(&server->scfg.rcons)) {
-		error("%s: rcon stop method missing rcon address",
+		debug(DBG_SRV, "%s: called with pending DNS requests",
 		      server->name);
-		return false;
-	}
-
-	if (server->scfg.stop_method == SERVER_STOP_METHOD_RCON &&
-	    !server->scfg.rcon_password) {
-		error("%s: rcon stop method missing rcon password",
-		      server->name);
-		return false;
-	}
-
-	if ((server->scfg.start_method == SERVER_START_METHOD_SYSTEMD ||
-	     server->scfg.stop_method == SERVER_STOP_METHOD_SYSTEMD) &&
-	    !server->scfg.systemd_service) {
-		error("%s: systemd start/stop method missing systemd service",
-		      server->name);
-		return false;
-	}
-
-	if ((server->scfg.systemd_service && !server->scfg.systemd_obj) ||
-	    (!server->scfg.systemd_service && server->scfg.systemd_obj)) {
-		error("%s: systemd service but no object path or vice versa",
-		      server->name);
-		return false;
-	}
-
-	if (server->scfg.idle_timeout > 0 &&
-	    server->scfg.stop_method == SERVER_STOP_METHOD_UNDEFINED) {
-		error("%s: idle_timeout set but missing stop method",
-		      server->name);
-		return false;
+		return true;
 	}
 
-	switch (server->scfg.type) {
-	case SERVER_TYPE_ANNOUNCE:
-		if (server->scfg.announce_port == 0) {
-			error("%s: missing announce port", server->name);
-			return false;
-		}
-
-		if (server->scfg.start_method != SERVER_START_METHOD_UNDEFINED) {
-			error("%s: can't set start_method for announce server",
-			      server->name);
-			return false;
-		}
-
-		if (!list_empty(&server->scfg.locals)) {
-			error("%s: can't set local addresses for announce server",
-			      server->name);
-			return false;
-		}
-
-		if (!list_empty(&server->scfg.remotes)) {
-			error("%s: can't set remote addresses for announce server",
-			      server->name);
-			return false;
-		}
-
-		break;
-
-	case SERVER_TYPE_PROXY:
-		if (server->scfg.announce_port == 0) {
-			error("%s: can't set announce port for proxy server",
-			      server->name);
-			return false;
-		}
-
-		if (list_empty(&server->scfg.locals)) {
-			error("%s: missing local addresses for proxy server",
-			      server->name);
-			return false;
-		}
-
-		if (list_empty(&server->scfg.remotes)) {
-			error("%s: missing remote addresses for proxy server",
-			      server->name);
-			return false;
-		}
-
-		list_for_each_entry(saddr, &server->scfg.locals, list) {
-			port = saddr_port(saddr);
-
-			if (port == 0) {
-				error("%s: invalid local port", server->name);
-				return false;
-			}
-
-			if (server->scfg.announce_port == 0)
-				server->scfg.announce_port = port;
-
-			if (server->scfg.announce_port != port) {
-				error("%s: multiple local ports", server->name);
-				return false;
-			}
-		}
-
-		if (server->scfg.announce_port == 0) {
-			error("%s: can't determine which port to announce",
-			      server->name);
-			return false;
-		}
-
-		break;
-
-	default:
-		error("%s: can't determine server type", server->name);
+	if (!scfg_validate(&server->scfg)) {
+		debug(DBG_SRV, "%s: config file not valid", server->name);
 		return false;
 	}
 
-	if (!server->scfg.pretty_name) {
-		char *suffix;
-
-		suffix = strrchr(server->name, '.');
-		if (!suffix || suffix == server->name) {
-			error("invalid server name: %s", server->name);
-			return false;
-		}
-
-		server->scfg.pretty_name =
-			xstrndup(server->name, suffix - server->name);
-		if (!server->scfg.pretty_name) {
-			error("failed to create display name: %s",
-			      server->name);
-			return false;
-		}
-	}
-
 	r = snprintf(server->ann_buf.buf, sizeof(server->ann_buf.buf),
 		     "[MOTD]%s[/MOTD][AD]%" PRIu16 "[/AD]",
-		     server->scfg.pretty_name,
+		     server->scfg.pretty_name ? server->scfg.pretty_name : server->name,
 		     server->scfg.announce_port);
 	if (r < 1 || r >= sizeof(server->ann_buf.buf)) {
 		error("%s: unable to create announce msg: %i\n", server->name,
@@ -584,8 +460,7 @@ bool server_commit(struct server *server)
 	}
 	server->ann_buf.len = r;
 
-	/* FIXME: config, dont reread config if server running, make sure fd is
-	 * available before this is called */
+	/* FIXME: check interactions with inotify */
 	server_dump(server);
 
 	list_for_each_entry_safe(saddr, tmp, &server->scfg.locals, list) {
@@ -602,7 +477,7 @@ bool server_commit(struct server *server)
 
 	server_check_running(server);
 
-	debug(DBG_SRV, "success");
+	debug(DBG_SRV, "%s: success", server->name);
 	return true;
 }
 
diff --git a/shared/config-parser.c b/shared/config-parser.c
index ee30156..6c2d33d 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -221,6 +221,148 @@ static char *systemd_object_path(const char *service)
 	return r;
 }
 
+/* FIXME: Add a strict argument */
+bool scfg_validate(struct server_config *scfg)
+{
+	struct saddr *saddr;
+	uint16_t port;
+
+	if (!scfg) {
+		error("invalid arguments");
+		return false;
+	}
+
+	if (!scfg->filename) {
+		error("filename not set");
+		return false;
+	}
+
+	if (!list_empty(&scfg->dnslookups)) {
+		error("%s: pending DNS requests",
+		      scfg->filename);
+		return false;
+	}
+
+	if (scfg->stop_method == SERVER_STOP_METHOD_RCON &&
+	    list_empty(&scfg->rcons)) {
+		error("%s: rcon stop method missing rcon address",
+		      scfg->filename);
+		return false;
+	}
+
+	if (scfg->stop_method == SERVER_STOP_METHOD_RCON &&
+	    !scfg->rcon_password) {
+		error("%s: rcon stop method missing rcon password",
+		      scfg->filename);
+		return false;
+	}
+
+	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;
+		} else {
+			if (scfg->systemd_service || scfg->systemd_obj) {
+				error("%s: systemd service set but not used",
+				      scfg->filename);
+				return false;
+			}
+		}
+	}
+
+	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;
+	}
+
+	switch (scfg->type) {
+	case SERVER_TYPE_ANNOUNCE:
+		if (scfg->announce_port == 0) {
+			error("%s: missing announce port", scfg->filename);
+			return false;
+		}
+
+		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_exec) {
+			error("%s: can't set start_exec for announce server",
+			      scfg->filename);
+			return false;
+		}
+
+		if (!list_empty(&scfg->locals)) {
+			error("%s: can't set local addresses for announce server",
+			      scfg->filename);
+			return false;
+		}
+
+		if (!list_empty(&scfg->remotes)) {
+			error("%s: can't set remote addresses for announce server",
+			      scfg->filename);
+			return false;
+		}
+
+		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 (list_empty(&scfg->locals)) {
+			error("%s: missing local addresses for proxy server",
+			      scfg->filename);
+			return false;
+		}
+
+		if (list_empty(&scfg->remotes)) {
+			error("%s: missing remote addresses for proxy server",
+			      scfg->filename);
+			return false;
+		}
+
+		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 (scfg->announce_port == 0)
+				scfg->announce_port = port;
+			else if (scfg->announce_port != port) {
+				error("%s: multiple local ports", scfg->filename);
+				return false;
+			}
+		}
+
+		if (scfg->announce_port == 0) {
+			error("%s: can't determine which port to announce",
+			      scfg->filename);
+			return false;
+		}
+
+		break;
+
+	default:
+		error("%s: can't determine server type", scfg->filename);
+		return false;
+	}
+
+	return true;
+}
+
 #define INVALID(fmt, ...) \
 do { \
 	verbose("%s: " fmt, scfg->filename __VA_OPT__(,) __VA_ARGS__); \
diff --git a/shared/config-parser.h b/shared/config-parser.h
index 2220221..2990349 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -85,6 +85,8 @@ struct cfg_value {
 	};
 };
 
+bool scfg_validate(struct server_config *scfg);
+
 bool scfg_parse(struct server_config *scfg, char *buf,
 		notification_cb_t notification_cb);