implement dns_adb_cancelfind(), rename _done() to _destroyfind(), and _lookup to _createfind()

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,
+	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);
 	}
 }

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,7 +1551,7 @@ 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,
+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)
@@ -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)
 {

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,7 +222,7 @@ 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,
+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);
@@ -234,11 +235,6 @@ dns_adb_lookup(dns_adb_t *adb, isc_task_t *task, isc_taskaction_t action,
  * be sent to the <task, taskaction, arg> 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 <host, address> 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:
  *

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,
+		result = dns_adb_createfind(res->view->adb,
 					    res->buckets[fctx->bucketnum].task,
 					    fctx_adbhandler, fctx, &name,
-					&fctx->domain, options, now, &lookup);
+					    &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)