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.

diff --git a/bin/named/geoip.c b/bin/named/geoip.c
index f67960fed5e62571f2d0fe9b27d5eb8c9cc9f673..d1824579e1863e817d237efb1cdd6deb12115ee6 100644 (file)
--- a/bin/named/geoip.c
+++ b/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 */
 }

diff --git a/bin/named/include/named/geoip.h b/bin/named/include/named/geoip.h
index bb148e5b61dde94b1a18f0bca13003ac8b338379..0b959fa23a2220aab8dd44f1cf7a4add98f15981 100644 (file)
--- a/bin/named/include/named/geoip.h
+++ b/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);

diff --git a/bin/named/server.c b/bin/named/server.c
index 400bd15b46a2f65db67d1286555ceecee289a0ec..628ed0d28a95870e6256aae722d6b41613702268 100644 (file)
--- a/bin/named/server.c
+++ b/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 */

diff --git a/lib/dns/geoip2.c b/lib/dns/geoip2.c
index c0ab66a2415c3220506b1bbcd05cf351d53ec105..098a8844d5022adce212a3f46202cb10a1c32180 100644 (file)
--- a/lib/dns/geoip2.c
+++ b/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);
-       }
-}

diff --git a/lib/dns/include/dns/geoip.h b/lib/dns/include/dns/geoip.h
index 4dc3291c62bec8f1602b5698516d8bf128f7a77e..223a457ef9fb1df22e33b26180d4a1f24a4c4ccf 100644 (file)
--- a/lib/dns/include/dns/geoip.h
+++ b/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 */

diff --git a/lib/dns/tests/geoip_test.c b/lib/dns/tests/geoip_test.c
index 7481881a1e2413787971b2bb19ed1e02af63c84a..21a887bae2ad85296a81ae6d5940d7995088c9f2 100644 (file)
--- a/lib/dns/tests/geoip_test.c
+++ b/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

diff --git a/lib/dns/win32/libdns.def.in b/lib/dns/win32/libdns.def.in
index f90ef4111328aeba8920b580dc1a198f7b579d51..d77368debbd458198a08361abe57071386ec5004 100644 (file)
--- a/lib/dns/win32/libdns.def.in
+++ b/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