[master] expanded libns unit tests

4772.	[test]		Expanded unit testing framework for libns, using
			hooks to interrupt query flow and inspect state
			at specified locations. [RT #46173]

CHANGES

+4772.	[test]		Expanded unit testing framework for libns, using
+			hooks to interrupt query flow and inspect state
+			at specified locations. [RT #46173]
+
 4771.	[bug]		When sending RFC 5011 refresh queries, disregard
 			cached DNSKEY rrsets. [RT #46251]

configure

@@ -940,6 +940,7 @@ infodir
 docdir
 oldincludedir
 includedir
+runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -1103,6 +1104,7 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1355,6 +1357,15 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
+  -runstatedir | --runstatedir | --runstatedi | --runstated \
+  | --runstate | --runstat | --runsta | --runst | --runs \
+  | --run | --ru | --r)
+    ac_prev=runstatedir ;;
+  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
+  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
+  | --run=* | --ru=* | --r=*)
+    runstatedir=$ac_optarg ;;
+
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1492,7 +1503,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir
+		libdir localedir mandir runstatedir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1645,6 +1656,7 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
+  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -11466,7 +11478,7 @@ fi
 XTARGETS=
 case "$enable_developer" in
 yes)
-	STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1"
+	STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1 -DNS_HOOKS_ENABLE=1"
 	test "${enable_fixed_rrset+set}" = set || enable_fixed_rrset=yes
 	test "${enable_querytrace+set}" = set || enable_querytrace=yes
 	test "${with_atf+set}" = set || with_atf=yes
@@ -22225,6 +22237,7 @@ if test "no" != "$atf"; then
 $as_echo "#define ATF_TEST 1" >>confdefs.h
 	STD_CINCLUDES="$STD_CINCLUDES -I$atf/include"
+        STD_CDEFINES="$STD_CDEFINES -DNS_HOOKS_ENABLE=1"
 	ATFBIN="$atf/bin"
 	ATFLIBS="-L$atf/lib -latf-c"
 	{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for exp in -lm" >&5

configure.in

@@ -62,7 +62,7 @@ AC_ARG_ENABLE(developer, [  --enable-developer      enable developer build setti
 XTARGETS=
 case "$enable_developer" in
 yes)
-	STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1"
+	STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1 -DNS_HOOKS_ENABLE=1"
 	test "${enable_fixed_rrset+set}" = set || enable_fixed_rrset=yes
 	test "${enable_querytrace+set}" = set || enable_querytrace=yes
 	test "${with_atf+set}" = set || with_atf=yes
@@ -4771,6 +4771,7 @@ ATFLIBS=
 if test "no" != "$atf"; then
 	AC_DEFINE(ATF_TEST, 1, [define if ATF unit tests are to be built.])
 	STD_CINCLUDES="$STD_CINCLUDES -I$atf/include"
+        STD_CDEFINES="$STD_CDEFINES -DNS_HOOKS_ENABLE=1"
 	ATFBIN="$atf/bin"
 	ATFLIBS="-L$atf/lib -latf-c"
 	AC_CHECK_LIB(m, exp, libm=yes, libm=no)

lib/ns/hooks.h

+/*
+ * Copyright (C) 2017  Internet Systems Consortium, Inc. ("ISC")
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#ifndef NS_HOOKS_H
+#define NS_HOOKS_H 1
+
+#ifdef NS_HOOKS_ENABLE
+
+/*! \file */
+
+#include <isc/result.h>
+
+/*
+ * Hooks provide a way of running a callback function once a certain place in
+ * code is reached.  Current use is limited to libns unit tests and thus:
+ *
+ *   - hook-related types and macros are not placed in libns header files,
+ *   - hook-related code is compiled away unless --enable-developer is used,
+ *   - hook-related macro names are prefixed with "NS_".
+ *
+ * However, the implementation is pretty generic and could be repurposed for
+ * general use, e.g. as part of libisc, after some further customization.
+ *
+ * Hooks are created by inserting a macro into any function returning
+ * isc_result_t (NS_PROCESS_HOOK()) or void (NS_PROCESS_HOOK_VOID()).  Each
+ * hook has an identifier, which is an integer that is an index into the hook
+ * table.  In an attempt to keep things as simple as possible, current
+ * implementation:
+ *
+ *   - uses hook tables which are statically-sized arrays only allowing a
+ *     single callback to be invoked for each hook identifier,
+ *   - only supports replacing whole hook tables.
+ *
+ * Hook callbacks are functions which:
+ *
+ *   - return a boolean value; if ISC_TRUE is returned by the callback, the
+ *     function into which the hook is inserted will return at hook insertion
+ *     point; if ISC_FALSE is returned by the callback, execution of the
+ *     function into which the hook is inserted continues normally,
+ *
+ *   - accept three pointers as arguments:
+ *
+ *       - a pointer specified by the hook itself,
+ *       - a pointer specified upon inserting the callback into the hook table,
+ *       - a pointer to isc_result_t which will be returned by the function
+ *         into which the hook is inserted if the callback returns ISC_TRUE.
+ *
+ * In order for a hook callback to be called for a given hook, a pointer to
+ * that callback (along with an optional pointer to callback-specific data) has
+ * to be inserted into the hook table entry for that hook.
+ *
+ * Consider the following sample code:
+ *
+ * ----------------------------------------------------------------------------
+ * const ns_hook_t *foo_hook_table = NULL;
+ *
+ * isc_result_t
+ * foo_bar(void) {
+ *     int val = 42;
+ *     ...
+ *     NS_PROCESS_HOOK(foo_hook_table, FOO_EXTRACT_VAL, &val);
+ *     ...
+ *     printf("This message may not be printed due to use of hooks.");
+ *
+ *     return (ISC_R_SUCCESS);
+ * }
+ *
+ * isc_boolean_t
+ * cause_failure(void *hook_data, void *callback_data, isc_result_t *resultp) {
+ *     ...
+ *     *resultp = ISC_R_FAILURE;
+ *
+ *     return (ISC_TRUE);
+ * }
+ *
+ * void
+ * test_foo_bar(void) {
+ *     isc_boolean_t foo_bar_called = ISC_FALSE;
+ *     const ns_hook_t my_hooks[FOO_HOOKS_COUNT] = {
+ *         [FOO_EXTRACT_VAL] = {
+ *             .callback = cause_failure,
+ *             .callback_data = &foo_bar_called,
+ *         },
+ *     };
+ *
+ *     foo_hook_table = my_hooks;
+ *
+ *     foo_bar();
+ * }
+ * ----------------------------------------------------------------------------
+ *
+ * When test_foo_bar() is called, the hook table is first replaced.  Then
+ * foo_bar() gets invoked.  Once execution reaches the insertion point for hook
+ * FOO_EXTRACT_VAL, cause_failure() will be called with &val as hook_data and
+ * &foo_bar_called as callback_data.  It can do whatever it pleases with these
+ * two values.  Eventually, cause_failure() sets *resultp to ISC_R_FAILURE and
+ * returns ISC_TRUE, which causes foo_bar() to return ISC_R_FAILURE and never
+ * execute the printf() call below hook insertion point.
+ */
+
+enum {
+	NS_QUERY_SETUP_QCTX_INITIALIZED,
+	NS_QUERY_LOOKUP_BEGIN,
+	NS_QUERY_DONE_BEGIN,
+	NS_QUERY_HOOKS_COUNT
+};
+
+typedef isc_boolean_t
+(*ns_hook_cb_t)(void *hook_data, void *callback_data, isc_result_t *resultp);
+
+typedef struct ns_hook {
+	ns_hook_cb_t callback;
+	void *callback_data;
+} ns_hook_t;
+
+#define _NS_PROCESS_HOOK(table, id, data, ...)				\
+	if (table != NULL) {						\
+		ns_hook_cb_t _callback = table[id].callback;		\
+		void *_callback_data = table[id].callback_data;		\
+		isc_result_t _result;					\
+									\
+		if (_callback != NULL &&				\
+		    _callback(data, _callback_data, &_result)) {	\
+			return __VA_ARGS__;				\
+		}							\
+	}
+
+#define NS_PROCESS_HOOK(table, id, data) \
+	_NS_PROCESS_HOOK(table, id, data, _result)
+
+#define NS_PROCESS_HOOK_VOID(table, id, data) \
+	_NS_PROCESS_HOOK(table, id, data)
+
+LIBNS_EXTERNAL_DATA extern const ns_hook_t *ns__hook_table;
+
+#endif /* NS_HOOKS_ENABLE */
+#endif /* NS_HOOKS_H */

lib/ns/include/ns/query.h

@@ -16,6 +16,7 @@
 #include <isc/netaddr.h>
 
 #include <dns/rdataset.h>
+#include <dns/resolver.h>
 #include <dns/rpz.h>
 #include <dns/types.h>
 
@@ -92,6 +93,55 @@ struct ns_query {
 #define NS_QUERYATTR_RRL_CHECKED	0x10000
 #define NS_QUERYATTR_REDIRECT		0x20000
 
+/* query context structure */
+
+typedef struct query_ctx {
+	isc_buffer_t *dbuf;			/* name buffer */
+	dns_name_t *fname;			/* found name from DB lookup */
+	dns_rdataset_t *rdataset;		/* found rdataset */
+	dns_rdataset_t *sigrdataset;		/* found sigrdataset */
+	dns_rdataset_t *noqname;		/* rdataset needing
+						 * NOQNAME proof */
+	dns_rdatatype_t qtype;
+	dns_rdatatype_t type;
+
+	unsigned int options;			/* DB lookup options */
+
+	isc_boolean_t redirected;		/* nxdomain redirected? */
+	isc_boolean_t is_zone;			/* is DB a zone DB? */
+	isc_boolean_t is_staticstub_zone;
+	isc_boolean_t resuming;			/* resumed from recursion? */
+	isc_boolean_t dns64, dns64_exclude, rpz;
+	isc_boolean_t authoritative;		/* authoritative query? */
+	isc_boolean_t want_restart;		/* CNAME chain or other
+						 * restart needed */
+	isc_boolean_t need_wildcardproof;	/* wilcard proof needed */
+	isc_boolean_t nxrewrite;		/* negative answer from RPZ */
+	isc_boolean_t findcoveringnsec;		/* lookup covering NSEC */
+	isc_boolean_t want_stale;		/* want stale records? */
+	dns_fixedname_t wildcardname;		/* name needing wcard proof */
+	dns_fixedname_t dsname;			/* name needing DS */
+
+	ns_client_t *client;			/* client object */
+	dns_fetchevent_t *event;		/* recursion event */
+
+	dns_db_t *db;				/* zone or cache database */
+	dns_dbversion_t *version;		/* DB version */
+	dns_dbnode_t *node;			/* DB node */
+
+	dns_db_t *zdb;				/* zone DB values, saved */
+	dns_name_t *zfname;			/* while searching cache */
+	dns_dbversion_t *zversion;		/* for a better answer */
+	dns_rdataset_t *zrdataset;
+	dns_rdataset_t *zsigrdataset;
+
+	dns_rpz_st_t *rpz_st;			/* RPZ state */
+	dns_zone_t *zone;			/* zone to search */
+
+	isc_result_t result;			/* query result */
+	int line;				/* line to report error */
+} query_ctx_t;
+
 isc_result_t
 ns_query_init(ns_client_t *client);
 
@@ -104,4 +154,16 @@ ns_query_start(ns_client_t *client);
 void
 ns_query_cancel(ns_client_t *client);
 
+/*%
+ * (Must not be used outside this module and its associated unit tests.)
+ */
+isc_result_t
+ns__query_sfcache(query_ctx_t *qctx);
+
+/*%
+ * (Must not be used outside this module and its associated unit tests.)
+ */
+isc_result_t
+ns__query_start(query_ctx_t *qctx);
+
 #endif /* NS_QUERY_H */

lib/ns/query.c

@@ -62,6 +62,8 @@
 #include <ns/stats.h>
 #include <ns/xfrout.h>
 
+#include "hooks.h"
+
 #if 0
 /*
  * It has been recommended that DNS64 be changed to return excluded
@@ -243,17 +245,32 @@ static void
 log_noexistnodata(void *val, int level, const char *fmt, ...)
 	ISC_FORMAT_PRINTF(3, 4);
 
-/*%
- * The structure and functions defined below implement the query logic
- * that previously lived in the single very complex function query_find().
- * The query_ctx_t structure maintains state from function to function.
- * The call flow for the general query processing algorithm is described
- * below:
+#ifdef NS_HOOKS_ENABLE
+
+LIBNS_EXTERNAL_DATA const ns_hook_t *ns__hook_table = NULL;
+
+#define PROCESS_HOOK(...) \
+	NS_PROCESS_HOOK(ns__hook_table, __VA_ARGS__)
+#define PROCESS_HOOK_VOID(...) \
+	NS_PROCESS_HOOK_VOID(ns__hook_table, __VA_ARGS__)
+
+#else
+
+#define PROCESS_HOOK(...)	do {} while (0)
+#define PROCESS_HOOK_VOID(...)	do {} while (0)
+
+#endif
+
+/*
+ * The functions defined below implement the query logic that previously lived
+ * in the single very complex function query_find().  The query_ctx_t structure
+ * defined in <ns/query.h> maintains state from function to function.  The call
+ * flow for the general query processing algorithm is described below:
  *
  * 1. Set up query context and other resources for a client
  *    query (query_setup())
  *
- * 2. Start the search (query_start())
+ * 2. Start the search (ns__query_start())
  *
  * 3. Identify authoritative data sources which may have an answer;
  *    search them (query_lookup()). If an answer is found, go to 7.
@@ -310,53 +327,6 @@ log_noexistnodata(void *val, int level, const char *fmt, ...)
  * DNS64, filter-aaaa, RPZ, RRL, and the SERVFAIL cache.)
  */
 
-typedef struct query_ctx {
-	isc_buffer_t *dbuf;			/* name buffer */
-	dns_name_t *fname;			/* found name from DB lookup */
-	dns_rdataset_t *rdataset;		/* found rdataset */
-	dns_rdataset_t *sigrdataset;		/* found sigrdataset */
-	dns_rdataset_t *noqname;		/* rdataset needing
-						 * NOQNAME proof */
-	dns_rdatatype_t qtype;
-	dns_rdatatype_t type;
-
-	unsigned int options;			/* DB lookup options */
-
-	isc_boolean_t redirected;		/* nxdomain redirected? */
-	isc_boolean_t is_zone;			/* is DB a zone DB? */
-	isc_boolean_t is_staticstub_zone;
-	isc_boolean_t resuming;			/* resumed from recursion? */
-	isc_boolean_t dns64, dns64_exclude, rpz;
-	isc_boolean_t authoritative;		/* authoritative query? */
-	isc_boolean_t want_restart;		/* CNAME chain or other
-						 * restart needed */
-	isc_boolean_t need_wildcardproof;	/* wilcard proof needed */
-	isc_boolean_t nxrewrite;		/* negative answer from RPZ */
-	isc_boolean_t findcoveringnsec;		/* lookup covering NSEC */
-	isc_boolean_t want_stale;		/* want stale records? */
-	dns_fixedname_t wildcardname;		/* name needing wcard proof */
-	dns_fixedname_t dsname;			/* name needing DS */
-
-	ns_client_t *client;			/* client object */
-	dns_fetchevent_t *event;		/* recursion event */
-
-	dns_db_t *db;				/* zone or cache database */
-	dns_dbversion_t *version;		/* DB version */
-	dns_dbnode_t *node;			/* DB node */
-
-	dns_db_t *zdb;				/* zone DB values, saved */
-	dns_name_t *zfname;			/* while searching cache */
-	dns_dbversion_t *zversion;		/* for a better answer */
-	dns_rdataset_t *zrdataset;
-	dns_rdataset_t *zsigrdataset;
-
-	dns_rpz_st_t *rpz_st;			/* RPZ state */
-	dns_zone_t *zone;			/* zone to search */
-
-	isc_result_t result;			/* query result */
-	int line;				/* line to report error */
-} query_ctx_t;
-
 static void
 query_trace(query_ctx_t *qctx);
 
@@ -367,9 +337,6 @@ qctx_init(ns_client_t *client, dns_fetchevent_t *event,
 static isc_result_t
 query_setup(ns_client_t *client, dns_rdatatype_t qtype);
 
-static isc_result_t
-query_start(query_ctx_t *qctx);
-
 static isc_result_t
 query_lookup(query_ctx_t *qctx);
 
@@ -384,9 +351,6 @@ query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname,
 static isc_result_t
 query_resume(query_ctx_t *qctx);
 
-static isc_result_t
-query_sfcache(query_ctx_t *qctx);
-
 static isc_result_t
 query_checkrrl(query_ctx_t *qctx, isc_result_t result);
 
@@ -5121,12 +5085,12 @@ query_trace(query_ctx_t *qctx) {
 /*
  * Set up query processing for the current query of 'client'.
  * Calls qctx_init() to initialize a query context, checks
- * the SERVFAIL cache, then hands off processing to query_start().
+ * the SERVFAIL cache, then hands off processing to ns__query_start().
  *
  * This is called only from ns_query_start(), to begin a query
  * for the first time.  Restarting an existing query (for
  * instance, to handle CNAME lookups), is done by calling
- * query_start() again with the same query context. Resuming from
+ * ns__query_start() again with the same query context. Resuming from
  * recursion is handled by query_resume().
  */
 static isc_result_t
@@ -5137,14 +5101,6 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) {
 	qctx_init(client, NULL, qtype, &qctx);
 	query_trace(&qctx);
 
-	/*
-	 * Check SERVFAIL cache
-	 */
-	result = query_sfcache(&qctx);
-	if (result != ISC_R_COMPLETE) {
-		return (result);
-	}
-
 	/*
 	 * If it's a SIG query, we'll iterate the node.
 	 */
@@ -5152,11 +5108,19 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) {
 	    qctx.qtype == dns_rdatatype_sig)
 	{
 		qctx.type = dns_rdatatype_any;
-	} else {
-		qctx.type = qctx.qtype;
 	}
 
-	return (query_start(&qctx));
+	PROCESS_HOOK(NS_QUERY_SETUP_QCTX_INITIALIZED, &qctx);
+
+	/*
+	 * Check SERVFAIL cache
+	 */
+	result = ns__query_sfcache(&qctx);
+	if (result != ISC_R_COMPLETE) {
+		return (result);
+	}
+
+	return (ns__query_start(&qctx));
 }
 
 /*%
@@ -5166,10 +5130,10 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) {
  * follow a CNAME chain.  Determines which authoritative database to
  * search, then hands off processing to query_lookup().
  */
-static isc_result_t
-query_start(query_ctx_t *qctx) {
+isc_result_t
+ns__query_start(query_ctx_t *qctx) {
 	isc_result_t result;
-	CCTRACE(ISC_LOG_DEBUG(3), "query_start");
+	CCTRACE(ISC_LOG_DEBUG(3), "ns__query_start");
 	qctx->want_restart = ISC_FALSE;
 	qctx->authoritative = ISC_FALSE;
 	qctx->version = NULL;
@@ -5206,6 +5170,12 @@ query_start(query_ctx_t *qctx) {
 	if (dns_rdatatype_atparent(qctx->qtype) &&
 	    !dns_name_equal(qctx->client->query.qname, dns_rootname))
 	{
+		/*
+		 * If authoritative data for this QTYPE is supposed to live in
+		 * the parent zone, do not look for an exact match for QNAME,
+		 * but rather for its containing zone (unless the QNAME is
+		 * root).
+		 */
 		qctx->options |= DNS_GETDB_NOEXACT;
 	}
 
@@ -5218,8 +5188,10 @@ query_start(query_ctx_t *qctx) {
 			 (qctx->options & DNS_GETDB_NOEXACT) != 0))
 	{
 		/*
-		 * If the query type is DS, look to see if we are
-		 * authoritative for the child zone.
+		 * This is a non-recursive QTYPE=DS query with QNAME whose
+		 * parent we are not authoritative for.  Check whether we are
+		 * authoritative for QNAME, because if so, we need to send a
+		 * "no data" response as required by RFC 4035, section 3.1.4.1.
 		 */
 		dns_db_t *tdb = NULL;
 		dns_zone_t *tzone = NULL;
@@ -5232,6 +5204,10 @@ query_start(query_ctx_t *qctx) {
 					  DNS_GETDB_PARTIAL,
 					  &tzone, &tdb, &tversion);
 		if (tresult == ISC_R_SUCCESS) {
+			/*
+			 * We are authoritative for QNAME.  Attach the relevant
+			 * zone to query context, set result to ISC_R_SUCCESS.
+			 */
 			qctx->options &= ~DNS_GETDB_NOEXACT;
 			query_putrdataset(qctx->client, &qctx->rdataset);
 			if (qctx->db != NULL) {
@@ -5247,6 +5223,10 @@ query_start(query_ctx_t *qctx) {
 			qctx->is_zone = ISC_TRUE;
 			result = ISC_R_SUCCESS;
 		} else {
+			/*
+			 * We are not authoritative for QNAME.  Clean up and
+			 * leave result as it was.
+			 */
 			if (tdb != NULL) {
 				dns_db_detach(&tdb);
 			}
@@ -5255,6 +5235,11 @@ query_start(query_ctx_t *qctx) {
 			}
 		}
 	}
+	/*
+	 * If we did not find a database from which we can answer the query,
+	 * respond with either REFUSED or SERVFAIL, depending on what the
+	 * result of query_getdb() was.
+	 */
 	if (result != ISC_R_SUCCESS) {
 		if (result == DNS_R_REFUSED) {
 			if (WANTRECURSION(qctx->client)) {
@@ -5269,12 +5254,17 @@ query_start(query_ctx_t *qctx) {
 			}
 		} else {
 			CCTRACE(ISC_LOG_ERROR,
-			       "query_start: query_getdb failed");
+			       "ns__query_start: query_getdb failed");
 			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
 		}
 		return (query_done(qctx));
 	}
 
+	/*
+	 * We found a database from which we can answer the query.  Update
+	 * relevant query context flags if the answer is to be prepared using
+	 * authoritative data.
+	 */
 	qctx->is_staticstub_zone = ISC_FALSE;
 	if (qctx->is_zone) {
 		qctx->authoritative = ISC_TRUE;
@@ -5285,6 +5275,10 @@ query_start(query_ctx_t *qctx) {
 		}
 	}
 
+	/*
+	 * Attach to the database which will be used to prepare the answer.
+	 * Update query statistics.
+	 */
 	if (qctx->event == NULL && qctx->client->query.restarts == 0) {
 		if (qctx->is_zone) {
 			if (qctx->zone != NULL) {
@@ -5326,6 +5320,8 @@ query_lookup(query_ctx_t *qctx) {
 
 	CCTRACE(ISC_LOG_DEBUG(3), "query_lookup");
 
+	PROCESS_HOOK(NS_QUERY_LOOKUP_BEGIN, qctx);
+
 	dns_clientinfomethods_init(&cm, ns_client_sourceip);
 	dns_clientinfo_init(&ci, qctx->client, NULL);
 
@@ -5884,10 +5880,11 @@ query_resume(query_ctx_t *qctx) {
  * If the query is recursive, check the SERVFAIL cache to see whether
  * identical queries have failed recently.  If we find a match, and it was
  * from a query with CD=1, *or* if the current query has CD=0, then we just
- * return SERVFAIL again.
+ * return SERVFAIL again.  This prevents a validation failure from eliciting a
+ * SERVFAIL response to a CD=1 query.
  */
-static isc_result_t
-query_sfcache(query_ctx_t *qctx) {
+isc_result_t
+ns__query_sfcache(query_ctx_t *qctx) {
 	isc_boolean_t failcache;
 	isc_uint32_t flags;
 
@@ -10317,7 +10314,7 @@ query_glueanswer(query_ctx_t *qctx) {
  *
  * - Clean up
  * - If we have an answer ready (positive or negative), send it.
- * - If we need to restart for a chaining query, call query_start() again.
+ * - If we need to restart for a chaining query, call ns__query_start() again.
  * - If we've started recursion, then just clean up; things will be
  *   restarted via fetch_callback()/query_resume().
  */
@@ -10326,6 +10323,8 @@ query_done(query_ctx_t *qctx) {
 	const dns_namelist_t *secs = qctx->client->message->sections;
 	CCTRACE(ISC_LOG_DEBUG(3), "query_done");
 
+	PROCESS_HOOK(NS_QUERY_DONE_BEGIN, qctx);
+
 	/*
 	 * General cleanup.
 	 */
@@ -10352,7 +10351,7 @@ query_done(query_ctx_t *qctx) {
 	 */
 	if (qctx->want_restart && qctx->client->query.restarts < MAX_RESTARTS) {
 		qctx->client->query.restarts++;
-		return (query_start(qctx));
+		return (ns__query_start(qctx));
 	}
 
 	if (qctx->want_stale) {

lib/ns/tests/Makefile.in

@@ -32,11 +32,13 @@ LIBS =		@LIBS@ @ATFLIBS@
 OBJS =		nstest.@O@
 SRCS =		nstest.c \
 		listenlist_test.c \
-		notify_test.c
+		notify_test.c \
+		query_test.c
 
 SUBDIRS =
 TARGETS =	listenlist_test@EXEEXT@ \
-		notify_test@EXEEXT@
+		notify_test@EXEEXT@ \
+		query_test
 
 @BIND9_MAKE_RULES@