Refactor the dns_geoip API to use ISC_THREAD_LOCAL

Previously, the dns_geoip API used isc_thread_key API for TLS, which is
fairly complicated and requires initialization of memory contexts, etc.
This part of code was refactored to use a ISC_THREAD_LOCAL pointer which
greatly simplifies the whole code related to storing TLS variables, and
creating the local memory context was moved to named and stored in the
named_g_geoip global context.

bin/named/geoip.c

@@ -113,8 +113,7 @@ named_geoip_load(char *dir) {
 #endif
 }
 
-void
-named_geoip_shutdown(void) {
+void named_geoip_unload(void) {
 #ifdef HAVE_GEOIP2
 	if (named_g_geoip->country != NULL) {
 		MMDB_close(named_g_geoip->country);
@@ -136,5 +135,12 @@ named_geoip_shutdown(void) {
 		MMDB_close(named_g_geoip->domain);
 		named_g_geoip->domain = NULL;
 	}
+#endif
+}
+
+void
+named_geoip_shutdown(void) {
+#ifdef HAVE_GEOIP2
+	named_geoip_unload();
 #endif /* HAVE_GEOIP2 */
 }

bin/named/include/named/geoip.h

@@ -19,5 +19,8 @@ named_geoip_init(void);
 void
 named_geoip_load(char *dir);
 
+void
+named_geoip_unload(void);
+
 void
 named_geoip_shutdown(void);

bin/named/server.c

@@ -8342,7 +8342,8 @@ load_configuration(const char *filename, named_server_t *server,
 	/*
 	 * Release any previously opened GeoIP2 databases.
 	 */
-	named_geoip_shutdown();
+	named_geoip_unload();
+
 	/*
 	 * Initialize GeoIP databases from the configured location.
 	 * This should happen before configuring any ACLs, so that we
@@ -9744,7 +9745,6 @@ shutdown_server(isc_task_t *task, isc_event_t *event) {
 	}
 #if defined(HAVE_GEOIP2)
 	named_geoip_shutdown();
-	dns_geoip_shutdown();
 #endif /* HAVE_GEOIP2 */
 
 	dns_db_detach(&server->in_roothints);
@@ -9866,7 +9866,7 @@ named_server_create(isc_mem_t *mctx, named_server_t **serverp) {
 	/*
 	 * GeoIP must be initialized before the interface
 	 * manager (which includes the ACL environment)
-	 * is created
+	 * is created.
 	 */
 	named_geoip_init();
 #endif /* HAVE_GEOIP2 */

lib/dns/geoip2.c

@@ -64,7 +64,6 @@
  */
 
 typedef struct geoip_state {
-	isc_mem_t *mctx;
 	uint16_t subtype;
 	const MMDB_s *db;
 	isc_netaddr_t addr;
@@ -72,117 +71,28 @@ typedef struct geoip_state {
 	MMDB_entry_s entry;
 } geoip_state_t;
 
-static isc_mutex_t key_mutex;
-static bool state_key_initialized = false;
-static isc_thread_key_t state_key;
-static isc_once_t mutex_once = ISC_ONCE_INIT;
-static isc_mem_t *state_mctx = NULL;
+ISC_THREAD_LOCAL geoip_state_t geoip_state = { 0 };
 
 static void
-key_mutex_init(void) {
-	isc_mutex_init(&key_mutex);
-}
-
-static void
-free_state(void *arg) {
-	geoip_state_t *state = arg;
-	if (state != NULL) {
-		isc_mem_putanddetach(&state->mctx,
-				     state, sizeof(geoip_state_t));
-	}
-	isc_thread_key_setspecific(state_key, NULL);
-}
-
-static isc_result_t
-state_key_init(void) {
-	isc_result_t result;
-
-	result = isc_once_do(&mutex_once, key_mutex_init);
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-
-	if (!state_key_initialized) {
-		LOCK(&key_mutex);
-		if (!state_key_initialized) {
-			int ret;
-
-			if (state_mctx == NULL) {
-				isc_mem_create(&state_mctx);
-			}
-			isc_mem_setname(state_mctx, "geoip_state", NULL);
-			isc_mem_setdestroycheck(state_mctx, false);
-
-			ret = isc_thread_key_create(&state_key, free_state);
-			if (ret == 0) {
-				state_key_initialized = true;
-			} else {
-				result = ISC_R_FAILURE;
-			}
-		}
-		UNLOCK(&key_mutex);
-	}
-
-	return (result);
-}
-
-static isc_result_t
 set_state(const MMDB_s *db, const isc_netaddr_t *addr,
-	  MMDB_lookup_result_s mmresult, MMDB_entry_s entry,
-	  geoip_state_t **statep)
+	  MMDB_lookup_result_s mmresult, MMDB_entry_s entry)
 {
-	geoip_state_t *state = NULL;
-	isc_result_t result;
-
-	result = state_key_init();
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-
-	state = (geoip_state_t *) isc_thread_key_getspecific(state_key);
-	if (state == NULL) {
-		state = isc_mem_get(state_mctx, sizeof(geoip_state_t));
-		memset(state, 0, sizeof(*state));
-
-		result = isc_thread_key_setspecific(state_key, state);
-		if (result != ISC_R_SUCCESS) {
-			isc_mem_put(state_mctx, state, sizeof(geoip_state_t));
-			return (result);
-		}
-
-		isc_mem_attach(state_mctx, &state->mctx);
-	}
-
-	state->db = db;
-	state->addr = *addr;
-	state->mmresult = mmresult;
-	state->entry = entry;
-
-	if (statep != NULL) {
-		*statep = state;
-	}
-
-	return (ISC_R_SUCCESS);
+	geoip_state.db = db;
+	geoip_state.addr = *addr;
+	geoip_state.mmresult = mmresult;
+	geoip_state.entry = entry;
 }
 
 static geoip_state_t *
 get_entry_for(MMDB_s * const db, const isc_netaddr_t *addr) {
-	isc_result_t result;
 	isc_sockaddr_t sa;
-	geoip_state_t *state;
 	MMDB_lookup_result_s match;
 	int err;
 
-	result = state_key_init();
-	if (result != ISC_R_SUCCESS) {
-		return (NULL);
-	}
-
-	state = (geoip_state_t *) isc_thread_key_getspecific(state_key);
-	if (state != NULL) {
-		if (db == state->db && isc_netaddr_equal(addr, &state->addr)) {
-			return (state);
-		}
+	if (db == geoip_state.db &&
+	    isc_netaddr_equal(addr, &geoip_state.addr))
+	{
+		return (&geoip_state);
 	}
 
 	isc_sockaddr_fromnetaddr(&sa, addr, 0);
@@ -191,12 +101,9 @@ get_entry_for(MMDB_s * const db, const isc_netaddr_t *addr) {
 		return (NULL);
 	}
 
-	result = set_state(db, addr, match, match.entry, &state);
-	if (result != ISC_R_SUCCESS) {
-		return (NULL);
-	}
+	set_state(db, addr, match, match.entry);
 
-	return (state);
+	return (&geoip_state);
 }
 
 static dns_geoip_subtype_t
@@ -482,10 +389,3 @@ dns_geoip_match(const isc_netaddr_t *reqaddr,
 	 */
 	return (false);
 }
-
-void
-dns_geoip_shutdown(void) {
-	if (state_mctx != NULL) {
-		isc_mem_detach(&state_mctx);
-	}
-}

lib/dns/include/dns/geoip.h

@@ -105,9 +105,6 @@ dns_geoip_match(const isc_netaddr_t *reqaddr,
 		const dns_geoip_databases_t *geoip,
 		const dns_geoip_elem_t *elt);
 
-void
-dns_geoip_shutdown(void);
-
 ISC_LANG_ENDDECLS
 
 #endif /* HAVE_GEOIP2 */

lib/dns/tests/geoip_test.c

@@ -334,17 +334,17 @@ int
 main(void) {
 #if defined(HAVE_GEOIP2)
 	const struct CMUnitTest tests[] = {
-		cmocka_unit_test_setup_teardown(country, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(country_v6, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(city, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(city_v6, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(asnum, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(isp, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(org, _setup, _teardown),
-		cmocka_unit_test_setup_teardown(domain, _setup, _teardown),
+		cmocka_unit_test(country),
+		cmocka_unit_test(country_v6),
+		cmocka_unit_test(city),
+		cmocka_unit_test(city_v6),
+		cmocka_unit_test(asnum),
+		cmocka_unit_test(isp),
+		cmocka_unit_test(org),
+		cmocka_unit_test(domain),
 	};
 
-	return (cmocka_run_group_tests(tests, NULL, NULL));
+	return (cmocka_run_group_tests(tests, _setup, _teardown));
 #else
 	print_message("1..0 # Skip GeoIP not enabled\n");
 #endif

lib/dns/win32/libdns.def.in

@@ -384,7 +384,6 @@ dns_generalstats_dump
 dns_generalstats_increment
 @IF GEOIP
 dns_geoip_match
-dns_geoip_shutdown
 @END GEOIP
 dns_hashalg_fromtext
 dns_ipkeylist_clear