From e317386090c6b18bf4d7c688dd2048de325aef28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 3 Aug 2021 15:27:06 +0200 Subject: [PATCH] dispatch: Remove 'timeout' callback - It is no longer necessary to pass a 'timeout' callback to dns_dispatch_addresponse(); timeouts are handled directly by the 'response' callback instead. - The netmgr handle is no longer passed to dispatch callbacks, since they don't (and can't) use it. instead, dispatch_cb_t now takes a result code, region, and argument. - Cleaned up timeout-related tests in dispatch_test.c --- bin/nsupdate/nsupdate.c | 3 +- lib/dns/dispatch.c | 80 +++++++++++++++++++++------------- lib/dns/include/dns/dispatch.h | 13 ++++-- lib/dns/request.c | 67 ++++++++++++---------------- lib/dns/resolver.c | 75 ++++++++++++++----------------- 5 files changed, 122 insertions(+), 116 deletions(-) diff --git a/bin/nsupdate/nsupdate.c b/bin/nsupdate/nsupdate.c index 9502fc8299..320e3812c0 100644 --- a/bin/nsupdate/nsupdate.c +++ b/bin/nsupdate/nsupdate.c @@ -741,8 +741,9 @@ doshutdown(void) { static void maybeshutdown(void) { /* when called from getinput, doshutdown might be already finished */ - if (requestmgr == NULL) + if (requestmgr == NULL) { return; + } ddebug("Shutting down request manager"); dns_requestmgr_shutdown(requestmgr); diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 08936b43c2..0475d67055 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -61,8 +61,6 @@ struct dns_dispatchmgr { /* locked by buffer_lock */ dns_qid_t *qid; - isc_mutex_t buffer_lock; - unsigned int buffers; in_port_t *v4ports; /*%< available ports for IPv4 */ unsigned int nv4ports; /*%< # of available ports for IPv4 */ @@ -84,10 +82,9 @@ struct dns_dispentry { isc_sockaddr_t peer; in_port_t port; dns_messageid_t id; - isc_nm_cb_t connected; - isc_nm_cb_t sent; - isc_nm_recv_cb_t response; - isc_nm_cb_t timedout; + dispatch_cb_t connected; + dispatch_cb_t sent; + dispatch_cb_t response; void *arg; bool canceled; ISC_LINK(dns_dispentry_t) link; @@ -433,7 +430,7 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, isc_sockaddr_t peer; isc_netaddr_t netaddr; int match; - isc_nm_recv_cb_t response = NULL; + dispatch_cb_t response = NULL; bool nomore = true; REQUIRE(VALID_RESPONSE(resp)); @@ -444,10 +441,8 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, LOCK(&disp->lock); if (isc_log_wouldlog(dns_lctx, LVL(90))) { - LOCK(&disp->mgr->buffer_lock); - dispatch_log(disp, LVL(90), "got packet: requests %d", + dispatch_log(disp, LVL(90), "got UDP packet: requests %d", disp->requests); - UNLOCK(&disp->mgr->buffer_lock); } if (eresult == ISC_R_CANCELED) { @@ -466,13 +461,13 @@ udp_recv(isc_nmhandle_t *handle, isc_result_t eresult, isc_region_t *region, peer = isc_nmhandle_peeraddr(handle); isc_netaddr_fromsockaddr(&netaddr, &peer); - if (eresult == ISC_R_TIMEDOUT && resp->timedout != NULL) { - resp->timedout(handle, ISC_R_TIMEDOUT, resp->arg); - if (isc_nmhandle_timer_running(handle)) { - nomore = false; - goto unlock; - } - } + /* if (eresult == ISC_R_TIMEDOUT && resp->timedout != NULL) { */ + /* resp->timedout(handle, ISC_R_TIMEDOUT, resp->arg); */ + /* if (isc_nmhandle_timer_running(handle)) { */ + /* nomore = false; */ + /* goto unlock; */ + /* } */ + /* } */ if (eresult != ISC_R_SUCCESS) { /* @@ -549,7 +544,7 @@ unlock: UNLOCK(&disp->lock); if (response != NULL) { - response(handle, eresult, region, resp->arg); + response(eresult, region, resp->arg); } if (nomore) { @@ -703,7 +698,7 @@ unlock: dispentry_detach(&resp0); if (resp != NULL) { - resp->response(handle, eresult, region, resp->arg); + resp->response(eresult, region, resp->arg); } } @@ -789,7 +784,6 @@ dns_dispatchmgr_create(isc_mem_t *mctx, isc_nm_t *nm, isc_nm_attach(nm, &mgr->nm); isc_mutex_init(&mgr->lock); - isc_mutex_init(&mgr->buffer_lock); ISC_LIST_INIT(mgr->list); @@ -868,8 +862,6 @@ dispatchmgr_destroy(dns_dispatchmgr_t *mgr) { qid_destroy(mgr->mctx, &mgr->qid); - isc_mutex_destroy(&mgr->buffer_lock); - if (mgr->blackhole != NULL) { dns_acl_detach(&mgr->blackhole); } @@ -1271,10 +1263,9 @@ dns_dispatch_detach(dns_dispatch_t **dispp) { isc_result_t dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, unsigned int timeout, const isc_sockaddr_t *dest, - isc_nm_cb_t connected, isc_nm_cb_t sent, - isc_nm_recv_cb_t response, isc_nm_cb_t timedout, - void *arg, dns_messageid_t *idp, - dns_dispentry_t **resp) { + dispatch_cb_t connected, dispatch_cb_t sent, + dispatch_cb_t response, void *arg, + dns_messageid_t *idp, dns_dispentry_t **resp) { dns_dispentry_t *res = NULL; dns_qid_t *qid = NULL; in_port_t localport = 0; @@ -1282,7 +1273,7 @@ dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, unsigned int bucket; bool ok = false; int i = 0; - isc_nm_recv_cb_t oldest_response = NULL; + dispatch_cb_t oldest_response = NULL; REQUIRE(VALID_DISPATCH(disp)); REQUIRE(dest != NULL); @@ -1323,7 +1314,6 @@ dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, .peer = *dest, .connected = connected, .sent = sent, - .timedout = timedout, .response = response, .arg = arg }; @@ -1394,7 +1384,7 @@ dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, UNLOCK(&disp->lock); if (oldest_response != NULL) { - oldest_response(res->handle, ISC_R_CANCELED, NULL, res->arg); + oldest_response(ISC_R_CANCELED, NULL, res->arg); } *idp = id; @@ -1530,7 +1520,7 @@ disp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } if (resp->connected != NULL) { - resp->connected(handle, eresult, resp->arg); + resp->connected(eresult, NULL, resp->arg); } detach: @@ -1571,7 +1561,7 @@ send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { REQUIRE(VALID_RESPONSE(resp)); - resp->sent(handle, result, resp->arg); + resp->sent(result, NULL, resp->arg); if (result != ISC_R_SUCCESS) { isc_nm_cancelread(handle); @@ -1580,6 +1570,34 @@ send_done(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { dispentry_detach(&resp); } +void +dns_dispatch_read(dns_dispentry_t *resp, uint16_t timeout) { + REQUIRE(resp != NULL); + + dns_dispatch_t *disp = resp->disp; + isc_nmhandle_t *handle = NULL; + + switch (disp->socktype) { + case isc_socktype_udp: + REQUIRE(resp->handle != NULL); + + handle = resp->handle; + + break; + case isc_socktype_tcp: + REQUIRE(disp != NULL && disp->handle == NULL); + + handle = disp->handle; + break; + default: + INSIST(0); + ISC_UNREACHABLE(); + } + + isc_nmhandle_settimeout(handle, timeout); + startrecv(disp, resp); +} + void dns_dispatch_send(dns_dispentry_t *resp, isc_region_t *r, isc_dscp_t dscp) { isc_nmhandle_t *handle = NULL; diff --git a/lib/dns/include/dns/dispatch.h b/lib/dns/include/dns/dispatch.h index e467c964e7..a4686724fa 100644 --- a/lib/dns/include/dns/dispatch.h +++ b/lib/dns/include/dns/dispatch.h @@ -279,6 +279,9 @@ dns_dispatch_send(dns_dispentry_t *resp, isc_region_t *r, isc_dscp_t dscp); *\li 'resp' is valid. */ +void +dns_dispatch_read(dns_dispentry_t *resp, uint16_t timeout); + isc_result_t dns_dispatch_gettcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *destaddr, const isc_sockaddr_t *localaddr, bool *connected, @@ -288,13 +291,15 @@ dns_dispatch_gettcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *destaddr, * if connected == NULL). */ +typedef void (*dispatch_cb_t)(isc_result_t eresult, isc_region_t *region, + void *cbarg); + isc_result_t dns_dispatch_addresponse(dns_dispatch_t *disp, unsigned int options, unsigned int timeout, const isc_sockaddr_t *dest, - isc_nm_cb_t connected, isc_nm_cb_t sent, - isc_nm_recv_cb_t response, isc_nm_cb_t timedout, - void *arg, dns_messageid_t *idp, - dns_dispentry_t **resp); + dispatch_cb_t connected, dispatch_cb_t sent, + dispatch_cb_t response, void *arg, + dns_messageid_t *idp, dns_dispentry_t **resp); /*%< * Add a response entry for this dispatch. * diff --git a/lib/dns/request.c b/lib/dns/request.c index 9e00932f79..513ffcbbad 100644 --- a/lib/dns/request.c +++ b/lib/dns/request.c @@ -108,16 +108,13 @@ static isc_result_t req_render(dns_message_t *message, isc_buffer_t **buffer, unsigned int options, isc_mem_t *mctx); static void -req_response(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, - void *arg); +req_response(isc_result_t result, isc_region_t *region, void *arg); static void -req_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg); +req_senddone(isc_result_t eresult, isc_region_t *region, void *arg); static void req_sendevent(dns_request_t *request, isc_result_t result); static void -req_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg); -static void -req_timeout(isc_nmhandle_t *handle, isc_result_t eresult, void *arg); +req_connected(isc_result_t eresult, isc_region_t *region, void *arg); static void req_attach(dns_request_t *source, dns_request_t **targetp); static void @@ -567,8 +564,8 @@ again: req_attach(request, &rclone); result = dns_dispatch_addresponse( request->dispatch, dispopt, request->timeout, destaddr, - req_connected, req_senddone, req_response, req_timeout, request, - &id, &request->dispentry); + req_connected, req_senddone, req_response, request, &id, + &request->dispentry); if (result != ISC_R_SUCCESS) { if ((options & DNS_REQUESTOPT_FIXEDID) != 0 && !newtcp) { newtcp = true; @@ -723,8 +720,7 @@ use_tcp: req_attach(request, &rclone); result = dns_dispatch_addresponse( request->dispatch, 0, request->timeout, destaddr, req_connected, - req_senddone, req_response, req_timeout, request, &id, - &request->dispentry); + req_senddone, req_response, request, &id, &request->dispentry); if (result != ISC_R_SUCCESS) { goto cleanup; } @@ -1007,10 +1003,10 @@ dns_request_destroy(dns_request_t **requestp) { *** Private: request. ***/ static void -req_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { +req_connected(isc_result_t eresult, isc_region_t *region, void *arg) { dns_request_t *request = (dns_request_t *)arg; - UNUSED(handle); + UNUSED(region); req_log(ISC_LOG_DEBUG(3), "req_connected: request %p: %s", request, isc_result_totext(eresult)); @@ -1044,13 +1040,13 @@ req_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } static void -req_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { +req_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { dns_request_t *request = (dns_request_t *)arg; REQUIRE(VALID_REQUEST(request)); REQUIRE(DNS_REQUEST_SENDING(request)); - UNUSED(handle); + UNUSED(region); req_log(ISC_LOG_DEBUG(3), "req_senddone: request %p", request); @@ -1072,42 +1068,37 @@ req_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } static void -req_timeout(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { +req_response(isc_result_t result, isc_region_t *region, void *arg) { dns_request_t *request = (dns_request_t *)arg; - REQUIRE(VALID_REQUEST(request)); - - UNUSED(eresult); - - req_log(ISC_LOG_DEBUG(3), "req_timeout: request %p", request); - - LOCK(&request->requestmgr->locks[request->hash]); - if (--request->udpcount != 0) { - isc_nmhandle_settimeout(handle, request->timeout); - if (!DNS_REQUEST_SENDING(request)) { - req_send(request); - } - } - UNLOCK(&request->requestmgr->locks[request->hash]); -} - -static void -req_response(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, - void *arg) { - dns_request_t *request = (dns_request_t *)arg; - - UNUSED(handle); - if (result == ISC_R_CANCELED) { return; } + if (result == ISC_R_TIMEDOUT) { + req_log(ISC_LOG_DEBUG(3), "req_timeout: request %p", request); + + LOCK(&request->requestmgr->locks[request->hash]); + if (--request->udpcount != 0) { + dns_dispatch_read(request->dispentry, request->timeout); + if (!DNS_REQUEST_SENDING(request)) { + req_send(request); + } + UNLOCK(&request->requestmgr->locks[request->hash]); + return; + } + + /* The lock is unlocked below */ + goto done; + } + REQUIRE(VALID_REQUEST(request)); req_log(ISC_LOG_DEBUG(3), "req_response: request %p: %s", request, dns_result_totext(result)); LOCK(&request->requestmgr->locks[request->hash]); + if (result != ISC_R_SUCCESS) { goto done; } diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 406d91118d..72387fdfcb 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -609,10 +609,9 @@ empty_bucket(dns_resolver_t *res); static isc_result_t resquery_send(resquery_t *query); static void -resquery_response(isc_nmhandle_t *handle, isc_result_t eresult, - isc_region_t *region, void *arg); +resquery_response(isc_result_t eresult, isc_region_t *region, void *arg); static void -resquery_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg); +resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg); static void fctx_try(fetchctx_t *fctx, bool retrying, bool badcache); static isc_result_t @@ -1391,11 +1390,8 @@ fctx_cancelquery(resquery_t *query, isc_time_t *finish, bool no_response, } /* - * Check for any outstanding socket events. If they exist, - * cancel them and let the event handlers finish the cleanup. - * (XXX: Currently the resolver, rather than dispatch, tracks - * whether it's sending or connecting; this will be moved into - * dispatch later.) + * Check for any outstanding dispatch responses. If they exist, + * cancel them and let their callbacks finish the cleanup. */ if (query->dispentry != NULL) { dns_dispatch_cancel(query->dispentry); @@ -1406,7 +1402,6 @@ fctx_cancelquery(resquery_t *query, isc_time_t *finish, bool no_response, ISC_LIST_UNLINK(fctx->queries, query, link); } - /* This is the final detach matching the "init" */ resquery_detach(&query); } @@ -1481,12 +1476,6 @@ fctx_cleanupaltaddrs(fetchctx_t *fctx) { } } -static inline void -fctx_stopqueries(fetchctx_t *fctx, bool no_response, bool age_untried) { - FCTXTRACE("stopqueries"); - fctx_cancelqueries(fctx, no_response, age_untried); -} - static inline void fctx_cleanupall(fetchctx_t *fctx) { fctx_cleanupfinds(fctx); @@ -1741,7 +1730,7 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) { fctx->qmin_warning = ISC_R_SUCCESS; - fctx_stopqueries(fctx, no_response, age_untried); + fctx_cancelqueries(fctx, no_response, age_untried); LOCK(&res->buckets[fctx->bucketnum].lock); @@ -1753,13 +1742,13 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) { } static void -resquery_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { +resquery_senddone(isc_result_t eresult, isc_region_t *region, void *arg) { resquery_t *query = (resquery_t *)arg; fetchctx_t *fctx = NULL; QTRACE("senddone"); - UNUSED(handle); + UNUSED(region); fctx = query->fctx; @@ -1802,7 +1791,7 @@ resquery_senddone(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } detach: - resquery_detach(&query); /* Detach dispatch query */ + resquery_detach(&query); } static inline isc_result_t @@ -1893,25 +1882,20 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) { isc_time_nowplusinterval(&fctx->next_timeout, &fctx->interval); } -static void -resquery_timeout(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { - resquery_t *query = (resquery_t *)arg; +static isc_result_t +resquery_timeout(resquery_t *query) { fetchctx_t *fctx = query->fctx; dns_fetchevent_t *event = NULL, *next = NULL; uint64_t timeleft; isc_time_t now; - REQUIRE(VALID_FCTX(fctx)); - FCTXTRACE("timeout"); /* * If not configured for serve-stale, do nothing. */ - if (eresult == ISC_R_CANCELED || - (fctx->options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) == 0) - { - return; + if ((fctx->options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) == 0) { + return (ISC_R_SUCCESS); } /* @@ -1922,7 +1906,7 @@ resquery_timeout(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { isc_time_now(&now); timeleft = isc_time_microdiff(&fctx->expires_try_stale, &now); if (timeleft >= US_PER_MSEC) { - return; + return (ISC_R_SUCCESS); } /* @@ -1950,9 +1934,12 @@ resquery_timeout(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { * resume waiting. */ timeleft = isc_time_microdiff(&fctx->next_timeout, &now); - if (timeleft < US_PER_MSEC) { - isc_nmhandle_settimeout(handle, (timeleft / US_PER_MSEC)); + if (timeleft >= US_PER_MSEC) { + dns_dispatch_read(query->dispentry, (timeleft / US_PER_MSEC)); + return (ISC_R_COMPLETE); } + + return (ISC_R_SUCCESS); } static isc_result_t @@ -2154,14 +2141,14 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, result = dns_dispatch_addresponse( query->dispatch, 0, isc_interval_ms(&fctx->interval), &query->addrinfo->sockaddr, resquery_connected, - resquery_senddone, resquery_response, resquery_timeout, query, - &query->id, &query->dispentry); + resquery_senddone, resquery_response, query, &query->id, + &query->dispentry); if (result != ISC_R_SUCCESS) { goto cleanup_dispatch; } /* Connect the socket */ - resquery_attach(query, &(resquery_t *){ NULL }); /* dispatch query */ + resquery_attach(query, &(resquery_t *){ NULL }); result = dns_dispatch_connect(query->dispentry); RUNTIME_CHECK(result == ISC_R_SUCCESS); @@ -2772,7 +2759,7 @@ cleanup_temps: } static void -resquery_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { +resquery_connected(isc_result_t eresult, isc_region_t *region, void *arg) { resquery_t *query = (resquery_t *)arg; isc_result_t result; fetchctx_t *fctx = NULL; @@ -2783,7 +2770,7 @@ resquery_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { QTRACE("connected"); - UNUSED(handle); + UNUSED(region); fctx = query->fctx; res = fctx->res; @@ -2881,7 +2868,7 @@ resquery_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } detach: - resquery_detach(&query); /* Detach dispatch query */ + resquery_detach(&query); } static void @@ -4405,7 +4392,7 @@ fctx_doshutdown(isc_task_t *task, isc_event_t *event) { * fetch, and clean up finds and addresses. To avoid deadlock * with the ADB, we must do this before we lock the bucket lock. */ - fctx_stopqueries(fctx, false, false); + fctx_cancelqueries(fctx, false, false); fctx_cleanupall(fctx); LOCK(&res->buckets[bucketnum].lock); @@ -7206,15 +7193,12 @@ betterreferral(respctx_t *rctx) { * resquery_send(). Sets up a response context (respctx_t). */ static void -resquery_response(isc_nmhandle_t *handle, isc_result_t eresult, - isc_region_t *region, void *arg) { +resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { isc_result_t result; resquery_t *query = (resquery_t *)arg; fetchctx_t *fctx = NULL; respctx_t rctx; - UNUSED(handle); - if (eresult == ISC_R_CANCELED) { return; } @@ -7225,6 +7209,13 @@ resquery_response(isc_nmhandle_t *handle, isc_result_t eresult, QTRACE("response"); + if (eresult == ISC_R_TIMEDOUT) { + result = resquery_timeout(query); + if (result == ISC_R_COMPLETE) { + return; + } + } + if (isc_sockaddr_pf(&query->addrinfo->sockaddr) == PF_INET) { inc_stats(fctx->res, dns_resstatscounter_responsev4); } else {