From 8602beecd11757e0607928895e596531ebd461a8 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Wed, 3 Jan 2024 16:24:53 +0100 Subject: [PATCH] Replace keystore attach/detach with ISC_REFCOUNT_IMPL/ISC_REFCOUNT_DECL This is now the default way to implement attaching to/detaching from a pointer. Also update cfg_keystore_fromconfig() to allow NULL value for the keystore pointer. In most cases we detach it immediately after the function call. --- bin/dnssec/dnssec-keygen.c | 4 +-- bin/named/server.c | 10 ++---- lib/dns/include/dns/keystore.h | 50 ++++++++++------------------ lib/dns/keystore.c | 27 ++++----------- lib/isccfg/check.c | 23 +++---------- lib/isccfg/include/isccfg/kaspconf.h | 2 +- lib/isccfg/kaspconf.c | 7 ++-- 7 files changed, 36 insertions(+), 87 deletions(-) diff --git a/bin/dnssec/dnssec-keygen.c b/bin/dnssec/dnssec-keygen.c index b9e5fdb62b..7876f3f731 100644 --- a/bin/dnssec/dnssec-keygen.c +++ b/bin/dnssec/dnssec-keygen.c @@ -276,14 +276,12 @@ kasp_from_conf(cfg_obj_t *config, isc_mem_t *mctx, const char *name, cfg_obj_t *kconfig = cfg_listelt_value(element); ks = NULL; result = cfg_keystore_fromconfig(kconfig, mctx, lctx, engine, - &kslist, &ks); + &kslist, NULL); if (result != ISC_R_SUCCESS) { fatal("failed to configure key-store '%s': %s", cfg_obj_asstring(cfg_tuple_get(kconfig, "name")), isc_result_totext(result)); } - INSIST(ks != NULL); - dns_keystore_detach(&ks); } /* Default key-directory key store. */ ks = NULL; diff --git a/bin/named/server.c b/bin/named/server.c index 64fbd5dc99..d2da295f06 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -8895,15 +8895,11 @@ load_configuration(const char *filename, named_server_t *server, /* * Create the built-in key store ("key-directory"). */ - keystore = NULL; result = cfg_keystore_fromconfig(NULL, named_g_mctx, named_g_lctx, - named_g_engine, &keystorelist, - &keystore); + named_g_engine, &keystorelist, NULL); if (result != ISC_R_SUCCESS) { goto cleanup_keystorelist; } - INSIST(keystore != NULL); - dns_keystore_detach(&keystore); /* * Create the DNSSEC key stores. @@ -8917,12 +8913,10 @@ load_configuration(const char *filename, named_server_t *server, keystore = NULL; result = cfg_keystore_fromconfig(kconfig, named_g_mctx, named_g_lctx, named_g_engine, - &keystorelist, &keystore); + &keystorelist, NULL); if (result != ISC_R_SUCCESS) { goto cleanup_keystorelist; } - INSIST(keystore != NULL); - dns_keystore_detach(&keystore); } /* diff --git a/lib/dns/include/dns/keystore.h b/lib/dns/include/dns/keystore.h index 8cf573cb31..8888eb12d4 100644 --- a/lib/dns/include/dns/keystore.h +++ b/lib/dns/include/dns/keystore.h @@ -24,6 +24,8 @@ * A key store defines where to store DNSSEC keys. */ +/* Add -DDNS_KEYSTORE_TRACE=1 to CFLAGS for detailed reference tracing */ + #include #include #include @@ -85,38 +87,6 @@ dns_keystore_create(isc_mem_t *mctx, const char *name, const char *engine, *\li Other errors are possible. */ -void -dns_keystore_attach(dns_keystore_t *source, dns_keystore_t **targetp); -/*%< - * Attach '*targetp' to 'source'. - * - * Requires: - * - *\li 'source' is a valid keystore. - * - *\li 'targetp' points to a NULL dns_keystore_t *. - * - * Ensures: - * - *\li *targetp is attached to source. - * - *\li While *targetp is attached, the keystore will not shut down. - */ - -void -dns_keystore_detach(dns_keystore_t **kspp); -/*%< - * Detach keystore. - * - * Requires: - * - *\li 'kspp' points to a valid dns_keystore_t * - * - * Ensures: - * - *\li *kspp is NULL. - */ - const char * dns_keystore_name(dns_keystore_t *keystore); /*%< @@ -231,4 +201,20 @@ dns_keystorelist_find(dns_keystorelist_t *list, const char *name, *\li #ISC_R_NOTFOUND No matching keystore was found. */ +#ifdef DNS_KEYSTORE_TRACE +/* Compatibility macros */ +#define dns_keystore_attach(ks, ksp) \ + dns_keystore__attach(ks, ksp, __func__, __FILE__, __LINE__) +#define dns_keystore_detach(ksp) \ + dns_keystore__detach(ksp, __func__, __FILE__, __LINE__) +#define dns_keystore_ref(ptr) \ + dns_keystore__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_keystore_unref(ptr) \ + dns_keystore__unref(ptr, __func__, __FILE__, __LINE__) + +ISC_REFCOUNT_TRACE_DECL(dns_keystore); +#else +ISC_REFCOUNT_DECL(dns_keystore); +#endif /* DNS_KEYSTORE_TRACE */ + ISC_LANG_ENDDECLS diff --git a/lib/dns/keystore.c b/lib/dns/keystore.c index 128bededc0..90b766d62e 100644 --- a/lib/dns/keystore.c +++ b/lib/dns/keystore.c @@ -54,17 +54,8 @@ dns_keystore_create(isc_mem_t *mctx, const char *name, const char *engine, return (ISC_R_SUCCESS); } -void -dns_keystore_attach(dns_keystore_t *source, dns_keystore_t **targetp) { - REQUIRE(DNS_KEYSTORE_VALID(source)); - REQUIRE(targetp != NULL && *targetp == NULL); - - isc_refcount_increment(&source->references); - *targetp = source; -} - static inline void -destroy(dns_keystore_t *keystore) { +dns__keystore_destroy(dns_keystore_t *keystore) { char *name; REQUIRE(!ISC_LINK_LINKED(keystore, link)); @@ -81,17 +72,11 @@ destroy(dns_keystore_t *keystore) { isc_mem_putanddetach(&keystore->mctx, keystore, sizeof(*keystore)); } -void -dns_keystore_detach(dns_keystore_t **kspp) { - REQUIRE(kspp != NULL && DNS_KEYSTORE_VALID(*kspp)); - - dns_keystore_t *ks = *kspp; - *kspp = NULL; - - if (isc_refcount_decrement(&ks->references) == 1) { - destroy(ks); - } -} +#ifdef DNS_KEYSTORE_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_keystore, dns__keystore_destroy); +#else +ISC_REFCOUNT_IMPL(dns_keystore, dns__keystore_destroy); +#endif const char * dns_keystore_name(dns_keystore_t *keystore) { diff --git a/lib/isccfg/check.c b/lib/isccfg/check.c index ad4ae8441d..c4ddf1940d 100644 --- a/lib/isccfg/check.c +++ b/lib/isccfg/check.c @@ -1414,15 +1414,12 @@ check_options(const cfg_obj_t *options, const cfg_obj_t *config, ret = cfg_keystore_fromconfig(kconfig, mctx, logctx, NULL, - &kslist, &ks); + &kslist, NULL); if (ret != ISC_R_SUCCESS) { if (result == ISC_R_SUCCESS) { result = ret; } } - if (ks != NULL) { - dns_keystore_detach(&ks); - } } } } @@ -1431,15 +1428,12 @@ check_options(const cfg_obj_t *options, const cfg_obj_t *config, * Add default key-store "key-directory". */ tresult = cfg_keystore_fromconfig(NULL, mctx, logctx, NULL, &kslist, - &ks); + NULL); if (tresult != ISC_R_SUCCESS) { if (result == ISC_R_SUCCESS) { result = tresult; } } - if (ks != NULL) { - dns_keystore_detach(&ks); - } /* * Check dnssec-policy. @@ -2962,16 +2956,10 @@ check_keydir(const cfg_obj_t *config, const cfg_obj_t *zconfig, element = cfg_list_next(element)) { cfg_obj_t *kcfg = cfg_listelt_value(element); - ks = NULL; (void)cfg_keystore_fromconfig(kcfg, mctx, logctx, NULL, &kslist, - &ks); - INSIST(ks != NULL); - dns_keystore_detach(&ks); + NULL); } - ks = NULL; - (void)cfg_keystore_fromconfig(NULL, mctx, logctx, NULL, &kslist, &ks); - INSIST(ks != NULL); - dns_keystore_detach(&ks); + (void)cfg_keystore_fromconfig(NULL, mctx, logctx, NULL, &kslist, NULL); /* * Look for the dnssec-policy by name, which is the dnssec-policy @@ -3037,9 +3025,6 @@ check: if (kasp != NULL) { dns_kasp_detach(&kasp); } - if (ks != NULL) { - dns_keystore_detach(&ks); - } for (kasp = ISC_LIST_HEAD(kasplist); kasp != NULL; kasp = kasp_next) { diff --git a/lib/isccfg/include/isccfg/kaspconf.h b/lib/isccfg/include/isccfg/kaspconf.h index 1bc727d89c..ccc1cecc5d 100644 --- a/lib/isccfg/include/isccfg/kaspconf.h +++ b/lib/isccfg/include/isccfg/kaspconf.h @@ -78,7 +78,7 @@ cfg_keystore_fromconfig(const cfg_obj_t *config, isc_mem_t *mctx, * *\li 'logctx' is a valid logging context. * - *\li kspp != NULL && *kspp == NULL + *\li kspp == NULL || *kspp == NULL * * Returns: * diff --git a/lib/isccfg/kaspconf.c b/lib/isccfg/kaspconf.c index 583f482f10..f756ed97da 100644 --- a/lib/isccfg/kaspconf.c +++ b/lib/isccfg/kaspconf.c @@ -737,8 +737,6 @@ cfg_keystore_fromconfig(const cfg_obj_t *config, isc_mem_t *mctx, dns_keystore_t *keystore = NULL; int i = 0; - REQUIRE(kspp != NULL && *kspp == NULL); - if (config != NULL) { name = cfg_obj_asstring(cfg_tuple_get(config, "name")); } else { @@ -789,7 +787,10 @@ cfg_keystore_fromconfig(const cfg_obj_t *config, isc_mem_t *mctx, INSIST(!(ISC_LIST_EMPTY(*keystorelist))); /* Success: Attach the keystore to the pointer and return. */ - dns_keystore_attach(keystore, kspp); + if (kspp != NULL) { + INSIST(*kspp == NULL); + dns_keystore_attach(keystore, kspp); + } /* Don't detach as keystore is on '*keystorelist' */ return (ISC_R_SUCCESS);