improve hook processing macros

- use a get_hooktab() function to determine the hook table.
- PROCESS_HOOK now jumps to a cleanup tag on failure
- add PROCESS_ALL_HOOKS in query.c, to run all hook functions at
  a specified hook point without stopping. this is to be used for
  intiialization and destruction functions that must run in every
  module.
- 'result' is set in PROCESS_HOOK only when a hook function
  interrupts processing.
- revised terminology: a "callback" is now a "hook action"
- remove unused NS_PROCESS_HOOK and NS_PROCESS_HOOK_VOID macros.

bin/hooks/filter-aaaa.c

@@ -68,37 +68,37 @@ static isc_mempool_t *datapool = NULL;
 static bool
 filter_qctx_initialize(void *hookdata, void *cbdata, isc_result_t *resp);
 static ns_hook_t filter_init = {
-	.callback = filter_qctx_initialize,
+	.action = filter_qctx_initialize,
 };
 
 static bool
 filter_respond_begin(void *hookdata, void *cbdata, isc_result_t *resp);
 static ns_hook_t filter_respbegin = {
-	.callback = filter_respond_begin,
+	.action = filter_respond_begin,
 };
 
 static bool
 filter_respond_any_found(void *hookdata, void *cbdata, isc_result_t *resp);
 static ns_hook_t filter_respanyfound = {
-	.callback = filter_respond_any_found,
+	.action = filter_respond_any_found,
 };
 
 static bool
 filter_prep_response_begin(void *hookdata, void *cbdata, isc_result_t *resp);
 static ns_hook_t filter_prepresp = {
-	.callback = filter_prep_response_begin,
+	.action = filter_prep_response_begin,
 };
 
 static bool
 filter_query_done_send(void *hookdata, void *cbdata, isc_result_t *resp);
 static ns_hook_t filter_donesend = {
-	.callback = filter_query_done_send,
+	.action = filter_query_done_send,
 };
 
 static bool
 filter_qctx_destroy(void *hookdata, void *cbdata, isc_result_t *resp);
 ns_hook_t filter_destroy = {
-	.callback = filter_qctx_destroy,
+	.action = filter_qctx_destroy,
 };
 
 /**

lib/ns/include/ns/hooks.h

@@ -165,6 +165,7 @@
 typedef enum {
 	NS_QUERY_QCTX_INITIALIZED,
 	NS_QUERY_QCTX_DESTROYED,
+	NS_QUERY_SETUP,
 	NS_QUERY_START_BEGIN,
 	NS_QUERY_LOOKUP_BEGIN,
 	NS_QUERY_RESUME_BEGIN,
@@ -189,11 +190,11 @@ typedef enum {
 } ns_hookpoint_t;
 
 typedef bool
-(*ns_hook_cb_t)(void *hook_data, void *callback_data, isc_result_t *resultp);
+(*ns_hook_action_t)(void *arg, void *data, isc_result_t *resultp);
 
 typedef struct ns_hook {
-	ns_hook_cb_t callback;
+	ns_hook_action_t action;
-	void *callback_data;
+	void *action_data;
 	ISC_LINK(struct ns_hook) link;
 } ns_hook_t;
 
@@ -278,35 +279,6 @@ typedef int ns_hook_version_t(unsigned int *flags);
  * the future to pass back driver capabilities or other information.
  */
 
-/*
- * Run a hook. Calls the function or functions registered at hookpoint 'id'.
- * If one of them returns true, we interrupt processing and return the
- * result that was returned by the hook function. If none of them return
- * true, we continue processing.
- */
-#define _NS_PROCESS_HOOK(table, id, data, ...)				\
-	if (table != NULL) {						\
-		ns_hook_t *_hook = ISC_LIST_HEAD((*table)[id]);		\
-		isc_result_t _result;					\
-									\
-		while (_hook != NULL) {					\
-			ns_hook_cb_t _callback = _hook->callback;	\
-			void *_callback_data = _hook->callback_data;	\
-			if (_callback != NULL &&			\
-			    _callback(data, _callback_data, &_result))	\
-			{						\
-				return __VA_ARGS__;			\
-			} else {					\
-				_hook = ISC_LIST_NEXT(_hook, link);	\
-			}						\
-		}							\
-	}
-
-#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)
-
 isc_result_t
 ns_hook_createctx(isc_mem_t *mctx, ns_hookctx_t **hctxp);
 

lib/ns/query.c

@@ -238,30 +238,73 @@ static void
 log_noexistnodata(void *val, int level, const char *fmt, ...)
 	ISC_FORMAT_PRINTF(3, 4);
 
-#define PROCESS_HOOK(_id, _qctx, ...)					\
+
+/*
+ * Return the hooktable in use with 'qctx', or if there isn't one
+ * set, return the default hooktable.
+ */
+static inline ns_hooktable_t *
+get_hooktab(query_ctx_t *qctx) {
+	if (qctx == NULL || qctx->view == NULL ||
+	    qctx->view->hooktable == NULL)
+	{
+		return (ns__hook_table);
+	}
+
+	return (qctx->view->hooktable);
+}
+
+/*
+ * Call the specified hook function in every configured module that
+ * implements that function. If any hook function returns 'true', we set
+ * 'result' and terminate processing by jumping to the 'cleanup' tag.
+ *
+ * (Note that a hook function may set the 'result' to ISC_R_SUCCESS but
+ * still terminate processing within the calling function. That's why this
+ * is a macro instead of an inline function; it needs to be able to use
+ * 'goto cleanup' regardless of the return value.)
+ */
+#define PROCESS_HOOK(_id, _qctx)					\
 	do {								\
-		ns_hooktable_t *_tab = ns__hook_table; 			\
+		isc_result_t _res;					\
-		query_ctx_t *_q = (_qctx);				\
+		ns_hooktable_t *_tab = get_hooktab(_qctx);		\
-		if (_q != NULL &&					\
+		ns_hook_t *_hook;					\
-		    _q->view != NULL &&					\
+		_hook = ISC_LIST_HEAD((*_tab)[_id]);			\
-		    _q->view->hooktable != NULL)			\
+		while (_hook != NULL) {					\
-		{							\
+			ns_hook_action_t _func = _hook->action;		\
-			_tab = _q->view->hooktable;			\
+			void *_data = _hook->action_data;		\
+			INSIST(_func != NULL);				\
+			if (_func(_qctx, _data, &_res)) {		\
+				result = _res;				\
+				goto cleanup;				\
+			} else {					\
+				_hook = ISC_LIST_NEXT(_hook, link);	\
+			}						\
 		}							\
-		NS_PROCESS_HOOK(_tab, _id, _q, __VA_ARGS__);		\
 	} while (false)
 
-#define PROCESS_HOOK_VOID(_id, _qctx, ...)				\
+/*
+ * Call the specified hook function in every configured module that
+ * implements that function. All modules are called; hook function return
+ * codes are ignored. This is intended for use with initialization and
+ * destruction calls which *must* run in every configured module.
+ *
+ * (This could be implemented as an inline void function, but is left as a
+ * macro for symmetry with PROCESS_HOOK above.)
+ */
+#define PROCESS_ALL_HOOKS(_id, _qctx)					\
 	do {								\
-		ns_hooktable_t *_tab = ns__hook_table; 			\
+		isc_result_t _res;					\
-		query_ctx_t *_q = (_qctx);				\
+		ns_hooktable_t *_tab = get_hooktab(_qctx);		\
-		if (_q != NULL &&					\
+		ns_hook_t *_hook;					\
-		    _q->view != NULL &&					\
+		_hook = ISC_LIST_HEAD((*_tab)[_id]);			\
-		    _q->view->hooktable != NULL)			\
+		while (_hook != NULL) {					\
-		{							\
+			ns_hook_action_t _func = _hook->action;		\
-			_tab = _q->view->hooktable;			\
+			void *_data = _hook->action_data;		\
+			INSIST(_func != NULL);				\
+			_func(_qctx, _data, &_res);			\
+			_hook = ISC_LIST_NEXT(_hook, link);		\
 		}							\
-		NS_PROCESS_HOOK_VOID(_tab, _id, _q, __VA_ARGS__);	\
 	} while (false)
 
 /*
@@ -1908,6 +1951,7 @@ query_setorder(query_ctx_t *qctx, dns_name_t *name, dns_rdataset_t *rdataset) {
 static void
 query_additional(query_ctx_t *qctx, dns_rdataset_t *rdataset) {
 	ns_client_t *client = qctx->client;
+	isc_result_t result;
 
 	CTRACE(ISC_LOG_DEBUG(3), "query_additional");
 
@@ -1923,7 +1967,6 @@ query_additional(query_ctx_t *qctx, dns_rdataset_t *rdataset) {
 	    (client->query.gluedb != NULL) &&
 	    dns_db_iszone(client->query.gluedb))
 	{
-		isc_result_t result;
 		ns_dbversion_t *dbversion;
 
 		dbversion = ns_client_findversion(client, client->query.gluedb);
@@ -1938,7 +1981,7 @@ query_additional(query_ctx_t *qctx, dns_rdataset_t *rdataset) {
 		}
 	}
 
-regular:
+ regular:
 	/*
 	 * Add other additional data if needed.
 	 * We don't care if dns_rdataset_additionaldata() fails.
@@ -4848,7 +4891,7 @@ qctx_init(ns_client_t *client, dns_fetchevent_t *event,
 	qctx->answer_has_ns = false;
 	qctx->authoritative = false;
 
-	PROCESS_HOOK_VOID(NS_QUERY_QCTX_INITIALIZED, qctx);
+	PROCESS_ALL_HOOKS(NS_QUERY_QCTX_INITIALIZED, qctx);
 }
 
 /*%
@@ -4913,7 +4956,8 @@ qctx_freedata(query_ctx_t *qctx) {
 
 static void
 qctx_destroy(query_ctx_t *qctx) {
-	PROCESS_HOOK_VOID(NS_QUERY_QCTX_DESTROYED, qctx);
+	PROCESS_ALL_HOOKS(NS_QUERY_QCTX_DESTROYED, qctx);
+
 	dns_view_detach(&qctx->view);
 }
 
@@ -4967,6 +5011,8 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) {
 	qctx_init(client, NULL, qtype, &qctx);
 	query_trace(&qctx);
 
+	PROCESS_HOOK(NS_QUERY_SETUP, &qctx);
+
 	/*
 	 * If it's a SIG query, we'll iterate the node.
 	 */
@@ -4986,6 +5032,8 @@ query_setup(ns_client_t *client, dns_rdatatype_t qtype) {
 	}
 
 	result = ns__query_start(&qctx);
+
+ cleanup:
 	qctx_destroy(&qctx);
 	return (result);
 }
@@ -5263,6 +5311,9 @@ ns__query_start(query_ctx_t *qctx) {
 	}
 
 	return (query_lookup(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -5386,6 +5437,9 @@ query_lookup(query_ctx_t *qctx) {
 	}
 
 	return (query_gotanswer(qctx, result));
+
+ cleanup:
+	return (result);
 }
 
 /*
@@ -5889,6 +5943,9 @@ query_resume(query_ctx_t *qctx) {
 	qctx->resuming = true;
 
 	return (query_gotanswer(qctx, result));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -6524,7 +6581,8 @@ query_usestale(query_ctx_t *qctx) {
  * result from the search.
  */
 static isc_result_t
-query_gotanswer(query_ctx_t *qctx, isc_result_t result) {
+query_gotanswer(query_ctx_t *qctx, isc_result_t res) {
+	isc_result_t result = res;
 	char errmsg[256];
 
 	CCTRACE(ISC_LOG_DEBUG(3), "query_gotanswer");
@@ -6624,6 +6682,9 @@ query_gotanswer(query_ctx_t *qctx, isc_result_t result) {
 		QUERY_ERROR(qctx, result);
 		return (ns_query_done(qctx));
 	}
+
+ cleanup:
+	return (result);
 }
 
 static void
@@ -6922,6 +6983,9 @@ query_respond_any(query_ctx_t *qctx) {
 	query_addauth(qctx);
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*
@@ -7139,6 +7203,9 @@ query_respond(query_ctx_t *qctx) {
 	query_addauth(qctx);
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 static isc_result_t
@@ -7555,6 +7622,9 @@ query_notfound(query_ctx_t *qctx) {
 	}
 
 	return (query_delegation(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -7563,6 +7633,7 @@ query_notfound(query_ctx_t *qctx) {
  */
 static isc_result_t
 query_prepare_delegation_response(query_ctx_t *qctx) {
+	isc_result_t result;
 	dns_rdataset_t **sigrdatasetp = NULL;
 	bool detach = false;
 
@@ -7605,6 +7676,9 @@ query_prepare_delegation_response(query_ctx_t *qctx) {
 	query_addds(qctx);
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -7698,6 +7772,9 @@ query_zone_delegation(query_ctx_t *qctx) {
 	}
 
 	return (query_prepare_delegation_response(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -7823,6 +7900,9 @@ query_delegation(query_ctx_t *qctx) {
 	}
 
 	return (query_prepare_delegation_response(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -7963,7 +8043,9 @@ query_addds(query_ctx_t *qctx) {
  * Handle authoritative NOERROR/NODATA responses.
  */
 static isc_result_t
-query_nodata(query_ctx_t *qctx, isc_result_t result) {
+query_nodata(query_ctx_t *qctx, isc_result_t res) {
+	isc_result_t result = res;
+
 	PROCESS_HOOK(NS_QUERY_NODATA_BEGIN, qctx);
 
 #ifdef dns64_bis_return_excluded_addresses
@@ -8074,6 +8156,9 @@ query_nodata(query_ctx_t *qctx, isc_result_t result) {
 	}
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -8347,6 +8432,9 @@ query_nxdomain(query_ctx_t *qctx, bool empty_wild) {
 		qctx->client->message->rcode = dns_rcode_nxdomain;
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*
@@ -9282,6 +9370,9 @@ query_cname(query_ctx_t *qctx) {
 	query_addauth(qctx);
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*
@@ -9430,6 +9521,9 @@ query_dname(query_ctx_t *qctx) {
 	query_addauth(qctx);
 
 	return (ns_query_done(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -9511,6 +9605,8 @@ query_addcname(query_ctx_t *qctx, dns_trust_t trust, dns_ttl_t ttl) {
  */
 static isc_result_t
 query_prepresponse(query_ctx_t *qctx) {
+	isc_result_t result = ISC_R_SUCCESS;
+
 	PROCESS_HOOK(NS_QUERY_PREP_RESPONSE_BEGIN, qctx);
 
 	if (WANTDNSSEC(qctx->client) &&
@@ -9524,9 +9620,12 @@ query_prepresponse(query_ctx_t *qctx) {
 
 	if (qctx->type == dns_rdatatype_any) {
 		return (query_respond_any(qctx));
-	} else {
-		return (query_respond(qctx));
 	}
+
+	return (query_respond(qctx));
+
+ cleanup:
+	return (result);
 }
 
 /*%
@@ -10396,6 +10495,7 @@ query_glueanswer(query_ctx_t *qctx) {
 
 isc_result_t
 ns_query_done(query_ctx_t *qctx) {
+	isc_result_t result;
 	const dns_namelist_t *secs = qctx->client->message->sections;
 
 	CCTRACE(ISC_LOG_DEBUG(3), "ns_query_done");
@@ -10501,6 +10601,9 @@ ns_query_done(query_ctx_t *qctx) {
 
 	ns_client_detach(&qctx->client);
 	return (qctx->result);
+
+ cleanup:
+	return (result);
 }
 
 static inline void

lib/ns/tests/nstest.c

@@ -629,17 +629,17 @@ destroy_message:
 }
 
 /*%
- * A hook callback which stores the query context pointed to by "hook_data" at
+ * A hook action which stores the query context pointed to by "arg" at
- * "callback_data".  Causes execution to be interrupted at hook insertion
+ * "data".  Causes execution to be interrupted at hook insertion
  * point.
  */
 static bool
-extract_qctx(void *hook_data, void *callback_data, isc_result_t *resultp) {
+extract_qctx(void *arg, void *data, isc_result_t *resultp) {
 	query_ctx_t **qctxp;
 	query_ctx_t *qctx;
 
-	REQUIRE(hook_data != NULL);
+	REQUIRE(arg != NULL);
-	REQUIRE(callback_data != NULL);
+	REQUIRE(data != NULL);
 	REQUIRE(resultp != NULL);
 
 	/*
@@ -649,10 +649,10 @@ extract_qctx(void *hook_data, void *callback_data, isc_result_t *resultp) {
 	 */
 	qctx = isc_mem_get(mctx, sizeof(*qctx));
 	if (qctx != NULL) {
-		memmove(qctx, (query_ctx_t *)hook_data, sizeof(*qctx));
+		memmove(qctx, (query_ctx_t *)arg, sizeof(*qctx));
 	}
 
-	qctxp = (query_ctx_t **)callback_data;
+	qctxp = (query_ctx_t **)data;
 	/*
 	 * If memory allocation failed, the supplied pointer will simply be set
 	 * to NULL.  We rely on the user of this hook to react properly.
@@ -674,8 +674,8 @@ static isc_result_t
 create_qctx_for_client(ns_client_t *client, query_ctx_t **qctxp) {
 	ns_hooktable_t *saved_hook_table, query_hooks;
 	ns_hook_t hook = {
-		.callback = extract_qctx,
+		.action = extract_qctx,
-		.callback_data = qctxp,
+		.action_data = qctxp,
 	};
 
 	REQUIRE(client != NULL);
@@ -691,7 +691,7 @@ create_qctx_for_client(ns_client_t *client, query_ctx_t **qctxp) {
 	 */
 
 	ns_hooktable_init(&query_hooks);
-	ns_hook_add(&query_hooks, NS_QUERY_QCTX_INITIALIZED, &hook);
+	ns_hook_add(&query_hooks, NS_QUERY_SETUP, &hook);
 
 	saved_hook_table = ns__hook_table;
 	ns__hook_table = &query_hooks;
@@ -801,11 +801,10 @@ ns_test_qctx_destroy(query_ctx_t **qctxp) {
 }
 
 bool
-ns_test_hook_catch_call(void *hook_data, void *callback_data,
+ns_test_hook_catch_call(void *arg, void *data, isc_result_t *resultp)
-			isc_result_t *resultp)
 {
-	UNUSED(hook_data);
+	UNUSED(arg);
-	UNUSED(callback_data);
+	UNUSED(data);
 
 	*resultp = ISC_R_UNSET;
 

lib/ns/tests/nstest.h

@@ -151,5 +151,4 @@ ns_test_qctx_destroy(query_ctx_t **qctxp);
  * A hook callback interrupting execution at given hook's insertion point.
  */
 bool
-ns_test_hook_catch_call(void *hook_data, void *callback_data,
+ns_test_hook_catch_call(void *arg, void *data, isc_result_t *resultp);
-			isc_result_t *resultp);

lib/ns/tests/query_test.c

@@ -85,7 +85,7 @@ run_sfcache_test(const ns__query_sfcache_test_params_t *test) {
 	query_ctx_t *qctx = NULL;
 	isc_result_t result;
 	ns_hook_t hook = {
-		.callback = ns_test_hook_catch_call,
+		.action = ns_test_hook_catch_call,
 	};
 
 	REQUIRE(test != NULL);
@@ -282,7 +282,7 @@ run_start_test(const ns__query_start_test_params_t *test) {
 	query_ctx_t *qctx = NULL;
 	isc_result_t result;
 	ns_hook_t hook = {
-		.callback = ns_test_hook_catch_call,
+		.action = ns_test_hook_catch_call,
 	};
 
 	REQUIRE(test != NULL);