From a06cdfc7b765e5d66a1e1642882bcc1e79125b26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 1/3] Add helper function for recursive-clients logging Reduce code duplication in check_recursionquota() by extracting its parts responsible for logging to a separate helper function. Remove result text from the "no more recursive clients" message because it always says "quota reached" (as the relevant branch is only evaluated when 'result' is set to ISC_R_QUOTA) and therefore brings no additional value. --- lib/ns/query.c | 50 +++++++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index dc382c7e33..322810b66d 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6315,6 +6315,22 @@ recparam_update(ns_query_recparam_t *param, dns_rdatatype_t qtype, dns_name_copy(qdomain, param->qdomain); } } +static void +recursionquota_log(ns_client_t *client, atomic_uint_fast32_t *last_log_time, + const char *format, isc_quota_t *quota) { + isc_stdtime_t now; + + isc_stdtime_get(&now); + if (now == atomic_load_relaxed(last_log_time)) { + return; + } + + atomic_store_relaxed(last_log_time, now); + ns_client_log(client, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_QUERY, + ISC_LOG_WARNING, format, isc_quota_getused(quota), + isc_quota_getsoft(quota), isc_quota_getmax(quota)); +} + static atomic_uint_fast32_t last_soft, last_hard; /*% @@ -6327,36 +6343,16 @@ check_recursionquota(ns_client_t *client, ns_query_rectype_t recursion_type) { result = recursionquotatype_attach_soft(client, recursion_type); if (result == ISC_R_SOFTQUOTA) { - isc_stdtime_t now; - isc_stdtime_get(&now); - if (now != atomic_load_relaxed(&last_soft)) { - atomic_store_relaxed(&last_soft, now); - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_QUERY, ISC_LOG_WARNING, - "recursive-clients soft limit " - "exceeded (%u/%u/%u), " - "aborting oldest query", - isc_quota_getused(*quotap), - isc_quota_getsoft(*quotap), - isc_quota_getmax(*quotap)); - } + recursionquota_log(client, &last_soft, + "recursive-clients soft limit exceeded " + "(%u/%u/%u), aborting oldest query", + *quotap); ns_client_killoldestquery(client); result = ISC_R_SUCCESS; } else if (result == ISC_R_QUOTA) { - isc_stdtime_t now; - isc_stdtime_get(&now); - if (now != atomic_load_relaxed(&last_hard)) { - ns_server_t *sctx = client->manager->sctx; - atomic_store_relaxed(&last_hard, now); - ns_client_log(client, NS_LOGCATEGORY_CLIENT, - NS_LOGMODULE_QUERY, ISC_LOG_WARNING, - "no more recursive clients " - "(%u/%u/%u): %s", - isc_quota_getused(&sctx->recursionquota), - isc_quota_getsoft(&sctx->recursionquota), - isc_quota_getmax(&sctx->recursionquota), - isc_result_totext(result)); - } + recursionquota_log(client, &last_hard, + "no more recursive clients (%u/%u/%u)", + &client->manager->sctx->recursionquota); ns_client_killoldestquery(client); } if (result != ISC_R_SUCCESS) { From 8d64beb06feacf4bd20f9e329d6b207adc955426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 2/3] Use a switch statement in check_recursionquota() Improve readability of the check_recursionquota() function by replacing a sequence of conditional statements with a switch statement. --- lib/ns/query.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 322810b66d..53e621c4db 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6342,27 +6342,30 @@ check_recursionquota(ns_client_t *client, ns_query_rectype_t recursion_type) { isc_result_t result; result = recursionquotatype_attach_soft(client, recursion_type); - if (result == ISC_R_SOFTQUOTA) { + switch (result) { + case ISC_R_SOFTQUOTA: recursionquota_log(client, &last_soft, "recursive-clients soft limit exceeded " "(%u/%u/%u), aborting oldest query", *quotap); ns_client_killoldestquery(client); - result = ISC_R_SUCCESS; - } else if (result == ISC_R_QUOTA) { + FALLTHROUGH; + case ISC_R_SUCCESS: + break; + case ISC_R_QUOTA: recursionquota_log(client, &last_hard, "no more recursive clients (%u/%u/%u)", &client->manager->sctx->recursionquota); ns_client_killoldestquery(client); - } - if (result != ISC_R_SUCCESS) { + FALLTHROUGH; + default: return (result); } dns_message_clonebuffer(client->message); ns_client_recursing(client); - return (result); + return (ISC_R_SUCCESS); } isc_result_t From 0d7e5d513cd946a7393875f9f730fb0b54d2f1bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Tue, 14 Jun 2022 13:13:32 +0200 Subject: [PATCH 3/3] Assert on unknown isc_quota_attach() return values The only values that the isc_quota_attach() function (called from check_recursionquota() via recursionquotatype_attach_soft()) can currently return are: ISC_R_SUCCESS, ISC_R_SOFTQUOTA, and ISC_R_QUOTA. Instead of just propagating any other (unexpected) error up the call stack, assert immediately, so that if the isc_quota_* API gets updated in the future to return values currently matching the "default" statement, check_recursionquota() can be promptly updated to handle such new return values as desired. --- lib/ns/query.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ns/query.c b/lib/ns/query.c index 53e621c4db..8ce90668f5 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -6357,9 +6357,9 @@ check_recursionquota(ns_client_t *client, ns_query_rectype_t recursion_type) { "no more recursive clients (%u/%u/%u)", &client->manager->sctx->recursionquota); ns_client_killoldestquery(client); - FALLTHROUGH; - default: return (result); + default: + UNREACHABLE(); } dns_message_clonebuffer(client->message);