From 897c9ddb4d745b2bfecf98b17e5487bb6656299a Mon Sep 17 00:00:00 2001 From: Michael Graff Date: Fri, 29 Oct 1999 18:00:31 +0000 Subject: [PATCH] implement dns_adb_cancelfind(), rename _done() to _destroyfind(), and _lookup to _createfind() --- bin/tests/adb_test.c | 24 ++++--- lib/dns/adb.c | 130 +++++++++++++++++++++++++++++++------- lib/dns/include/dns/adb.h | 55 +++++++++++----- lib/dns/resolver.c | 13 ++-- 4 files changed, 163 insertions(+), 59 deletions(-) diff --git a/bin/tests/adb_test.c b/bin/tests/adb_test.c index 2f901527da..3167bbb171 100644 --- a/bin/tests/adb_test.c +++ b/bin/tests/adb_test.c @@ -212,26 +212,24 @@ static void lookup_callback(isc_task_t *task, isc_event_t *ev) { client_t *client; - dns_adbhandle_t *handle; client = ev->arg; - handle = ev->sender; + INSIST(client->handle == ev->sender); printf("Task %p got event %p type %08x from %p, client %p\n", - task, ev, ev->type, handle, client); + task, ev, ev->type, client->handle, client); + + isc_event_free(&ev); CLOCK(); dns_adb_dumphandle(client->handle, stderr); - dns_adb_done(&client->handle); - handle = NULL; + dns_adb_destroyfind(&client->handle); ISC_LIST_UNLINK(clients, client, link); free_client(&client); CUNLOCK(); - - isc_event_free(&ev); } void @@ -346,17 +344,17 @@ lookup(char *target) result = dns_name_dup(&name, mctx, &client->name); check_result(result, "dns_name_dup %s", target); - result = dns_adb_lookup(adb, t2, lookup_callback, client, - &client->name, dns_rootname, - (DNS_ADBFIND_INET | DNS_ADBFIND_WANTEVENT), - now, &client->handle); + result = dns_adb_createfind(adb, t2, lookup_callback, client, + &client->name, dns_rootname, + (DNS_ADBFIND_INET | DNS_ADBFIND_WANTEVENT), + now, &client->handle); check_result(result, "dns_adb_lookup()"); dns_adb_dumphandle(client->handle, stderr); - if (client->handle->query_pending) + if ((client->handle->options & DNS_ADBFIND_WANTEVENT) != 0) ISC_LIST_APPEND(clients, client, link); else { - dns_adb_done(&client->handle); + dns_adb_destroyfind(&client->handle); free_client(&client); } } diff --git a/lib/dns/adb.c b/lib/dns/adb.c index e7a4ffb681..5bfbd0ed41 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -266,6 +266,12 @@ static inline void link_name(dns_adb_t *, int, dns_adbname_t *); static inline void unlink_name(dns_adb_t *, dns_adbname_t *); static void kill_name(dns_adbname_t **, isc_eventtype_t ev); +#define HANDLE_EVENT_SENT 0x00000001 +#define HANDLE_EVENT_FREED 0x00000002 + +#define EVENT_SENT(h) (((h)->flags & HANDLE_EVENT_SENT) != 0) +#define EVENT_FREED(h) (((h)->flags & HANDLE_EVENT_FREED) != 0) + #define WANTEVENT(x) (((x) & DNS_ADBFIND_WANTEVENT) != 0) #define WANTEMPTYEVENT(x) (((x) & DNS_ADBFIND_EMPTYEVENT) != 0) #define HAVE_INET(n) (!ISC_LIST_EMPTY((n)->v4)) @@ -581,6 +587,24 @@ clean_namehooks(dns_adb_t *adb, dns_adbnamehooklist_t *namehooks) UNLOCK(&adb->entrylocks[addr_bucket]); } +/* + * Assumes nothing is locked, since this is called by the client. + */ +static void +event_free(isc_event_t *event) +{ + dns_adbhandle_t *handle; + + INSIST(event != NULL); + handle = event->destroy_arg; + INSIST(DNS_ADBHANDLE_VALID(handle)); + + LOCK(&handle->lock); + handle->flags |= HANDLE_EVENT_FREED; + event->destroy_arg = NULL; + UNLOCK(&handle->lock); +} + /* * Assumes the name bucket is locked. */ @@ -597,16 +621,20 @@ clean_handles_at_name(dns_adbname_t *name, isc_eventtype_t evtype) /* * Unlink the handle from the name, letting the caller - * call dns_adb_done() on it to clean it up later. + * call dns_adb_destroyfind() on it to clean it up later. */ ISC_LIST_UNLINK(name->handles, handle, plink); handle->adbname = NULL; handle->name_bucket = DNS_ADB_INVALIDBUCKET; + INSIST(!EVENT_SENT(handle)); + ev = &handle->event; task = ev->sender; ev->sender = handle; ev->type = evtype; + ev->destroy = event_free; + ev->destroy_arg = handle; DP(("Sending event %p to task %p for handle %p\n", ev, task, handle)); @@ -924,8 +952,9 @@ new_adbhandle(dns_adb_t *adb) ISC_LINK_INIT(h, publink); ISC_LINK_INIT(h, plink); ISC_LIST_INIT(h->list); - h->name_bucket = DNS_ADB_INVALIDBUCKET; h->adbname = NULL; + h->name_bucket = DNS_ADB_INVALIDBUCKET; + h->flags = 0; /* * private members @@ -1522,10 +1551,10 @@ dns_adb_detach(dns_adb_t **adbx) } isc_result_t -dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, - void *arg, dns_name_t *name, dns_name_t *zone, - unsigned int options, isc_stdtime_t now, - dns_adbhandle_t **handlep) +dns_adb_createfind(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, + void *arg, dns_name_t *name, dns_name_t *zone, + unsigned int options, isc_stdtime_t now, + dns_adbhandle_t **handlep) { dns_adbhandle_t *handle; dns_adbname_t *adbname; @@ -1542,7 +1571,7 @@ dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, REQUIRE(zone != NULL); REQUIRE(handlep != NULL && *handlep == NULL); - if ((options & DNS_ADBFIND_WANTEVENT) != 0) { + if (WANTEVENT(options)) { REQUIRE(task != NULL); } @@ -1665,7 +1694,14 @@ dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, handle->query_pending = (adbname->query_pending & wanted_addresses); } else { + /* + * Remove the flag so the caller knows there will never + * be an event, and set internal flags to fake that + * the event was sent and freed, so dns_adb_destroyfind() will + * do the right thing. + */ handle->options &= ~DNS_ADBFIND_WANTEVENT; + handle->flags |= (HANDLE_EVENT_SENT | HANDLE_EVENT_FREED); } handle->partial_result |= (adbname->partial_result & wanted_addresses); @@ -1865,7 +1901,7 @@ dns_adb_insert(dns_adb_t *adb, dns_name_t *host, isc_sockaddr_t *addr, } void -dns_adb_done(dns_adbhandle_t **handlep) +dns_adb_destroyfind(dns_adbhandle_t **handlep) { dns_adbhandle_t *handle; dns_adbentry_t *entry; @@ -1884,24 +1920,11 @@ dns_adb_done(dns_adbhandle_t **handlep) adb = handle->adb; REQUIRE(DNS_ADB_VALID(adb)); - bucket = handle->name_bucket; - if (bucket == DNS_ADB_INVALIDBUCKET) - goto cleanup; + REQUIRE(EVENT_FREED(handle)); - /* - * We need to get the adbname's lock to unlink the handle. - */ - violate_locking_hierarchy(&handle->lock, &adb->namelocks[bucket]); bucket = handle->name_bucket; - if (bucket != DNS_ADB_INVALIDBUCKET) { - ISC_LIST_UNLINK(handle->adbname->handles, handle, plink); - handle->adbname = NULL; - handle->name_bucket = DNS_ADB_INVALIDBUCKET; - } - UNLOCK(&adb->namelocks[bucket]); - bucket = DNS_ADB_INVALIDBUCKET; + INSIST(bucket == DNS_ADB_INVALIDBUCKET); - cleanup: UNLOCK(&handle->lock); /* @@ -1921,12 +1944,73 @@ dns_adb_done(dns_adbhandle_t **handlep) ai = ISC_LIST_HEAD(handle->list); } + /* + * WARNING: The handle is freed with the adb locked. This is done + * to avoid a race condition where we free the handle, some other + * thread tests to see if it should be destroyed, detects it should + * be, destroys it, and then we try to lock it for our check, but the + * lock is destroyed. + */ LOCK(&adb->lock); free_adbhandle(adb, &handle); check_exit(adb); UNLOCK(&adb->lock); } +void +dns_adb_cancelfind(dns_adbhandle_t *handle) +{ + isc_event_t *ev; + isc_task_t *task; + dns_adb_t *adb; + int bucket; + + LOCK(&handle->lock); + + DP(("dns_adb_cancelfind on handle %p\n", handle)); + + adb = handle->adb; + REQUIRE(DNS_ADB_VALID(adb)); + + REQUIRE(!EVENT_FREED(handle)); + REQUIRE(WANTEVENT(handle->options)); + + bucket = handle->name_bucket; + if (bucket == DNS_ADB_INVALIDBUCKET) + goto cleanup; + + /* + * We need to get the adbname's lock to unlink the handle. + */ + violate_locking_hierarchy(&handle->lock, &adb->namelocks[bucket]); + bucket = handle->name_bucket; + if (bucket != DNS_ADB_INVALIDBUCKET) { + ISC_LIST_UNLINK(handle->adbname->handles, handle, plink); + handle->adbname = NULL; + handle->name_bucket = DNS_ADB_INVALIDBUCKET; + } + UNLOCK(&adb->namelocks[bucket]); + bucket = DNS_ADB_INVALIDBUCKET; + + cleanup: + + if (!EVENT_SENT(handle)) { + ev = &handle->event; + task = ev->sender; + ev->sender = handle; + ev->type = DNS_EVENT_ADBCANCELED; + ev->destroy = event_free; + ev->destroy_arg = handle; + + DP(("Sending event %p to task %p for handle %p\n", + ev, task, handle)); + + isc_task_sendanddetach(&task, &ev); + } + + UNLOCK(&handle->lock); +} + void dns_adb_dump(dns_adb_t *adb, FILE *f) { diff --git a/lib/dns/include/dns/adb.h b/lib/dns/include/dns/adb.h index f0b4496a2b..e2e8d45a50 100644 --- a/lib/dns/include/dns/adb.h +++ b/lib/dns/include/dns/adb.h @@ -127,6 +127,7 @@ struct dns_adbhandle { /* Private */ isc_mutex_t lock; /* locks all below */ int name_bucket; + unsigned int flags; dns_adbname_t *adbname; dns_adb_t *adb; isc_event_t event; @@ -175,7 +176,7 @@ struct dns_adbaddrinfo { * was canceled. * * In each of these cases, the addresses returned by the initial call - * to dns_adb_lookup() can still be used until they are no longer needed. + * to dns_adb_createfind() can still be used until they are no longer needed. */ /**** @@ -221,10 +222,10 @@ dns_adb_detach(dns_adb_t **adb); isc_result_t -dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, - void *arg, dns_name_t *name, dns_name_t *zone, - unsigned int families, isc_stdtime_t now, - dns_adbhandle_t **handle); +dns_adb_createfind(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, + void *arg, dns_name_t *name, dns_name_t *zone, + unsigned int families, isc_stdtime_t now, + dns_adbhandle_t **handle); /* * Main interface for clients. The adb will look up the name given in * "name" and will build up a list of found addresses, and perhaps start @@ -234,11 +235,6 @@ dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action, * be sent to the with the sender of that event * set to a pointer to the dns_adbhandle_t returned by this function. * - * The events must be canceled using either dns_adb_cancel() or - * dns_adb_done(). dns_adb_cancel() will cause no more events to be posted - * to the task, but the handle is not destroyed. dns_adb_done() will - * stop events as well, and will also destroy the handle. - * * The list of addresses returned is unordered. The caller must impose * any ordering required. The list will not contain "known bad" addresses, * however. For instance, it will not return hosts that are known to be @@ -330,15 +326,39 @@ dns_adb_insert(dns_adb_t *adb, dns_name_t *host, isc_sockaddr_t *addr, * ISC_R_EXISTS -- the tuple exists already. */ - void -dns_adb_done(dns_adbhandle_t **handle); +dns_adb_cancelfind(dns_adbhandle_t *handle); /* - * Stops any internal lookups for this handle. + * Cancels the find, and sends the event off to the caller. + * + * It is an error to call dns_adb_cancelfind() on a handle where + * no event is wanted, or will ever be sent. * * Requires: * - * 'adb' be a valid dns_adb_t pointer. + * 'handle' be a valid dns_adbhandle_t pointer. + * + * events would have been posted to the task. This can be checked + * with (handle->options & DNS_ADBFIND_WANTEVENT). + * + * Ensures: + * + * The event was posted to the task. + * + * Note: + * + * It is possible that the real completion event was posted just + * before the dns_adb_cancelfind() call was made. In this case, + * dns_adb_cancelfind() will do nothing. The event handler needs + * to be prepared to handle this situation. + */ + +void +dns_adb_destroyfind(dns_adbhandle_t **handle); +/* + * Destroys the handle reference. + * + * Requires: * * 'handle' != NULL and *handle be valid dns_adbhandle_t pointer. * @@ -349,8 +369,9 @@ dns_adb_done(dns_adbhandle_t **handle); * * Note: * - * The task used to launch this handle can be used internally for - * a short time after this function returns. + * This can only be called after the event was delivered for a + * handle. Additionally, the event MUST have been freed via + * isc_event_free() BEFORE this function is called. */ void @@ -394,7 +415,7 @@ dns_adb_marklame(dns_adb_t *adb, dns_adbaddrinfo_t *addr, dns_name_t *zone, * * addr be valid. * - * zone be the zone used in the dns_adb_lookup() call. + * zone be the zone used in the dns_adb_createfind() call. * * Returns: * diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index c7146143ef..f59f155254 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -282,7 +282,7 @@ fctx_freelookups(fetchctx_t *fctx) { lookup = next_lookup) { next_lookup = ISC_LIST_NEXT(lookup, publink); ISC_LIST_UNLINK(fctx->lookups, lookup, publink); - dns_adb_done(&lookup); + dns_adb_destroyfind(&lookup); } fctx->lookup = NULL; } @@ -604,10 +604,11 @@ fctx_getaddresses(fetchctx_t *fctx) { * See what we know about this address. */ lookup = NULL; - result = dns_adb_lookup(res->view->adb, - res->buckets[fctx->bucketnum].task, - fctx_adbhandler, fctx, &name, - &fctx->domain, options, now, &lookup); + result = dns_adb_createfind(res->view->adb, + res->buckets[fctx->bucketnum].task, + fctx_adbhandler, fctx, &name, + &fctx->domain, options, now, + &lookup); if (result != ISC_R_SUCCESS) return (result); if (!ISC_LIST_EMPTY(lookup->list)) { @@ -629,7 +630,7 @@ fctx_getaddresses(fetchctx_t *fctx) { * We're not fetching them either. We lose * for this name. */ - dns_adb_done(&lookup); + dns_adb_destroyfind(&lookup); } } if (lookup != NULL)