Return some sanity to DNS handling

7 files changed, 187 insertions, 241 deletions

diff --git a/minecproxy/server-config.c b/minecproxy/server-config.c
index 1bc3d38..aab9ffb 100644
--- a/minecproxy/server-config.c
+++ b/minecproxy/server-config.c
@@ -16,6 +16,18 @@
 #include "server-config.h"
 #include "config.h"
 
+void server_cfg_async_dns_result(struct server_config *scfg)
+{
+	struct server *server = container_of(scfg, struct server, scfg);
+
+	scfg_async_dns_result(scfg);
+
+	if (list_empty(&server->scfg.dnslookups)) {
+		uring_task_put(&server->task);
+		server_commit(server);
+	}
+}
+
 static void scfg_read_cb(struct uring_task *task, int res)
 {
 	struct server *server = container_of(task, struct server, task);
@@ -34,14 +46,20 @@ static void scfg_read_cb(struct uring_task *task, int res)
 	debug(DBG_CFG, "%s: parsing cfg (%i bytes)", server->name, res);
 	uring_task_close_fd(&server->task);
 	
-	if (!scfg_parse(&server->scfg, server->tbuf.buf,
-			server_async_dns_update, &lineno, &error)) {
+	if (!scfg_parse(&server->scfg, server->tbuf.buf, true,
+			&lineno, &error)) {
 		error("%s: failed to parse config file, line %u: %s",
 		      server->name, lineno, error);
 		server_delete(server);
 		return;
 	}
 
+	if (!list_empty(&server->scfg.dnslookups)) {
+		uring_task_get(&server->task);
+		scfg_async_dns_start(&server->scfg);
+		return;
+	}
+
 	server_commit(server);
 }
 
diff --git a/minecproxy/server-config.h b/minecproxy/server-config.h
index 590dae0..4ccba19 100644
--- a/minecproxy/server-config.h
+++ b/minecproxy/server-config.h
@@ -1,6 +1,8 @@
 #ifndef fooserverconfighfoo
 #define fooserverconfighfoo
 
+void server_cfg_async_dns_result(struct server_config *scfg);
+
 void server_cfg_monitor_delete();
 
 void server_cfg_monitor_refdump();
diff --git a/minecproxy/server.c b/minecproxy/server.c
index f1f0006..13396ef 100644
--- a/minecproxy/server.c
+++ b/minecproxy/server.c
@@ -328,18 +328,6 @@ void server_update_active_players(struct server *server, int count)
 	}
 }
 
-void server_async_dns_update(struct server_config *scfg, bool done)
-{
-	struct server *server = container_of(scfg, struct server, scfg);
-
-	if (done) {
-		uring_task_put(&server->task);
-		server_commit(server);
-	} else {
-		uring_task_get(&server->task);
-	}
-}
-
 static void server_idle_connected_cb(struct connection *conn, bool connected)
 {
 	struct server *server = container_of(conn, struct server, idle_conn);
diff --git a/minecproxy/server.h b/minecproxy/server.h
index c6ea252..6545903 100644
--- a/minecproxy/server.h
+++ b/minecproxy/server.h
@@ -57,8 +57,6 @@ bool server_stop(struct server *server);
 
 void server_update_active_players(struct server *server, int count);
 
-void server_async_dns_update(struct server_config *scfg, bool done);
-
 bool server_idle_check(struct server *server);
 
 bool server_announce(struct server *server, int fd);
diff --git a/minecproxy/signal-handler.c b/minecproxy/signal-handler.c
index 5369670..23d72fa 100644
--- a/minecproxy/signal-handler.c
+++ b/minecproxy/signal-handler.c
@@ -9,7 +9,6 @@
 #include "uring.h"
 #include "server.h"
 #include "server-config.h"
-#include "config-parser.h"
 #include "igmp.h"
 #include "announce.h"
 #include "idle.h"
@@ -46,23 +45,13 @@ static void signalfd_read(struct uring_task *task, int res)
 		si = &sia[i];
 		switch (si->si_signo) {
 		case SIGUSR1: {
-			struct dns_async *dns;
-
 			debug(DBG_SIG, "Got a SIGUSR1");
 			if (si->si_code != SI_ASYNCNL || !si->si_ptr) {
 				error("SIGUSR1: unexpected values in siginfo");
 				goto out;
 			}
 
-			dns = si->si_ptr;
-			if (!dns->cb) {
-				error("DNS callback not set (%p)", dns);
-				goto out;
-			}
-
-			debug(DBG_DNS, "DNS lookup complete, dns: %p, dns->cb: %p", dns,
-			      dns->cb);
-			dns->cb(dns);
+			server_cfg_async_dns_result(si->si_ptr);
 			break;
 		}
 
diff --git a/shared/config-parser.c b/shared/config-parser.c
index 878262e..b8abecb 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -14,61 +14,25 @@
 #include "server-config-options.h"
 #include "config.h"
 
-static void handle_dns_reply(struct server_config *scfg, enum scfg_keys type,
-			     struct saddr *saddr)
+static bool handle_addrinfo_results(struct addrinfo *results,
+				    struct list_head *target_list,
+				    unsigned *naddrs)
 {
-	switch (type) {
-	case SCFG_KEY_LOCAL:
-		list_add(&saddr->list, &scfg->locals);
-		break;
-	case SCFG_KEY_REMOTE:
-		list_add(&saddr->list, &scfg->remotes);
-		break;
-	case SCFG_KEY_RCON:
-		list_add(&saddr->list, &scfg->rcons);
-		break;
-	default:
-		error("invalid DNS reply type");
-		break;
-	}
-}
-
-static void scfg_dns_cb(struct dns_async *dns, enum scfg_keys type)
-{
-	struct server_config *scfg;
 	struct sockaddr_in *in4;
 	struct sockaddr_in6 *in6;
 	struct saddr *saddr;
-	struct addrinfo *results = NULL, *ai;
-	int r;
-
-	assert_return(dns);
-
-	scfg = dns->priv;
-	debug(DBG_DNS, "called, dns: %p, name: %s", dns, dns->name);
-
-	r = gai_error(&dns->gcb);
-	if (r == EAI_INPROGRESS) {
-		/* This shouldn't happen, assume we'll get called again */
-		error("called with request in progress");
-		return;
-	} else if (r == EAI_CANCELED) {
-		/* The server must be in the process of going away */
-		goto out;
-	} else if (r < 0) {
-		error("DNS lookup of %s:%s failed: %s", dns->name, dns->port,
-		      gai_strerror(r));
-		goto out;
-	}
-
-	results = dns->gcb.ar_result;
+	struct addrinfo *ai;
+	bool rv = true;
 
 	for (ai = results; ai; ai = ai->ai_next) {
+		if (ai->ai_family != AF_INET &&
+		    ai->ai_family != AF_INET6)
+			continue;
+
 		saddr = zmalloc(sizeof(*saddr));
 		if (!saddr) {
-			error("DNS lookup of %s:%s failed: %m", dns->name,
-			      dns->port);
-			goto out;
+			error("DNS lookup failed: %m");
+			rv = false;
 		}
 
 		switch (ai->ai_family) {
@@ -76,105 +40,78 @@ static void scfg_dns_cb(struct dns_async *dns, enum scfg_keys type)
 			in4 = (struct sockaddr_in *)ai->ai_addr;
 			saddr_set_ipv4(saddr, in4->sin_addr.s_addr,
 				       in4->sin_port);
+			(*naddrs)++;
 			break;
 
 		case AF_INET6:
 			in6 = (struct sockaddr_in6 *)ai->ai_addr;
 			saddr_set_ipv6(saddr, &in6->sin6_addr, in6->sin6_port);
 			break;
-
-		default:
-			error("getaddrinfo(%s:%s): unknown address family (%i)",
-			      dns->name, dns->port, ai->ai_family);
-			xfree(saddr);
-			continue;
+			(*naddrs)++;
 		}
 
-		handle_dns_reply(scfg, type, saddr);
+		list_add(&saddr->list, target_list);
 	}
 
-out:
 	freeaddrinfo(results);
-	list_del(&dns->list);
-	if (dns->notification_cb)
-		dns->notification_cb(scfg, true);
-	xfree(dns);
-}
-
-static void scfg_dns_local_cb(struct dns_async *dns)
-{
-	scfg_dns_cb(dns, SCFG_KEY_LOCAL);
-}
-
-static void scfg_dns_remote_cb(struct dns_async *dns)
-{
-	scfg_dns_cb(dns, SCFG_KEY_REMOTE);
+	return rv;
 }
 
-static void scfg_dns_rcon_cb(struct dns_async *dns)
+void scfg_async_dns_result(struct server_config *scfg)
 {
-	scfg_dns_cb(dns, SCFG_KEY_RCON);
-}
-
-/*
- * FIXME: this is brittle, the dns request has already been triggered,
- * can't just cancel on error.
- */
-static bool handle_dns(struct server_config *scfg, enum scfg_keys type,
-		       struct cfg_value *value,
-		       notification_cb_t notification_cb)
-{
-	struct saddr *saddr, *tmp;
-	struct dns_async *dns;
-
-	assert_return(scfg && type && value, false);
+	struct dns_async *dns, *tmp;
+	unsigned naddrs = 0;
+	bool alldone = true;
+	int r;
 
-	switch (value->type) {
-	case CFG_VAL_TYPE_ADDRS:
-		debug(DBG_DNS, "%i: got immediate addrs", type);
+	assert_return(scfg);
 
-		list_for_each_entry_safe(saddr, tmp, &value->saddrs, list) {
-			list_del(&saddr->list);
-			handle_dns_reply(scfg, type, saddr);
-		}
-		return true;
+	debug(DBG_DNS, "called, scfg: %p", scfg);
 
-	case CFG_VAL_TYPE_ASYNC_ADDRS:
-		debug(DBG_DNS, "doing async lookup of DNS record: %p",
-		      value->dns_async);
+	list_for_each_entry(dns, &scfg->dnslookups, list) {
+		if (!dns->pending)
+			continue;
 
-		dns = value->dns_async;
-		switch (type) {
-		case SCFG_KEY_LOCAL:
-			dns->cb = scfg_dns_local_cb;
+		r = gai_error(&dns->gcb);
+		switch (r) {
+		case EAI_INPROGRESS:
+			/* Assume we'll get called again */
+			alldone = false;
 			break;
-		case SCFG_KEY_REMOTE:
-			dns->cb = scfg_dns_remote_cb;
+
+		case EAI_CANCELED:
+			/* The server must be in the process of going away */
+			dns->pending = false;
 			break;
-		case SCFG_KEY_RCON:
-			dns->cb = scfg_dns_rcon_cb;
+
+		case 0:
+			/* Success */
+			dns->pending = false;
+			handle_addrinfo_results(dns->gcb.ar_result,
+						dns->target_list,
+						&naddrs);
+			debug(DBG_DNS, "async DNS added %u records", naddrs);
 			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");
-			/* Ditto */
-			return false;
+		default:
+			/* Misc failures */
+			dns->pending = false;
+			error("DNS lookup of %s:%s failed: %s",
+			      dns->name, dns->port, gai_strerror(r));
+			break;
 		}
+	}
 
-		dns->priv = scfg;
-		dns->notification_cb = notification_cb;
-		list_add(&dns->list, &scfg->dnslookups);
-		dns->notification_cb(scfg, false);
-		return true;
+	if (!alldone)
+		return;
 
-	default:
-		return false;
+	list_for_each_entry_safe(dns, tmp, &scfg->dnslookups, list) {
+		list_del(&dns->list);
+		xfree(dns);
 	}
+
+	xfree(scfg->gcbs);
+	scfg->gcbs = NULL;
 }
 
 static inline char tohex(uint8_t val)
@@ -318,9 +255,69 @@ bool scfg_validate(struct server_config *scfg, const char **error)
 	return true;
 }
 
-bool scfg_parse(struct server_config *scfg, char *buf,
-		notification_cb_t notification_cb, unsigned *lineno,
-		const char **error)
+bool scfg_async_dns_start(struct server_config *scfg)
+{
+	struct dns_async *dns;
+	unsigned ndns = 0;
+	int r;
+
+	assert_return(scfg, false);
+
+	list_for_each_entry(dns, &scfg->dnslookups, list)
+		ndns++;
+
+	scfg->gcbs = zmalloc(sizeof(struct gaicb) * ndns);
+	if (!scfg->gcbs) {
+		error("failed to allocate gcbs: %m");
+		return false;
+	}
+
+	ndns = 0;
+	list_for_each_entry(dns, &scfg->dnslookups, list) {
+		scfg->gcbs[ndns++] = &dns->gcb;
+		dns->pending = true;
+	}
+
+	r = getaddrinfo_a(GAI_NOWAIT, scfg->gcbs, ndns, &scfg->dns_sev);
+	if (r != 0) {
+		error("getaddrinfo_a(%u): %s", ndns, gai_strerror(r));
+		/* FIXME: More error handling */
+	}
+
+	return true;
+}
+
+static void scfg_handle_dns(struct server_config *scfg, struct cfg_value *value,
+			    struct list_head *target_list)
+{
+	struct saddr *saddr, *tmp;
+	struct dns_async *dns;
+
+	switch (value->type) {
+	case CFG_VAL_TYPE_ADDRS:
+		debug(DBG_DNS, "got sync results");
+		list_for_each_entry_safe(saddr, tmp, &value->saddrs, list) {
+			list_del(&saddr->list);
+			list_add(&saddr->list, target_list);
+		}
+		break;
+
+	case CFG_VAL_TYPE_ASYNC_ADDRS:
+		dns = value->dns_async;
+		debug(DBG_DNS, "enqueing async lookup of %s:%s (%p)",
+		      dns->name, dns->port, dns);
+		dns->target_list = target_list;
+		list_add(&dns->list, &scfg->dnslookups);
+		break;
+
+	default:
+		error("Unexpected value type");
+		break;
+	}
+}
+
+bool scfg_parse(struct server_config *scfg, char *buf, bool async,
+		unsigned *lineno, const char **error)
 {
 	char *pos;
 
@@ -339,7 +336,7 @@ 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, async, lineno))
 			break;
 
 		if (key == SCFG_KEY_INVALID)
@@ -376,15 +373,11 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 			break;
 
 		case SCFG_KEY_LOCAL:
-			if (!handle_dns(scfg, SCFG_KEY_LOCAL, &value,
-					notification_cb))
-				ERROR("handle_dns failed");
+			scfg_handle_dns(scfg, &value, &scfg->locals);
 			break;
 
 		case SCFG_KEY_REMOTE:
-			if (!handle_dns(scfg, SCFG_KEY_REMOTE, &value,
-					notification_cb))
-				ERROR("handle_dns failed");
+			scfg_handle_dns(scfg, &value, &scfg->remotes);
 			break;
 
 		case SCFG_KEY_IDLE_TIMEOUT:
@@ -433,9 +426,7 @@ bool scfg_parse(struct server_config *scfg, char *buf,
 			break;
 
 		case SCFG_KEY_RCON:
-			if (!handle_dns(scfg, SCFG_KEY_RCON, &value,
-					notification_cb))
-				ERROR("handle_dns failed");
+			scfg_handle_dns(scfg, &value, &scfg->rcons);
 			break;
 
 		case SCFG_KEY_RCON_PASSWORD:
@@ -514,8 +505,10 @@ void scfg_delete(struct server_config *scfg)
 		xfree(saddr);
 	}
 
-	list_for_each_entry(dns, &scfg->dnslookups, list)
-		gai_cancel(&dns->gcb);
+	list_for_each_entry(dns, &scfg->dnslookups, list) {
+		if (dns->pending)
+			gai_cancel(&dns->gcb);
+	}
 }
 
 bool scfg_init(struct server_config *scfg, const char *filename)
@@ -547,6 +540,11 @@ bool scfg_init(struct server_config *scfg, const char *filename)
 	INIT_LIST_HEAD(&scfg->locals);
 	INIT_LIST_HEAD(&scfg->rcons);
 	INIT_LIST_HEAD(&scfg->dnslookups);
+	scfg->gcbs = NULL;
+	scfg->dns_sev.sigev_notify = SIGEV_SIGNAL;
+	scfg->dns_sev.sigev_signo = SIGUSR1;
+	scfg->dns_sev.sigev_value.sival_ptr = scfg;
+
 	return true;
 }
 
@@ -608,17 +606,13 @@ static char *get_line(char **pos, unsigned *lineno)
 	return begin;
 }
 
-static bool dnslookup(const char *name, uint16_t port, struct cfg_value *rvalue,
-		      unsigned *naddrs, bool async)
+/* FIXME: we should gather these up and do all in one go */
+static bool cfg_dnslookup(const char *name, uint16_t port, struct cfg_value *rvalue,
+			  unsigned *naddrs, bool async)
 {
-	struct sockaddr_in *in4;
-	struct sockaddr_in6 *in6;
 	struct dns_async tmp;
 	struct dns_async *dns;
-	int mode = async ? GAI_NOWAIT : GAI_WAIT;
-	struct addrinfo *results = NULL, *ai;
-	struct saddr *saddr = NULL;
-	bool rv = false;
+	struct addrinfo *results = NULL;
 	int r;
 
 	assert_return(!empty_str(name) && strlen(name) < sizeof(dns->name) &&
@@ -631,9 +625,17 @@ static bool dnslookup(const char *name, uint16_t port, struct cfg_value *rvalue,
 		dns = zmalloc(sizeof(*dns));
 		if (!dns) {
 			error("async DNS lookup of %s failed: %m", name);
-			goto out;
+			return false;
 		}
 		debug(DBG_DNS, "doing async DNS lookup of %s: %p", name, dns);
+
+		dns->pending = false;
+
+		dns->gcb.ar_name = dns->name;
+		dns->gcb.ar_service = dns->port;
+		dns->gcb.ar_request = &dns->req;
+
+		rvalue->dns_async = dns;
 	} else {
 		memset(&tmp, 0, sizeof(tmp));
 		dns = &tmp;
@@ -648,70 +650,17 @@ static bool dnslookup(const char *name, uint16_t port, struct cfg_value *rvalue,
 	dns->req.ai_protocol = 0;
 	dns->req.ai_flags = AI_NUMERICSERV;
 
-	dns->sev.sigev_notify = SIGEV_SIGNAL;
-	dns->sev.sigev_signo = SIGUSR1;
-	dns->sev.sigev_value.sival_ptr = dns;
-
-	dns->gcb.ar_name = dns->name;
-	dns->gcb.ar_service = dns->port;
-	dns->gcb.ar_request = &dns->req;
-
-	struct gaicb *gcbs[] = { &dns->gcb };
+	if (async)
+		return true;
 
-	r = getaddrinfo_a(mode, gcbs, ARRAY_SIZE(gcbs), &dns->sev);
+	r = getaddrinfo(dns->name, dns->port, &dns->req, &results);
 	if (r != 0) {
-		error("getaddrinfo(%s:%" PRIu16 "): %s", name, port,
+		error("getaddrinfo(%s:%s): %s", dns->name, dns->port,
 		      gai_strerror(r));
-		goto out;
-	}
-
-	if (async) {
-		rvalue->dns_async = dns;
-		rv = true;
-		goto out;
-	}
-
-	results = dns->gcb.ar_result;
-
-	/* FIXME: parts can be shared with the async cb? */
-	for (ai = results; ai; ai = ai->ai_next) {
-		saddr = zmalloc(sizeof(*saddr));
-		if (!saddr) {
-			error("sync DNS lookup of %s failed: %m", name);
-			goto out;
-		}
-
-		switch (ai->ai_family) {
-		case AF_INET:
-			in4 = (struct sockaddr_in *)ai->ai_addr;
-			saddr_set_ipv4(saddr, in4->sin_addr.s_addr,
-				       in4->sin_port);
-			debug(DBG_CFG, "addrstr: %s", saddr->addrstr);
-			list_add(&saddr->list, &rvalue->saddrs);
-			(*naddrs)++;
-			break;
-
-		case AF_INET6:
-			in6 = (struct sockaddr_in6 *)ai->ai_addr;
-			saddr_set_ipv6(saddr, &in6->sin6_addr, in6->sin6_port);
-			debug(DBG_CFG, "addrstr: %s", saddr->addrstr);
-			list_add(&saddr->list, &rvalue->saddrs);
-			(*naddrs)++;
-			break;
-
-		default:
-			error("getaddrinfo(%s:%s): unknown address family (%i)",
-			      dns->name, dns->port, ai->ai_family);
-			xfree(saddr);
-			break;
-		}
+		return false;
 	}
 
-	rv = true;
-
-out:
-	freeaddrinfo(results);
-	return rv;
+	return handle_addrinfo_results(results, &rvalue->saddrs, naddrs);
 }
 
 bool strtosockaddrs(const char *str, struct cfg_value *rvalue, bool async)
@@ -808,7 +757,7 @@ bool strtosockaddrs(const char *str, struct cfg_value *rvalue, bool async)
 		xfree(saddr);
 		debug(DBG_CFG, "maybe got a hostname:port (%s:%" PRIu16 ")",
 		      str, port);
-		if (!dnslookup(str, port, rvalue, &naddrs, async))
+		if (!cfg_dnslookup(str, port, rvalue, &naddrs, async))
 			goto error;
 
 	} else if (strtou16_strict(tmp, &port) == 0) {
diff --git a/shared/config-parser.h b/shared/config-parser.h
index ffce5d4..d84f9fe 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -42,7 +42,10 @@ struct server_config {
 	struct list_head locals;
 	struct list_head remotes;
 	struct list_head rcons;
+
 	struct list_head dnslookups;
+	struct gaicb **gcbs;
+	struct sigevent dns_sev;
 };
 
 enum cfg_value_type {
@@ -54,17 +57,13 @@ enum cfg_value_type {
 	CFG_VAL_TYPE_BOOL,
 };
 
-typedef void (*notification_cb_t)(struct server_config *scfg, bool done);
-
 struct dns_async {
 	char name[FQDN_STR_LEN + 1];
 	char port[PORT_STR_LEN + 1];
+	bool pending;
 	struct addrinfo req;
 	struct gaicb gcb;
-	struct sigevent sev;
-	void (*cb)(struct dns_async *);
-	notification_cb_t notification_cb;
-	void *priv;
+	struct list_head *target_list;
 	struct list_head list;
 };
 
@@ -85,11 +84,14 @@ struct cfg_value {
 	};
 };
 
+void scfg_async_dns_result(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, unsigned *lineno,
-		const char **error);
+bool scfg_async_dns_start(struct server_config *scfg);
+
+bool scfg_parse(struct server_config *scfg, char *buf, bool async,
+		unsigned *lineno, const char **error);
 
 void scfg_delete(struct server_config *scfg);