Creating TCP dispatch now creates/binds the socket

Previously, creation of TCP dispatches differed from UDP in that a TCP
dispatch was created to attach to an existing socket, whereas a UDP
dispatch would be created in a vacuum and sockets would be opened on
demand when a transaction was initiated.

We are moving as much socket code as possible into the dispatch module,
so that it can be replaced with a netmgr version as easily as
possible. (This will also have the side effect of making TCP and UDP
dispatches more similar.)

As a step in that direction, this commit changes
dns_dispatch_createtcp() so that it creates the TCP socket.

lib/dns/dispatch.c

@@ -631,6 +631,7 @@ get_dispsocket(dns_dispatch_t *disp, const isc_sockaddr_t *dest,
 			continue;
 		}
 		UNLOCK(&qid->lock);
+
 		result = open_socket(sockmgr, &localaddr,
 				     ISC_SOCKET_REUSEADDRESS, &sock);
 		if (result == ISC_R_SUCCESS) {
@@ -1652,8 +1653,9 @@ qid_destroy(isc_mem_t *mctx, dns_qid_t **qidp) {
 /*
  * Allocate and set important limits.
  */
-static isc_result_t
+static void
-dispatch_allocate(dns_dispatchmgr_t *mgr, dns_dispatch_t **dispp) {
+dispatch_allocate(dns_dispatchmgr_t *mgr, isc_sockettype_t type, int pf,
+		  unsigned int attributes, dns_dispatch_t **dispp) {
 	dns_dispatch_t *disp = NULL;
 
 	REQUIRE(VALID_DISPATCHMGR(mgr));
@@ -1668,18 +1670,50 @@ dispatch_allocate(dns_dispatchmgr_t *mgr, dns_dispatch_t **dispp) {
 	isc_refcount_increment0(&mgr->irefs);
 
 	*disp = (dns_dispatch_t){ .mgr = mgr,
+				  .socktype = type,
 				  .shutdown_why = ISC_R_UNEXPECTED };
 	isc_refcount_init(&disp->refcount, 1);
 	ISC_LINK_INIT(disp, link);
 	ISC_LIST_INIT(disp->activesockets);
 	ISC_LIST_INIT(disp->inactivesockets);
 
+	switch (type) {
+	case isc_sockettype_tcp:
+		disp->attributes |= DNS_DISPATCHATTR_TCP;
+		break;
+	case isc_sockettype_udp:
+		disp->attributes |= DNS_DISPATCHATTR_UDP;
+		break;
+	default:
+		INSIST(0);
+		ISC_UNREACHABLE();
+	}
+
+	switch (pf) {
+	case PF_INET:
+		disp->attributes |= DNS_DISPATCHATTR_IPV4;
+		break;
+	case PF_INET6:
+		disp->attributes |= DNS_DISPATCHATTR_IPV6;
+		break;
+	default:
+		INSIST(0);
+		ISC_UNREACHABLE();
+	}
+
+	/*
+	 * Set whatever attributes were passed in that haven't been
+	 * reset automatically by the code above.
+	 */
+	attributes &= ~(DNS_DISPATCHATTR_UDP | DNS_DISPATCHATTR_TCP |
+			DNS_DISPATCHATTR_IPV4 | DNS_DISPATCHATTR_IPV6);
+	disp->attributes |= attributes;
+
 	isc_mutex_init(&disp->lock);
 	disp->failsafe_ev = allocate_devent(disp);
 	disp->magic = DISPATCH_MAGIC;
 
 	*dispp = disp;
-	return (ISC_R_SUCCESS);
 }
 
 /*
@@ -1720,30 +1754,38 @@ dispatch_free(dns_dispatch_t **dispp) {
 }
 
 isc_result_t
-dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socket_t *sock,
+dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
 		       isc_taskmgr_t *taskmgr, const isc_sockaddr_t *localaddr,
 		       const isc_sockaddr_t *destaddr, unsigned int attributes,
-		       dns_dispatch_t **dispp) {
+		       isc_dscp_t dscp, dns_dispatch_t **dispp) {
 	isc_result_t result;
 	dns_dispatch_t *disp = NULL;
+	isc_sockaddr_t src;
 	int pf;
 
 	REQUIRE(VALID_DISPATCHMGR(mgr));
-	REQUIRE(isc_socket_gettype(sock) == isc_sockettype_tcp);
+	REQUIRE(sockmgr != NULL);
-
+	REQUIRE(destaddr != NULL);
-	attributes |= DNS_DISPATCHATTR_TCP;
-	attributes &= ~DNS_DISPATCHATTR_UDP;
 
 	LOCK(&mgr->lock);
 
-	result = dispatch_allocate(mgr, &disp);
+	pf = isc_sockaddr_pf(destaddr);
-	if (result != ISC_R_SUCCESS) {
+	dispatch_allocate(mgr, isc_sockettype_tcp, pf, attributes, &disp);
-		UNLOCK(&mgr->lock);
-		return (result);
-	}
 
-	disp->socktype = isc_sockettype_tcp;
+	disp->peer = *destaddr;
-	isc_socket_attach(sock, &disp->socket);
+
+	if (localaddr != NULL) {
+		disp->local = *localaddr;
+	} else {
+		switch (pf) {
+		case AF_INET:
+			isc_sockaddr_any(&disp->local);
+			break;
+		case AF_INET6:
+			isc_sockaddr_any6(&disp->local);
+			break;
+		}
+	}
 
 	disp->ntasks = 1;
 	disp->task[0] = NULL;
@@ -1752,6 +1794,26 @@ dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socket_t *sock,
 		goto cleanup;
 	}
 
+	result = isc_socket_create(sockmgr, isc_sockaddr_pf(destaddr),
+				   isc_sockettype_tcp, &disp->socket);
+	if (result != ISC_R_SUCCESS) {
+		goto cleanup;
+	}
+
+	if (localaddr == NULL) {
+		isc_sockaddr_anyofpf(&src, pf);
+	} else {
+		src = *localaddr;
+		isc_sockaddr_setport(&src, 0);
+	}
+
+	result = isc_socket_bind(disp->socket, &src, 0);
+	if (result != ISC_R_SUCCESS) {
+		goto cleanup;
+	}
+
+	isc_socket_dscp(disp->socket, dscp);
+
 	disp->ctlevent =
 		isc_event_allocate(mgr->mctx, disp, DNS_EVENT_DISPATCHCONTROL,
 				   destroy_disp, disp, sizeof(isc_event_t));
@@ -1761,48 +1823,6 @@ dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socket_t *sock,
 	dns_tcpmsg_init(mgr->mctx, disp->socket, &disp->tcpmsg);
 	disp->tcpmsg_valid = 1;
 
-	if (destaddr == NULL) {
-		(void)isc_socket_getpeername(sock, &disp->peer);
-		attributes |= DNS_DISPATCHATTR_PRIVATE;
-	} else {
-		disp->peer = *destaddr;
-	}
-
-	pf = isc_sockaddr_pf(&disp->peer);
-
-	if (localaddr == NULL) {
-		if (destaddr != NULL) {
-			switch (pf) {
-			case AF_INET:
-				isc_sockaddr_any(&disp->local);
-				break;
-			case AF_INET6:
-				isc_sockaddr_any6(&disp->local);
-				break;
-			}
-		} else {
-			(void)isc_socket_getsockname(sock, &disp->local);
-		}
-	} else {
-		disp->local = *localaddr;
-	}
-
-	switch (pf) {
-	case PF_INET:
-		attributes |= DNS_DISPATCHATTR_IPV4;
-		break;
-
-	case PF_INET6:
-		attributes |= DNS_DISPATCHATTR_IPV6;
-		break;
-
-	default:
-		result = ISC_R_NOTIMPLEMENTED;
-		goto cleanup;
-	}
-
-	disp->attributes = attributes;
-
 	/*
 	 * Append it to the dispatcher list.
 	 */
@@ -1908,6 +1928,7 @@ dns_dispatch_gettcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *destaddr,
 		disp = ISC_LIST_NEXT(disp, link);
 	}
 	UNLOCK(&mgr->lock);
+
 	return (match ? ISC_R_SUCCESS : ISC_R_NOTFOUND);
 }
 
@@ -1924,43 +1945,29 @@ dns_dispatch_createudp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
 	REQUIRE(taskmgr != NULL);
 	REQUIRE(dispp != NULL && *dispp == NULL);
 
-	attributes |= DNS_DISPATCHATTR_UDP;
-	attributes &= ~DNS_DISPATCHATTR_TCP;
-
 	LOCK(&mgr->lock);
 	result = dispatch_createudp(mgr, sockmgr, taskmgr, localaddr,
 				    attributes, &disp);
-
 	if (result == ISC_R_SUCCESS) {
 		*dispp = disp;
 	}
-
 	UNLOCK(&mgr->lock);
-	return (ISC_R_SUCCESS);
+
+	return (result);
 }
 
 static isc_result_t
 dispatch_createudp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
 		   isc_taskmgr_t *taskmgr, const isc_sockaddr_t *localaddr,
 		   unsigned int attributes, dns_dispatch_t **dispp) {
-	isc_result_t result;
+	isc_result_t result = ISC_R_SUCCESS;
 	dns_dispatch_t *disp = NULL;
 	isc_socket_t *sock = NULL;
 	isc_sockaddr_t sa_any;
 	int pf, i = 0;
 
-	/*
-	 * dispatch_allocate() checks mgr for us.
-	 */
-	disp = NULL;
-	result = dispatch_allocate(mgr, &disp);
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-
 	pf = isc_sockaddr_pf(localaddr);
-
+	dispatch_allocate(mgr, isc_sockettype_udp, pf, attributes, &disp);
-	disp->socktype = isc_sockettype_udp;
 
 	/*
 	 * For dispatches with a specified source address, we open a
@@ -2014,25 +2021,6 @@ dispatch_createudp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
 	isc_mem_create(&disp->sepool);
 	isc_mem_setname(disp->sepool, "disp_sepool");
 
-	attributes &= ~DNS_DISPATCHATTR_TCP;
-	attributes |= DNS_DISPATCHATTR_UDP;
-
-	switch (pf) {
-	case PF_INET:
-		attributes |= DNS_DISPATCHATTR_IPV4;
-		break;
-
-	case PF_INET6:
-		attributes |= DNS_DISPATCHATTR_IPV6;
-		break;
-
-	default:
-		result = ISC_R_NOTIMPLEMENTED;
-		goto kill_socket;
-	}
-
-	disp->attributes = attributes;
-
 	/*
 	 * Append it to the dispatcher list.
 	 */

lib/dns/include/dns/dispatch.h

@@ -223,10 +223,10 @@ dns_dispatch_createudp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
  */
 
 isc_result_t
-dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socket_t *sock,
+dns_dispatch_createtcp(dns_dispatchmgr_t *mgr, isc_socketmgr_t *sockmgr,
 		       isc_taskmgr_t *taskmgr, const isc_sockaddr_t *localaddr,
 		       const isc_sockaddr_t *destaddr, unsigned int attributes,
-		       dns_dispatch_t **dispp);
+		       isc_dscp_t dscp, dns_dispatch_t **dispp);
 /*%<
  * Create a new dns_dispatch and attach it to the provided isc_socket_t.
  *

lib/dns/request.c

@@ -498,9 +498,6 @@ tcp_dispatch(bool newtcp, dns_requestmgr_t *requestmgr,
 	     const isc_sockaddr_t *srcaddr, const isc_sockaddr_t *destaddr,
 	     isc_dscp_t dscp, bool *connected, dns_dispatch_t **dispatchp) {
 	isc_result_t result;
-	isc_socket_t *sock = NULL;
-	isc_sockaddr_t src;
-	isc_sockaddr_t bind_any;
 
 	if (!newtcp) {
 		result = dns_dispatch_gettcp(requestmgr->dispatchmgr, destaddr,
@@ -517,31 +514,9 @@ tcp_dispatch(bool newtcp, dns_requestmgr_t *requestmgr,
 		}
 	}
 
-	result = isc_socket_create(requestmgr->socketmgr,
+	result = dns_dispatch_createtcp(
-				   isc_sockaddr_pf(destaddr),
+		requestmgr->dispatchmgr, requestmgr->socketmgr,
-				   isc_sockettype_tcp, &sock);
+		requestmgr->taskmgr, srcaddr, destaddr, 0, dscp, dispatchp);
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-	if (srcaddr == NULL) {
-		isc_sockaddr_anyofpf(&bind_any, isc_sockaddr_pf(destaddr));
-		result = isc_socket_bind(sock, &bind_any, 0);
-	} else {
-		src = *srcaddr;
-		isc_sockaddr_setport(&src, 0);
-		result = isc_socket_bind(sock, &src, 0);
-	}
-	if (result != ISC_R_SUCCESS) {
-		goto cleanup;
-	}
-
-	isc_socket_dscp(sock, dscp);
-	result = dns_dispatch_createtcp(requestmgr->dispatchmgr, sock,
-					requestmgr->taskmgr, srcaddr, destaddr,
-					0, dispatchp);
-
-cleanup:
-	isc_socket_detach(&sock);
 	return (result);
 }
 

lib/dns/resolver.c

@@ -228,7 +228,6 @@ typedef struct query {
 	dns_dispatchmgr_t *dispatchmgr;
 	dns_dispatch_t *dispatch;
 	dns_adbaddrinfo_t *addrinfo;
-	isc_socket_t *tcpsocket;
 	isc_time_t start;
 	dns_messageid_t id;
 	dns_dispentry_t *dispentry;
@@ -1200,7 +1199,7 @@ resquery_destroy(resquery_t **queryp) {
 	*queryp = NULL;
 	REQUIRE(!ISC_LINK_LINKED(query, link));
 
-	INSIST(query->tcpsocket == NULL);
+	INSIST(query->dispatch == NULL);
 
 	fctx = query->fctx;
 	res = fctx->res;
@@ -1245,13 +1244,13 @@ update_edns_stats(resquery_t *query) {
 static void
 fctx_cancelquery(resquery_t **queryp, dns_dispatchevent_t **deventp,
 		 isc_time_t *finish, bool no_response, bool age_untried) {
-	fetchctx_t *fctx;
+	fetchctx_t *fctx = NULL;
-	resquery_t *query;
+	resquery_t *query = NULL;
 	unsigned int rtt, rttms;
 	unsigned int factor;
-	dns_adbfind_t *find;
+	dns_adbfind_t *find = NULL;
+	isc_socket_t *sock = NULL;
 	dns_adbaddrinfo_t *addrinfo;
-	isc_socket_t *sock;
 	isc_stdtime_t now;
 
 	query = *queryp;
@@ -1423,33 +1422,20 @@ fctx_cancelquery(resquery_t **queryp, dns_dispatchevent_t **deventp,
 	 * only needs to worry about managing the connect and send events;
 	 * the dispatcher manages the recv events.
 	 */
-	if (RESQUERY_CONNECTING(query)) {
+	if (query->dispentry != NULL) {
-		/*
+		sock = dns_dispatch_getentrysocket(query->dispentry);
-		 * Cancel the connect.
+	} else {
-		 */
+		sock = dns_dispatch_getsocket(query->dispatch);
-		if (query->tcpsocket != NULL) {
-			isc_socket_cancel(query->tcpsocket, NULL,
-					  ISC_SOCKCANCEL_CONNECT);
-		} else if (query->dispentry != NULL) {
-			sock = dns_dispatch_getentrysocket(query->dispentry);
-			if (sock != NULL) {
-				isc_socket_cancel(sock, NULL,
-						  ISC_SOCKCANCEL_CONNECT);
-			}
-		}
 	}
-	if (RESQUERY_SENDING(query)) {
+
-		/*
+	/* Cancel the connect. */
-		 * Cancel the pending send.
+	if (sock != NULL && RESQUERY_CONNECTING(query)) {
-		 */
+		isc_socket_cancel(sock, NULL, ISC_SOCKCANCEL_CONNECT);
-		if (query->dispentry != NULL) {
+	}
-			sock = dns_dispatch_getentrysocket(query->dispentry);
+
-		} else {
+	/* Cancel the pending send. */
-			sock = dns_dispatch_getsocket(query->dispatch);
+	if (sock != NULL && RESQUERY_SENDING(query)) {
-		}
+		isc_socket_cancel(sock, NULL, ISC_SOCKCANCEL_SEND);
-		if (sock != NULL) {
-			isc_socket_cancel(sock, NULL, ISC_SOCKCANCEL_SEND);
-		}
 	}
 
 	if (query->dispentry != NULL) {
@@ -1829,19 +1815,12 @@ process_sendevent(resquery_t *query, isc_event_t *event) {
 	bool destroy_query = false;
 	bool retry = false;
 	isc_result_t result;
-	fetchctx_t *fctx;
+	fetchctx_t *fctx = NULL;
 
 	fctx = query->fctx;
 
 	if (RESQUERY_CANCELED(query)) {
 		if (query->sends == 0 && query->connects == 0) {
-			/*
-			 * This query was canceled while the
-			 * isc_socket_sendto/connect() was in progress.
-			 */
-			if (query->tcpsocket != NULL) {
-				isc_socket_detach(&query->tcpsocket);
-			}
 			destroy_query = true;
 		}
 	} else {
@@ -2007,10 +1986,10 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) {
 static isc_result_t
 fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 	   unsigned int options) {
-	dns_resolver_t *res;
+	dns_resolver_t *res = NULL;
-	isc_task_t *task;
+	isc_task_t *task = NULL;
 	isc_result_t result;
-	resquery_t *query;
+	resquery_t *query = NULL;
 	isc_sockaddr_t addr;
 	bool have_addr = false;
 	unsigned int srtt;
@@ -2123,21 +2102,12 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 			query->dscp = dscp;
 		}
 
-		result = isc_socket_create(res->socketmgr, pf,
+		result = dns_dispatch_createtcp(
-					   isc_sockettype_tcp,
+			res->dispatchmgr, res->socketmgr, res->taskmgr, &addr,
-					   &query->tcpsocket);
+			&addrinfo->sockaddr, 0, query->dscp, &query->dispatch);
 		if (result != ISC_R_SUCCESS) {
 			goto cleanup_query;
 		}
-
-		result = isc_socket_bind(query->tcpsocket, &addr, 0);
-		if (result != ISC_R_SUCCESS) {
-			goto cleanup_socket;
-		}
-
-		/*
-		 * A dispatch will be created once the connect succeeds.
-		 */
 	} else {
 		if (have_addr) {
 			switch (isc_sockaddr_pf(&addr)) {
@@ -2195,19 +2165,16 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 	query->magic = QUERY_MAGIC;
 
 	if ((query->options & DNS_FETCHOPT_TCP) != 0) {
+		isc_socket_t *sock = NULL;
+
 		/*
 		 * Connect to the remote server.
-		 *
-		 * XXXRTH  Should we attach to the socket?
 		 */
-		if (query->dscp != -1) {
+		sock = dns_dispatch_getsocket(query->dispatch);
-			isc_socket_dscp(query->tcpsocket, query->dscp);
+		result = isc_socket_connect(sock, &addrinfo->sockaddr, task,
-		}
-		result = isc_socket_connect(query->tcpsocket,
-					    &addrinfo->sockaddr, task,
 					    resquery_connected, query);
 		if (result != ISC_R_SUCCESS) {
-			goto cleanup_socket;
+			goto cleanup_dispatch;
 		}
 		query->connects++;
 		QTRACE("connecting via TCP");
@@ -2244,9 +2211,6 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
 
 	return (ISC_R_SUCCESS);
 
-cleanup_socket:
-	isc_socket_detach(&query->tcpsocket);
-
 cleanup_dispatch:
 	if (query->dispatch != NULL) {
 		dns_dispatch_detach(&query->dispatch);
@@ -2961,11 +2925,8 @@ resquery_connected(isc_task_t *task, isc_event_t *event) {
 		 * This query was canceled while the connect() was in
 		 * progress.
 		 */
-		isc_socket_detach(&query->tcpsocket);
 		resquery_destroy(&query);
 	} else {
-		int attrs = 0;
-
 		switch (sevent->result) {
 		case ISC_R_SUCCESS:
 
@@ -2989,27 +2950,16 @@ resquery_connected(isc_task_t *task, isc_event_t *event) {
 				fctx_done(fctx, result, __LINE__);
 				break;
 			}
+
 			/*
-			 * We are connected.  Create a dispatcher and
+			 * We are connected.  Update the dispatcher and
 			 * send the query.
 			 */
-			attrs = DNS_DISPATCHATTR_CONNECTED;
+			dns_dispatch_changeattributes(
-			result = dns_dispatch_createtcp(
+				query->dispatch, DNS_DISPATCHATTR_CONNECTED,
-				query->dispatchmgr, query->tcpsocket,
+				DNS_DISPATCHATTR_CONNECTED);
-				query->fctx->res->taskmgr, NULL, NULL, attrs,
-				&query->dispatch);
-
-			/*
-			 * Regardless of whether dns_dispatch_create()
-			 * succeeded or not, we don't need our reference
-			 * to the socket anymore.
-			 */
-			isc_socket_detach(&query->tcpsocket);
-
-			if (result == ISC_R_SUCCESS) {
-				result = resquery_send(query);
-			}
 
+			result = resquery_send(query);
 			if (result != ISC_R_SUCCESS) {
 				FCTXTRACE("query canceled: "
 					  "resquery_send() failed; responding");
@@ -3030,10 +2980,6 @@ resquery_connected(isc_task_t *task, isc_event_t *event) {
 				   "no route to host; no response",
 				   sevent->result);
 
-			/*
-			 * No route to remote.
-			 */
-			isc_socket_detach(&query->tcpsocket);
 			/*
 			 * Do not query this server again in this fetch context
 			 * if the server is unavailable over TCP.
@@ -3049,7 +2995,7 @@ resquery_connected(isc_task_t *task, isc_event_t *event) {
 				   "unexpected event result; responding",
 				   sevent->result);
 
-			isc_socket_detach(&query->tcpsocket);
+			dns_dispatch_detach(&query->dispatch);
 			fctx_cancelquery(&query, NULL, NULL, false, false);
 			break;
 		}