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.

diff --git a/bin/hooks/filter-aaaa.c b/bin/hooks/filter-aaaa.c
index fa964fab6997eb47c6b15972ccade6e5e22d3bc6..174725eac26d525895a3736fb76508a88c027fb2 100644 (file)
--- a/bin/hooks/filter-aaaa.c
+++ b/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,
 };
 
 /**

diff --git a/lib/ns/include/ns/hooks.h b/lib/ns/include/ns/hooks.h
index 95f9aca1838efe98d3ad539060ca2f3d961321b0..760292f0cc307d521ee431f386d3c93acf552236 100644 (file)
--- a/lib/ns/include/ns/hooks.h
+++ b/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;
-       void *callback_data;
+       ns_hook_action_t action;
+       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);
 

diff --git a/lib/ns/query.c b/lib/ns/query.c
index dcfa5cc3688d8f2abaeaea7800bf35bb4cd98254..5de1f2b1f20230b66157006aa2bc0a9aaed75ff6 100644 (file)
--- a/lib/ns/query.c
+++ b/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;                  \
-               query_ctx_t *_q = (_qctx);                              \
-               if (_q != NULL &&                                       \
-                   _q->view != NULL &&                                 \
-                   _q->view->hooktable != NULL)                        \
-               {                                                       \
-                       _tab = _q->view->hooktable;                     \
+               isc_result_t _res;                                      \
+               ns_hooktable_t *_tab = get_hooktab(_qctx);              \
+               ns_hook_t *_hook;                                       \
+               _hook = ISC_LIST_HEAD((*_tab)[_id]);                    \
+               while (_hook != NULL) {                                 \
+                       ns_hook_action_t _func = _hook->action;         \
+                       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;                  \
-               query_ctx_t *_q = (_qctx);                              \
-               if (_q != NULL &&                                       \
-                   _q->view != NULL &&                                 \
-                   _q->view->hooktable != NULL)                        \
-               {                                                       \
-                       _tab = _q->view->hooktable;                     \
+               isc_result_t _res;                                      \
+               ns_hooktable_t *_tab = get_hooktab(_qctx);              \
+               ns_hook_t *_hook;                                       \
+               _hook = ISC_LIST_HEAD((*_tab)[_id]);                    \
+               while (_hook != NULL) {                                 \
+                       ns_hook_action_t _func = _hook->action;         \
+                       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

diff --git a/lib/ns/tests/nstest.c b/lib/ns/tests/nstest.c
index 355d1758650fcce55cba6678519c96424d0a30e7..2532042e82f9ab5a7823fcce98e4a6df0e66d3bc 100644 (file)
--- a/lib/ns/tests/nstest.c
+++ b/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
- * "callback_data".  Causes execution to be interrupted at hook insertion
+ * A hook action which stores the query context pointed to by "arg" at
+ * "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(callback_data != NULL);
+       REQUIRE(arg != 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,
-               .callback_data = qctxp,
+               .action = extract_qctx,
+               .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,
-                       isc_result_t *resultp)
+ns_test_hook_catch_call(void *arg, void *data, isc_result_t *resultp)
 {
-       UNUSED(hook_data);
-       UNUSED(callback_data);
+       UNUSED(arg);
+       UNUSED(data);
 
        *resultp = ISC_R_UNSET;
 

diff --git a/lib/ns/tests/nstest.h b/lib/ns/tests/nstest.h
index bc04487046334ebda7a5f20bd34056dd14a759a8..fc69befdc695f00180b9dd60f9926a4c787b3b42 100644 (file)
--- a/lib/ns/tests/nstest.h
+++ b/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,
-                       isc_result_t *resultp);
+ns_test_hook_catch_call(void *arg, void *data, isc_result_t *resultp);

diff --git a/lib/ns/tests/query_test.c b/lib/ns/tests/query_test.c
index f8f59a23b3c23892c601e67e6f4a36d73d2ce89e..4b7002ae9cf2e69c6b20c09d8d1033bb5a252cc1 100644 (file)
--- a/lib/ns/tests/query_test.c
+++ b/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);