Merge branch '1392-initial-tcp-highwater-wrong' into 'master'

Resolve "Initial TCP high-water value is wrong"

Closes #1392

See merge request isc-projects/bind9!2610

CHANGES

@@ -1,3 +1,6 @@
+5336.	[bug]		The TCP high-water statistic could report an
+			incorrect value on startup. [GL #1392]
+
 5335.	[func]		Make TCP listening code multithreaded. [GL !2659]
 
 5334.	[doc]		Update documentation with dnssec-policy clarifications.

bin/tests/system/tcp/tests.sh

@@ -105,6 +105,10 @@ echo_i "TCP high-water: check initial statistics ($n)"
 ret=0
 refresh_tcp_stats
 assert_int_equal "${TCP_CUR}" 0 "current TCP clients count" || ret=1
+# We compare initial tcp-highwater value with 1 because as part of the
+# system test startup, the script start.pl executes dig to check if target
+# named is running, and that increments tcp-quota by one.
+assert_int_equal "${TCP_HIGH}" 1 "tcp-highwater count" || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status + ret))
 

lib/isc/include/isc/netmgr.h

@@ -264,10 +264,10 @@ isc_nm_tcp_stoplistening(isc_nmsocket_t *sock);
 
 isc_result_t
 isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface,
-		     isc_nm_recv_cb_t cb, void *arg,
-		     size_t extrahandlesize, int backlog,
-		     isc_quota_t *quota,
-		     isc_nmsocket_t **sockp);
+		    isc_nm_recv_cb_t cb, void *cbarg,
+		    isc_nm_cb_t accept_cb, void *accept_cbarg,
+		    size_t extrahandlesize, int backlog, isc_quota_t *quota,
+		    isc_nmsocket_t **sockp);
 /*%<
  * Start listening for DNS messages over the TCP interface 'iface', using
  * net manager 'mgr'.
@@ -281,6 +281,9 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface,
  * When a complete DNS message is received on the socket, 'cb' will be
  * called with 'cbarg' as its argument.
  *
+ * When a new TCP connection is accepted, 'accept_cb' will be called
+ * with 'accept_cbarg' as its argument.
+ *
  * When handles are allocated for the socket, 'extrasize' additional bytes
  * will be allocated along with the handle for an associated object
  * (typically ns_client).

lib/isc/netmgr/netmgr-int.h

@@ -477,6 +477,9 @@ struct isc_nmsocket {
 
 	isc__nm_readcb_t	rcb;
 	void			*rcbarg;
+
+	isc__nm_cb_t		accept_cb;
+	void 			*accept_cbarg;
 };
 
 bool

lib/isc/netmgr/tcpdns.c

@@ -96,7 +96,7 @@ dnstcp_readtimeout(uv_timer_t *timer) {
 }
 
 /*
- * Accept callback for TCP-DNS connection
+ * Accept callback for TCP-DNS connection.
  */
 static void
 dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
@@ -111,10 +111,14 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
 		return;
 	}
 
+	if (dnslistensock->accept_cb.accept != NULL) {
+		dnslistensock->accept_cb.accept(handle, ISC_R_SUCCESS,
+						dnslistensock->accept_cbarg);
+	}
+
 	/* We need to create a 'wrapper' dnssocket for this connection */
 	dnssock = isc_mem_get(handle->sock->mgr->mctx, sizeof(*dnssock));
-	isc__nmsocket_init(dnssock, handle->sock->mgr,
-			   isc_nm_tcpdnssocket);
+	isc__nmsocket_init(dnssock, handle->sock->mgr, isc_nm_tcpdnssocket);
 
 	/* We need to copy read callbacks from outer socket */
 	dnssock->rcb.recv = dnslistensock->rcb.recv;
@@ -278,8 +282,8 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
 isc_result_t
 isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface,
 		    isc_nm_recv_cb_t cb, void *cbarg,
-		    size_t extrahandlesize, int backlog,
-		    isc_quota_t *quota,
+		    isc_nm_cb_t accept_cb, void *accept_cbarg,
+		    size_t extrahandlesize, int backlog, isc_quota_t *quota,
 		    isc_nmsocket_t **sockp)
 {
 	/* A 'wrapper' socket object with outer set to true TCP socket */
@@ -293,6 +297,8 @@ isc_nm_listentcpdns(isc_nm_t *mgr, isc_nmiface_t *iface,
 	dnslistensock->iface = iface;
 	dnslistensock->rcb.recv = cb;
 	dnslistensock->rcbarg = cbarg;
+	dnslistensock->accept_cb.accept = accept_cb;
+	dnslistensock->accept_cbarg = accept_cbarg;
 	dnslistensock->extrahandlesize = extrahandlesize;
 
 	/* We set dnslistensock->outer to a true listening socket */

lib/ns/client.c

@@ -1657,11 +1657,6 @@ ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
 	}
 	if (isc_nmhandle_is_stream(handle)) {
 		client->attributes |= NS_CLIENTATTR_TCP;
-		unsigned int curr_tcpquota =
-			isc_quota_getused(&client->sctx->tcpquota);
-		ns_stats_update_if_greater(client->sctx->nsstats,
-					   ns_statscounter_tcphighwater,
-					   curr_tcpquota);
 	}
 
 	INSIST(client->recursionquota == NULL);
@@ -2185,6 +2180,20 @@ ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg) {
 	isc_task_unpause(client->task);
 }
 
+void
+ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
+	ns_server_t *sctx = (ns_server_t *) arg;
+	unsigned int tcpquota;
+
+	UNUSED(handle);
+	UNUSED(result);
+
+	tcpquota = isc_quota_getused(&sctx->tcpquota);
+	ns_stats_update_if_greater(sctx->nsstats,
+				   ns_statscounter_tcphighwater,
+				   tcpquota);
+}
+
 static void
 get_clientmctx(ns_clientmgr_t *manager, isc_mem_t **mctxp) {
 	isc_mem_t *clientmctx;

lib/ns/include/ns/client.h

@@ -487,7 +487,7 @@ isc_result_t
 ns_client_addopt(ns_client_t *client, dns_message_t *message,
 		 dns_rdataset_t **opt);
 
-/*
+/*%<
  * Get a client object from the inactive queue, or create one, as needed.
  * (Not intended for use outside this module and associated tests.)
  */
@@ -495,11 +495,19 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message,
 void
 ns__client_request(isc_nmhandle_t *handle, isc_region_t *region, void *arg);
 
-/*
+/*%<
  * Handle client requests.
  * (Not intended for use outside this module and associated tests.)
  */
 
+ void
+ ns__client_tcpconn(isc_nmhandle_t *handle, isc_result_t result, void *arg);
+
+ /*%<
+  * Called every time a TCP connection is establish.  This is used for
+  * updating TCP statistics.
+  */
+
 dns_rdataset_t *
 ns_client_newrdataset(ns_client_t *client);
 

lib/ns/interfacemgr.c

@@ -28,6 +28,7 @@
 #include <ns/log.h>
 #include <ns/interfacemgr.h>
 #include <ns/server.h>
+#include <ns/stats.h>
 
 #ifdef HAVE_NET_ROUTE_H
 #include <net/route.h>
@@ -463,6 +464,7 @@ ns_interface_listentcp(ns_interface_t *ifp) {
 	result = isc_nm_listentcpdns(ifp->mgr->nm,
 				     (isc_nmiface_t *) &ifp->addr,
 				     ns__client_request, ifp,
+				     ns__client_tcpconn, ifp->mgr->sctx,
 				     sizeof(ns_client_t),
 				     ifp->mgr->backlog,
 				     &ifp->mgr->sctx->tcpquota,
@@ -473,6 +475,13 @@ ns_interface_listentcp(ns_interface_t *ifp) {
 				 isc_result_totext(result));
 	}
 
+	/*
+	 * We call this now to update the tcp-highwater statistic:
+	 * this is necessary because we are adding to the TCP quota just
+	 * by listening.
+	 */
+	ns__client_tcpconn(NULL, ISC_R_SUCCESS, ifp->mgr->sctx);
+
 #if 0
 #ifndef ISC_ALLOW_MAPPED
 	isc_socket_ipv6only(ifp->tcpsocket, true);