From 8a3a216f40cf3befc68d60851c1635e2cfa31cfb Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Thu, 9 Feb 2023 14:37:43 +0000 Subject: [PATCH 1/6] Support for iterating over the leaves in a qp-trie The iterator object records a path through the trie, in a similar manner to the existing dns_rbtnodechain. --- lib/dns/include/dns/qp.h | 78 ++++++++++++++++++++++++++++++----- lib/dns/qp.c | 66 ++++++++++++++++++++++++++++++ lib/dns/qp_p.h | 6 ++- tests/dns/qp_test.c | 88 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 226 insertions(+), 12 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 5e9a1f25a1..5befed68ca 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -154,10 +154,7 @@ typedef union dns_qpreadable { #define dns_qpreader(qpr) ((qpr).qp) /*% - * A trie lookup key is a small array, allocated on the stack during trie - * searches. Keys are usually created on demand from DNS names using - * `dns_qpkey_fromname()`, but in principle you can define your own - * functions to convert other types to trie lookup keys. + * The maximum size of a key is also the maximum depth of a trie. * * A domain name can be up to 255 bytes. When converted to a key, each * character in the name corresponds to one byte in the key if it is a @@ -165,7 +162,29 @@ typedef union dns_qpreadable { * using two bytes in the key. So we allow keys to be up to 512 bytes. * (The actual max is (255 - 5) * 2 + 6 == 506) */ -typedef uint8_t dns_qpkey_t[512]; +#define DNS_QP_MAXKEY 512 + +/*% + * A trie lookup key is a small array, allocated on the stack during trie + * searches. Keys are usually created on demand from DNS names using + * `dns_qpkey_fromname()`, but in principle you can define your own + * functions to convert other types to trie lookup keys. + */ +typedef uint8_t dns_qpkey_t[DNS_QP_MAXKEY]; + +/*% + * A trie iterator describes a path through the trie from the root to + * a leaf node, for use with `dns_qpiter_init()` and `dns_qpiter_next()`. + */ +typedef struct dns_qpiter { + unsigned int magic; + dns_qpreader_t *qp; + uint16_t sp; + struct __attribute__((__packed__)) { + uint32_t ref; + uint8_t more; + } stack[DNS_QP_MAXKEY]; +} dns_qpiter_t; /*% * These leaf methods allow the qp-trie code to call back to the code @@ -376,16 +395,13 @@ dns_qpmulti_memusage(dns_qpmulti_t *multi); /* * XXXFANF todo, based on what we discover BIND needs * - * fancy searches: longest match, lexicographic predecessor, - * etc. + * fancy searches: longest match, lexicographic predecessor + * (for NSEC), successor (for modification-safe iteration), etc. * * do we need specific lookup functions to find out if the * returned value is readonly or mutable? * * richer modification such as dns_qp_replace{key,name} - * - * iteration - probably best to put an explicit stack in the iterator, - * cf. rbtnodechain */ size_t @@ -484,6 +500,48 @@ dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name); * \li ISC_R_SUCCESS if the leaf was deleted from the trie */ +void +dns_qpiter_init(dns_qpreadable_t qpr, dns_qpiter_t *qpi); +/*%< + * Initialize an iterator + * + * SAFETY NOTE: If `qpr` is a `dns_qp_t`, it is not safe to modify the + * trie during iteration. If `qpr` is a `dns_qpread_t` or `dns_qpsnap_t` + * then (like any other read-only access) modifications will not affect + * iteration. + * + * Requires: + * \li `qp` is a pointer to a valid qp-trie + * \li `qpi` is a pointer to a qp iterator + */ + +isc_result_t +dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r); +/*%< + * Get the next leaf object of a trie in lexicographic order of its keys. + * + * NOTE: see the safety note under `dns_qpiter_init()`. + * + * For example, + * + * dns_qpiter_t qpi; + * void *pval; + * uint32_t ival; + * dns_qpiter_init(qp, &qpi); + * while (dns_qpiter_next(&qpi, &pval, &ival)) { + * // do something with pval and ival + * } + * + * Requires: + * \li `qpi` is a pointer to a valid qp iterator + * \li `pval_r != NULL` + * \li `ival_r != NULL` + * + * Returns: + * \li ISC_R_SUCCESS if a leaf was found and pval_r and ival_r were set + * \li ISC_R_NOMORE otherwise + */ + /*********************************************************************** * * functions - transactions diff --git a/lib/dns/qp.c b/lib/dns/qp.c index e990a61e3f..935dba21cf 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1681,6 +1681,72 @@ dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name) { return (dns_qp_deletekey(qp, key, keylen)); } +/*********************************************************************** + * + * iterate + */ + +void +dns_qpiter_init(dns_qpreadable_t qpr, dns_qpiter_t *qpi) { + dns_qpreader_t *qp = dns_qpreader(qpr); + REQUIRE(QP_VALID(qp)); + REQUIRE(qpi != NULL); + qpi->magic = QPITER_MAGIC; + qpi->qp = qp; + qpi->sp = 0; + qpi->stack[qpi->sp].ref = qp->root_ref; + qpi->stack[qpi->sp].more = 0; +} + +/* + * note: this function can go wrong when the iterator refers to + * a mutable view of the trie which is altered while iterating + */ +isc_result_t +dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r) { + REQUIRE(QPITER_VALID(qpi)); + REQUIRE(QP_VALID(qpi->qp)); + REQUIRE(pval_r != NULL); + REQUIRE(ival_r != NULL); + + dns_qpreader_t *qp = qpi->qp; + + if (qpi->stack[qpi->sp].ref == INVALID_REF) { + INSIST(qpi->sp == 0); + qpi->magic = 0; + return (ISC_R_NOMORE); + } + + /* push branch nodes onto the stack until we reach a leaf */ + for (;;) { + qp_node_t *n = ref_ptr(qp, qpi->stack[qpi->sp].ref); + if (node_tag(n) == LEAF_TAG) { + *pval_r = leaf_pval(n); + *ival_r = leaf_ival(n); + break; + } + qpi->sp++; + INSIST(qpi->sp < DNS_QP_MAXKEY); + qpi->stack[qpi->sp].ref = branch_twigs_ref(n); + qpi->stack[qpi->sp].more = branch_twigs_size(n) - 1; + } + + /* pop the stack until we find a twig with a successor */ + while (qpi->sp > 0 && qpi->stack[qpi->sp].more == 0) { + qpi->sp--; + } + + /* move across to the next twig */ + if (qpi->stack[qpi->sp].more > 0) { + qpi->stack[qpi->sp].more--; + qpi->stack[qpi->sp].ref++; + } else { + INSIST(qpi->sp == 0); + qpi->stack[qpi->sp].ref = INVALID_REF; + } + return (ISC_R_SUCCESS); +} + /*********************************************************************** * * search diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index 183335e4e4..12029e72c2 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -379,11 +379,13 @@ ref_ptr(dns_qpreadable_t qpr, qp_ref_t ref) { */ #define QP_MAGIC ISC_MAGIC('t', 'r', 'i', 'e') +#define QPITER_MAGIC ISC_MAGIC('q', 'p', 'i', 't') #define QPMULTI_MAGIC ISC_MAGIC('q', 'p', 'm', 'v') #define QPREADER_MAGIC ISC_MAGIC('q', 'p', 'r', 'x') -#define QP_VALID(qp) ISC_MAGIC_VALID(qp, QP_MAGIC) -#define QPMULTI_VALID(qp) ISC_MAGIC_VALID(qp, QPMULTI_MAGIC) +#define QP_VALID(p) ISC_MAGIC_VALID(p, QP_MAGIC) +#define QPITER_VALID(p) ISC_MAGIC_VALID(p, QPITER_MAGIC) +#define QPMULTI_VALID(p) ISC_MAGIC_VALID(p, QPMULTI_MAGIC) /* * Polymorphic initialization of the `dns_qpreader_t` prefix. diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index aceaa344f8..3b096d28db 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -22,6 +22,9 @@ #define UNIT_TESTING #include +#include +#include +#include #include #include #include @@ -29,6 +32,8 @@ #include #include +#include "qp_p.h" + #include #include @@ -129,9 +134,92 @@ ISC_RUN_TEST_IMPL(qpkey_sort) { } } +#define ITER_ITEMS 100 + +static uint32_t +check_leaf(void *uctx, void *pval, uint32_t ival) { + uint32_t *items = uctx; + assert_in_range(ival, 1, ITER_ITEMS - 1); + assert_ptr_equal(items + ival, pval); + return (1); +} + +static size_t +qpiter_makekey(dns_qpkey_t key, void *uctx, void *pval, uint32_t ival) { + check_leaf(uctx, pval, ival); + + char str[8]; + snprintf(str, sizeof(str), "%03u", ival); + + size_t i = 0; + while (str[i] != '\0') { + key[i] = str[i] - '0' + SHIFT_BITMAP; + i++; + } + key[i++] = SHIFT_NOBYTE; + + return (i); +} + +static void +getname(void *uctx, char *buf, size_t size) { + strlcpy(buf, "test", size); + UNUSED(uctx); + UNUSED(size); +} + +const struct dns_qpmethods qpiter_methods = { + check_leaf, + check_leaf, + qpiter_makekey, + getname, +}; + +ISC_RUN_TEST_IMPL(qpiter) { + dns_qp_t *qp = NULL; + uint32_t item[ITER_ITEMS] = { 0 }; + + dns_qp_create(mctx, &qpiter_methods, item, &qp); + for (size_t tests = 0; tests < 1234; tests++) { + uint32_t ival = isc_random_uniform(ITER_ITEMS - 1) + 1; + void *pval = &item[ival]; + item[ival] = ival; + + /* randomly insert or remove */ + dns_qpkey_t key; + size_t len = qpiter_makekey(key, item, pval, ival); + if (dns_qp_insert(qp, pval, ival) == ISC_R_EXISTS) { + dns_qp_deletekey(qp, key, len); + item[ival] = 0; + } + + /* check that we see only valid items in the correct order */ + uint32_t prev = 0; + dns_qpiter_t qpi; + dns_qpiter_init(qp, &qpi); + while (dns_qpiter_next(&qpi, &pval, &ival) == ISC_R_SUCCESS) { + assert_in_range(ival, prev + 1, ITER_ITEMS - 1); + assert_int_equal(ival, item[ival]); + assert_ptr_equal(pval, &item[ival]); + item[ival] = ~ival; + prev = ival; + } + + /* ensure we saw every item */ + for (ival = 0; ival < ITER_ITEMS; ival++) { + if (item[ival] != 0) { + assert_int_equal(item[ival], ~ival); + item[ival] = ival; + } + } + } + dns_qp_destroy(&qp); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY(qpkey_name) ISC_TEST_ENTRY(qpkey_sort) +ISC_TEST_ENTRY(qpiter) ISC_TEST_LIST_END ISC_TEST_MAIN From fa1b57ee6e703bf39d1ebab1b6d99e0fa4bfb32b Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 10 Feb 2023 16:53:31 +0000 Subject: [PATCH 2/6] Support for finding the longest parent domain in a qp-trie This is the first of the "fancy" searches that know how the DNS namespace maps on to the structure of a qp-trie. For example, it will find the closest enclosing zone in the zone tree. --- lib/dns/include/dns/qp.h | 41 +++++++++++- lib/dns/qp.c | 116 ++++++++++++++++++++++++++++++++ tests/dns/qp_test.c | 140 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 294 insertions(+), 3 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 5befed68ca..a860c5a901 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -19,7 +19,11 @@ * * Keys are `dns_qpkey_t`, which is a string-like thing, usually created * from a DNS name. You can use both relative and absolute DNS names as - * keys. + * keys, even in the same trie, except for one caveat: if a trie contains + * names relative to the zone apex, the natural way to represent the apex + * itself (spelled `@` in zone files) is a zero-length name; but a + * zero-length name has the same qpkey representation as the root zone + * (apart from its length), so they collide. * * Leaf values are a pair of a `void *` pointer and a `uint32_t` * (because that is what fits inside an internal qp-trie leaf node). @@ -259,6 +263,13 @@ typedef enum dns_qpgc { DNS_QPGC_ALL, } dns_qpgc_t; +/*% + * Options for fancy searches such as `dns_qp_findname_parent()` + */ +typedef enum dns_qpfind { + DNS_QPFIND_NOEXACT = 1 << 0, +} dns_qpfind_t; + /*********************************************************************** * * functions - create, destory, enquire @@ -395,8 +406,8 @@ dns_qpmulti_memusage(dns_qpmulti_t *multi); /* * XXXFANF todo, based on what we discover BIND needs * - * fancy searches: longest match, lexicographic predecessor - * (for NSEC), successor (for modification-safe iteration), etc. + * more fancy searches: lexicographic predecessor (for NSEC), + * successor (for modification-safe iteration), etc. * * do we need specific lookup functions to find out if the * returned value is readonly or mutable? @@ -457,6 +468,30 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, * \li ISC_R_SUCCESS if the leaf was found */ +isc_result_t +dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name, + dns_qpfind_t options, void **pval_r, uint32_t *ival_r); +/*%< + * Find a leaf in a qp-trie that is a parent domain of or equal to the + * given DNS name. + * + * If the DNS_QPFIND_NOEXACT option is set, find a strict parent + * domain not equal to the search name. + * + * The leaf values are assigned to `*pval_r` and `*ival_r` + * + * Requires: + * \li `qpr` is a pointer to a readable qp-trie + * \li `name` is a pointer to a valid `dns_name_t` + * \li `pval_r != NULL` + * \li `ival_r != NULL` + * + * Returns: + * \li ISC_R_SUCCESS if an exact match was found + * \li ISC_R_PARTIALMATCH if a parent domain was found + * \li ISC_R_NOTFOUND if no match was found + */ + isc_result_t dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival); /*%< diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 935dba21cf..c6bfa74953 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -278,6 +278,25 @@ qpkey_compare(const dns_qpkey_t key_a, const size_t keylen_a, return (QPKEY_EQUAL); } +/* + * Given a key constructed by dns_qpkey_fromname(), trim it down to the last + * label boundary before the `max` length. + * + * This is used when searching a trie for the best match for a name. + */ +static size_t +qpkey_trim_label(dns_qpkey_t key, size_t len, size_t max) { + size_t stop = 0; + for (size_t offset = 0; offset < max; offset++) { + if (qpkey_bit(key, len, offset) == SHIFT_NOBYTE && + qpkey_bit(key, len, offset + 1) != SHIFT_NOBYTE) + { + stop = offset + 1; + } + } + return (stop); +} + /*********************************************************************** * * allocator wrappers @@ -1800,4 +1819,101 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, return (dns_qp_getkey(qpr, key, keylen, pval_r, ival_r)); } +isc_result_t +dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name, + dns_qpfind_t options, void **pval_r, uint32_t *ival_r) { + dns_qpreader_t *qp = dns_qpreader(qpr); + dns_qpkey_t search, found; + size_t searchlen, foundlen; + size_t offset; + qp_shift_t bit; + qp_node_t *n, *twigs; + isc_result_t result; + unsigned int labels = 0; + struct offref { + uint32_t off; + qp_ref_t ref; + } label[DNS_NAME_MAXLABELS]; + + REQUIRE(QP_VALID(qp)); + REQUIRE(pval_r != NULL); + REQUIRE(ival_r != NULL); + + searchlen = dns_qpkey_fromname(search, name); + if ((options & DNS_QPFIND_NOEXACT) != 0) { + searchlen = qpkey_trim_label(search, searchlen, searchlen); + result = DNS_R_PARTIALMATCH; + } else { + result = ISC_R_SUCCESS; + } + + n = get_root(qp); + if (n == NULL) { + return (ISC_R_NOTFOUND); + } + + /* + * Like `dns_qp_insert()`, we must find a leaf. However, we don't make a + * second pass: instead, we keep track of any leaves with shorter keys + * that we discover along the way. (In general, qp-trie searches can be + * one-pass, by recording their traversal, or two-pass, for less stack + * memory usage.) + * + * A shorter key that can be a parent domain always has a leaf node at + * SHIFT_NOBYTE (indicating end of its key) where our search key has a + * normal character immediately after a label separator. Note 1: It is + * OK if `offset - 1` underflows: it will become SIZE_MAX, which is + * greater than `searchlen`, so `qpkey_bit()` will return SHIFT_NOBYTE, + * which is what we want when `offset == 0`. Note 2: Any SHIFT_NOBYTE + * twig is always `twigs[0]`. + */ + while (is_branch(n)) { + prefetch_twigs(qp, n); + twigs = branch_twigs_vector(qp, n); + offset = branch_key_offset(n); + bit = qpkey_bit(search, searchlen, offset); + if (bit != SHIFT_NOBYTE && branch_has_twig(n, SHIFT_NOBYTE) && + qpkey_bit(search, searchlen, offset - 1) == SHIFT_NOBYTE && + !is_branch(&twigs[0])) + { + label[labels].off = offset; + label[labels].ref = branch_twigs_ref(n); + labels++; + INSIST(labels <= DNS_NAME_MAXLABELS); + } + if (branch_has_twig(n, bit)) { + n = branch_twig_ptr(qp, n, bit); + } else if (labels == 0) { + /* any twig will do */ + n = &twigs[0]; + } else { + n = ref_ptr(qp, label[labels - 1].ref); + break; + } + } + + /* do the keys differ, and if so, where? */ + foundlen = leaf_qpkey(qp, n, found); + offset = qpkey_compare(search, searchlen, found, foundlen); + + if (offset == QPKEY_EQUAL || offset == foundlen) { + *pval_r = leaf_pval(n); + *ival_r = leaf_ival(n); + if (offset == QPKEY_EQUAL) { + return (result); + } else { + return (DNS_R_PARTIALMATCH); + } + } + while (labels-- > 0) { + if (offset > label[labels].off) { + n = ref_ptr(qp, label[labels].ref); + *pval_r = leaf_pval(n); + *ival_r = leaf_ival(n); + return (DNS_R_PARTIALMATCH); + } + } + return (ISC_R_NOTFOUND); +} + /**********************************************************************/ diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 3b096d28db..9391804b15 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -216,10 +216,150 @@ ISC_RUN_TEST_IMPL(qpiter) { dns_qp_destroy(&qp); } +static uint32_t +no_op(void *uctx, void *pval, uint32_t ival) { + UNUSED(uctx); + UNUSED(pval); + UNUSED(ival); + return (1); +} + +static size_t +qpkey_fromstring(dns_qpkey_t key, void *uctx, void *pval, uint32_t ival) { + dns_fixedname_t fixed; + + UNUSED(uctx); + UNUSED(ival); + if (*(char *)pval == '\0') { + return (0); + } + dns_test_namefromstring(pval, &fixed); + return (dns_qpkey_fromname(key, dns_fixedname_name(&fixed))); +} + +const struct dns_qpmethods string_methods = { + no_op, + no_op, + qpkey_fromstring, + getname, +}; + +struct check_partialmatch { + const char *query; + dns_qpfind_t options; + isc_result_t result; + const char *found; +}; + +static void +check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { + for (int i = 0; check[i].query != NULL; i++) { + isc_result_t result; + dns_fixedname_t fixed; + dns_name_t *name = dns_fixedname_name(&fixed); + void *pval = NULL; + uint32_t ival; + +#if 0 + fprintf(stderr, "%s %u %s %s\n", check[i].query, + check[i].options, isc_result_totext(check[i].result), + check[i].found); +#endif + dns_test_namefromstring(check[i].query, &fixed); + result = dns_qp_findname_parent(qp, name, check[i].options, + &pval, &ival); + assert_int_equal(result, check[i].result); + if (check[i].found == NULL) { + assert_null(pval); + } else { + assert_string_equal(pval, check[i].found); + } + } +} + +static void +insert_str(dns_qp_t *qp, const char *str) { + isc_result_t result; + uintptr_t pval = (uintptr_t)str; + INSIST((pval & 3) == 0); + result = dns_qp_insert(qp, (void *)pval, 0); + assert_int_equal(result, ISC_R_SUCCESS); +} + +ISC_RUN_TEST_IMPL(partialmatch) { + isc_result_t result; + dns_qp_t *qp = NULL; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + + /* + * Fixed size strings [16] should ensure leaf-compatible alignment. + */ + const char insert[][16] = { + "a.b.", "b.", "fo.bar.", "foo.bar.", + "fooo.bar.", "web.foo.bar.", ".", "", + }; + + int i = 0; + while (insert[i][0] != '.') { + insert_str(qp, insert[i++]); + } + + static struct check_partialmatch check1[] = { + { "a.b.", 0, ISC_R_SUCCESS, "a.b." }, + { "a.b.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "b." }, + { "b.c.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, + { "bar.", 0, ISC_R_NOTFOUND, NULL }, + { "f.bar.", 0, ISC_R_NOTFOUND, NULL }, + { "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." }, + { "foo.bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, + { "foooo.bar.", 0, ISC_R_NOTFOUND, NULL }, + { "w.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, + { "www.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, + { "web.foo.bar.", 0, ISC_R_SUCCESS, "web.foo.bar." }, + { "webby.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, + { "my.web.foo.bar.", 0, DNS_R_PARTIALMATCH, "web.foo.bar." }, + { "web.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, + "foo.bar." }, + { "my.web.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, + "web.foo.bar." }, + { NULL, 0, 0, NULL }, + }; + check_partialmatch(qp, check1); + + /* what if the trie contains the root? */ + INSIST(insert[i][0] == '.'); + insert_str(qp, insert[i++]); + + static struct check_partialmatch check2[] = { + { "b.c.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, + { "bar.", 0, DNS_R_PARTIALMATCH, "." }, + { "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." }, + { "foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, + { NULL, 0, 0, NULL }, + }; + check_partialmatch(qp, check2); + + /* what if entries in the trie are relative to the zone apex? */ + dns_qpkey_t rootkey = { SHIFT_NOBYTE }; + result = dns_qp_deletekey(qp, rootkey, 1); + assert_int_equal(result, ISC_R_SUCCESS); + INSIST(insert[i][0] == '\0'); + insert_str(qp, insert[i++]); + check_partialmatch(qp, (struct check_partialmatch[]){ + { "bar", 0, DNS_R_PARTIALMATCH, "" }, + { "bar.", 0, DNS_R_PARTIALMATCH, "" }, + { NULL, 0, 0, NULL }, + }); + + dns_qp_destroy(&qp); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY(qpkey_name) ISC_TEST_ENTRY(qpkey_sort) ISC_TEST_ENTRY(qpiter) +ISC_TEST_ENTRY(partialmatch) ISC_TEST_LIST_END ISC_TEST_MAIN From 44c80c4ae1585f9237cdcf7fe0571e3bd3fd9ca3 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Thu, 2 Mar 2023 13:30:24 +0000 Subject: [PATCH 3/6] Support for off-loop read-ony qp-trie transactions It is sometimes necessary to access a qp-trie outside an isc_loop, such as in tests or an isc_work callback. The best option was to use a `dns_qpmulti_write()` transaction, but that has overheads that are not necessary for read-only access, such as committing a new version of the trie even when nothing changed. So this commit adds a `dns_qpmulti_read()` transaction, which is nearly as lightweight as a query transaction, but it takes the mutex like a write transaction. --- lib/dns/include/dns/qp.h | 25 ++++++++++++++++++++++++- lib/dns/qp.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index a860c5a901..09da5ad1a5 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -53,6 +53,12 @@ * lifetime of a `dns_qpread_t`, instead of using locks. Readers are * not blocked by any write activity, and vice versa. * + * For read-only access outside the scope of a loop, such as from an + * isc_work callback, `dns_qpmulti_lockedread()`. This looks like a + * query transaction; the difference is that a locked read transaction + * takes the `dns_qpmulti_t` mutex. When you have finished with a + * `dns_qpread_t`, call `dns_qpread_destroy()` to release the mutex. + * * For reads that need a stable view of the trie for multiple cycles * of an isc_loop, or which can be used from any thread, call * `dns_qpmulti_snapshot()` to get a `dns_qpsnap_t`. A snapshot is for @@ -598,10 +604,27 @@ dns_qpmulti_query(dns_qpmulti_t *multi, dns_qpread_t *qpr); * \li `qpr` is a valid read-only qp-trie handle */ +void +dns_qpmulti_lockedread(dns_qpmulti_t *multi, dns_qpread_t *qpr); +/*%< + * Start a read-only transaction that takes the `dns_qpmulti_t` mutex. + * + * The `dns_qpmulti_lockedread()` function must NOT be called from an + * isc_loop thread. We keep query and read transactions separate to + * avoid accidentally taking or failing to take the mutex. + * + * Requires: + * \li `multi` is a pointer to a valid multi-threaded qp-trie + * \li `qpr != NULL` + * + * Returns: + * \li `qpr` is a valid read-only qp-trie handle + */ + void dns_qpread_destroy(dns_qpmulti_t *multi, dns_qpread_t *qpr); /*%< - * End a lightweight read transaction. + * End a lightweight query or read transaction. * * Requires: * \li `multi` is a pointer to a valid multi-threaded qp-trie diff --git a/lib/dns/qp.c b/lib/dns/qp.c index c6bfa74953..bec2ef8485 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1282,12 +1282,31 @@ dns_qpmulti_query(dns_qpmulti_t *multi, dns_qpread_t *qp) { REQUIRE(QPMULTI_VALID(multi)); REQUIRE(qp != NULL); - dns_qpmulti_t *whence = reader_open(multi, qp); - INSIST(whence == multi); - - /* we must be in an isc_loop thread */ + /* we MUST be in an isc_loop thread */ qp->tid = isc_tid(); REQUIRE(qp->tid != ISC_TID_UNKNOWN); + + dns_qpmulti_t *whence = reader_open(multi, qp); + INSIST(whence == multi); +} + +/* + * a locked read takes the mutex + */ + +void +dns_qpmulti_lockedread(dns_qpmulti_t *multi, dns_qpread_t *qp) { + REQUIRE(QPMULTI_VALID(multi)); + REQUIRE(qp != NULL); + + /* we MUST NOT be in an isc_loop thread */ + qp->tid = isc_tid(); + REQUIRE(qp->tid == ISC_TID_UNKNOWN); + + LOCK(&multi->mutex); + + dns_qpmulti_t *whence = reader_open(multi, qp); + INSIST(whence == multi); } void @@ -1295,6 +1314,9 @@ dns_qpread_destroy(dns_qpmulti_t *multi, dns_qpread_t *qp) { REQUIRE(QPMULTI_VALID(multi)); REQUIRE(QP_VALID(qp)); REQUIRE(qp->tid == isc_tid()); + if (qp->tid == ISC_TID_UNKNOWN) { + UNLOCK(&multi->mutex); + } *qp = (dns_qpread_t){}; } From 39f38754e22d048275e0cadf2ce480408c70cab7 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 3 Mar 2023 12:05:51 +0000 Subject: [PATCH 4/6] Compact more in dns_qp_compact(DNS_QPGC_ALL) Commit 0858514ae8 enriched dns_qp_compact() to give callers more control over how thoroughly the trie should be compacted. In the DNS_QPGC_ALL case, if the trie is small it might be compacted to a new position in the same memory chunk. In this situation it will still be holding references to old leaf objects which have been removed from the trie but will not be completely detached until the chunk containing the references is freed. This change resets the qp-trie allocator to a fresh chunk before a DNS_QPGC_ALL compaction, so all the old memory chunks will be evacuated and old leaf objects can be detached sooner. --- lib/dns/qp.c | 1 + tests/bench/load-names.c | 2 +- tests/bench/qp-dump.c | 2 +- tests/bench/qpmulti.c | 8 ++++++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index bec2ef8485..881b68ec9a 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -903,6 +903,7 @@ dns_qp_compact(dns_qp_t *qp, dns_qpgc_t mode) { return; } if (mode == DNS_QPGC_ALL) { + alloc_reset(qp); qp->compact_all = true; } compact(qp); diff --git a/tests/bench/load-names.c b/tests/bench/load-names.c index a1473d8694..f7d5aa79d2 100644 --- a/tests/bench/load-names.c +++ b/tests/bench/load-names.c @@ -164,7 +164,7 @@ add_qp(void *qp, size_t count) { static void sqz_qp(void *qp) { - dns_qp_compact(qp, true); + dns_qp_compact(qp, DNS_QPGC_ALL); } static isc_result_t diff --git a/tests/bench/qp-dump.c b/tests/bench/qp-dump.c index 515048ea97..9cf72c1661 100644 --- a/tests/bench/qp-dump.c +++ b/tests/bench/qp-dump.c @@ -224,7 +224,7 @@ main(int argc, char *argv[]) { labels += name->labels; names += 1; } - dns_qp_compact(qp, true); + dns_qp_compact(qp, DNS_QPGC_ALL); size_t smallbytes = wirebytes + labels + names * sizeof(isc_refcount_t); dns_qp_memusage_t memusage = dns_qp_memusage(qp); diff --git a/tests/bench/qpmulti.c b/tests/bench/qpmulti.c index 59c9a11a3e..f6150024a9 100644 --- a/tests/bench/qpmulti.c +++ b/tests/bench/qpmulti.c @@ -323,8 +323,12 @@ mutate_transactions(uv_idle_t *idle) { args->absent++; } } + /* + * We would normally use DNS_QPGC_MAYBE, but here we do the + * fragmented check ourself so we can count compactions + */ if (dns_qp_memusage(qp).fragmented) { - dns_qp_compact(qp, false); + dns_qp_compact(qp, DNS_QPGC_NOW); args->compactions++; } dns_qpmulti_commit(args->multi, &qp); @@ -392,7 +396,7 @@ load_multi(struct bench_state *bctx) { item[i].present = true; count++; } - dns_qp_compact(qp, true); + dns_qp_compact(qp, DNS_QPGC_ALL); dns_qpmulti_commit(bctx->multi, &qp); bctx->load_time = isc_time_monotonic() - start; From b3e35fd1201b9ff1416da4a3d58350d2c303e26d Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Fri, 10 Mar 2023 15:55:00 +0000 Subject: [PATCH 5/6] A few qp-trie cleanups Revert refcount debug tracing (commit a8b29f0365), there are better ways to do it. Use the dns_qpmethods_t typedef where appropriate. Some stylistic improvements. --- fuzz/dns_qp.c | 11 +++--- lib/dns/include/dns/qp.h | 15 +++++--- lib/dns/qp.c | 62 +++++++++++------------------- lib/dns/qp_p.h | 30 --------------- tests/bench/load-names.c | 77 ++++++++++++++++++------------------- tests/bench/qp-dump.c | 83 ++++++++++++++++++++++------------------ tests/bench/qpmulti.c | 59 ++++++++++++++-------------- tests/dns/qp_test.c | 10 ++--- tests/dns/qpmulti_test.c | 14 +++---- 9 files changed, 158 insertions(+), 203 deletions(-) diff --git a/fuzz/dns_qp.c b/fuzz/dns_qp.c index a528019597..1bc19fa38f 100644 --- a/fuzz/dns_qp.c +++ b/fuzz/dns_qp.c @@ -57,19 +57,18 @@ static struct { dns_qpkey_t ascii; } item[256 * 256 / 4]; -static uint32_t +static void fuzz_attach(void *ctx, void *pval, uint32_t ival) { assert(ctx == NULL); assert(pval == &item[ival]); - return (item[ival].refcount++); + item[ival].refcount++; } -static uint32_t +static void fuzz_detach(void *ctx, void *pval, uint32_t ival) { assert(ctx == NULL); assert(pval == &item[ival]); - assert(item[ival].refcount > 0); - return (item[ival].refcount--); + item[ival].refcount--; } static size_t @@ -86,7 +85,7 @@ fuzz_triename(void *ctx, char *buf, size_t size) { strlcpy(buf, "fuzz", size); } -const struct dns_qpmethods fuzz_methods = { +const dns_qpmethods_t fuzz_methods = { fuzz_attach, fuzz_detach, fuzz_makekey, diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 09da5ad1a5..a425a4c257 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -109,7 +109,9 @@ typedef struct dns_qpmulti dns_qpmulti_t; * Read-only parts of a qp-trie. * * A `dns_qpreader_t` is the common prefix of the `dns_qpreadable` - * types, containing just the fields neded for the hot path. + * types, containing just the fields neded for the hot path. The + * internals of a `dns_qpreader_t` are private; they are only exposed + * so that callers can allocate a `dns_qpread_t` on the stack. * * Ranty aside: annoyingly, C doesn't allow us to use a predeclared * structure type as an anonymous struct member, so we have to use a @@ -135,6 +137,9 @@ typedef struct dns_qpreader { * The caller provides space for it on the stack; it can be * used by only one thread. As well as the `DNS_QPREADER_FIELDS`, * it contains a thread ID to check for incorrect usage. + * + * The internals of a `dns_qpread_t` are private; they are only + * exposed so that callers can allocate an instance on the stack. */ typedef struct dns_qpread { DNS_QPREADER_FIELDS; @@ -209,9 +214,7 @@ typedef struct dns_qpiter { * The `attach` and `detach` methods adjust reference counts on value * objects. They support copy-on-write and safe memory reclamation * needed for multi-version concurrency. The methods are only called - * when the `dns_qpmulti_t` mutex is held. For tracing purposes, they - * should return the same value as `isc_refcount_increment()` or - * `isc_refcount_decrement()`, respectively + * when the `dns_qpmulti_t` mutex is held. * * Note: When a value object reference count is greater than one, the * object is in use by concurrent readers so it must not be modified. A @@ -230,8 +233,8 @@ typedef struct dns_qpiter { * readable identifier into `buf` which has max length `size`. */ typedef struct dns_qpmethods { - uint32_t (*attach)(void *uctx, void *pval, uint32_t ival); - uint32_t (*detach)(void *uctx, void *pval, uint32_t ival); + void (*attach)(void *uctx, void *pval, uint32_t ival); + void (*detach)(void *uctx, void *pval, uint32_t ival); size_t (*makekey)(dns_qpkey_t key, void *uctx, void *pval, uint32_t ival); void (*triename)(void *uctx, char *buf, size_t size); diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 881b68ec9a..60acf13ed5 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -482,6 +482,7 @@ static inline qp_ref_t alloc_twigs(dns_qp_t *qp, qp_weight_t size) { qp_chunk_t chunk = qp->bump; qp_cell_t cell = qp->usage[chunk].used; + if (cell + size <= QP_CHUNK_SIZE) { qp->usage[chunk].used += size; qp->used_count += size; @@ -843,6 +844,7 @@ compact_recursive(dns_qp_t *qp, qp_node_t *parent) { qp_weight_t size = branch_twigs_size(parent); qp_ref_t twigs_ref = branch_twigs_ref(parent); qp_chunk_t chunk = ref_chunk(twigs_ref); + if (qp->compact_all || (chunk != qp->bump && chunk_usage(qp, chunk) < QP_MIN_USED)) { @@ -1027,15 +1029,12 @@ dns_qp_gctime(isc_nanosecs_t *compact_p, isc_nanosecs_t *recycle_p, static dns_qp_t * transaction_open(dns_qpmulti_t *multi, dns_qp_t **qptp) { - dns_qp_t *qp; - REQUIRE(QPMULTI_VALID(multi)); REQUIRE(qptp != NULL && *qptp == NULL); LOCK(&multi->mutex); - qp = &multi->writer; - + dns_qp_t *qp = &multi->writer; INSIST(QP_VALID(qp)); /* @@ -1396,11 +1395,9 @@ dns_qpsnap_destroy(dns_qpmulti_t *multi, dns_qpsnap_t **qpsp) { void dns_qp_create(isc_mem_t *mctx, const dns_qpmethods_t *methods, void *uctx, dns_qp_t **qptp) { - dns_qp_t *qp; - REQUIRE(qptp != NULL && *qptp == NULL); - qp = isc_mem_get(mctx, sizeof(*qp)); + dns_qp_t *qp = isc_mem_get(mctx, sizeof(*qp)); QP_INIT(qp, methods, uctx); isc_mem_attach(mctx, &qp->mctx); alloc_reset(qp); @@ -1412,12 +1409,9 @@ void dns_qpmulti_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, const dns_qpmethods_t *methods, void *uctx, dns_qpmulti_t **qpmp) { - dns_qpmulti_t *multi; - dns_qp_t *qp; - REQUIRE(qpmp != NULL && *qpmp == NULL); - multi = isc_mem_get(mctx, sizeof(*multi)); + dns_qpmulti_t *multi = isc_mem_get(mctx, sizeof(*multi)); *multi = (dns_qpmulti_t){ .magic = QPMULTI_MAGIC, .reader_ref = INVALID_REF, @@ -1432,7 +1426,7 @@ dns_qpmulti_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, * allocates; to ensure dns_qpmulti_write() does too, pretend the * previous transaction was an update */ - qp = &multi->writer; + dns_qp_t *qp = &multi->writer; QP_INIT(qp, methods, uctx); isc_mem_attach(mctx, &qp->mctx); qp->transaction_mode = QP_UPDATE; @@ -1460,12 +1454,10 @@ destroy_guts(dns_qp_t *qp) { void dns_qp_destroy(dns_qp_t **qptp) { - dns_qp_t *qp; - REQUIRE(qptp != NULL); REQUIRE(QP_VALID(*qptp)); - qp = *qptp; + dns_qp_t *qp = *qptp; *qptp = NULL; /* do not try to destroy part of a dns_qpmulti_t */ @@ -1478,14 +1470,11 @@ dns_qp_destroy(dns_qp_t **qptp) { void dns_qpmulti_destroy(dns_qpmulti_t **qpmp) { - dns_qp_t *qp = NULL; - dns_qpmulti_t *multi = NULL; - REQUIRE(qpmp != NULL); REQUIRE(QPMULTI_VALID(*qpmp)); - multi = *qpmp; - qp = &multi->writer; + dns_qpmulti_t *multi = *qpmp; + dns_qp_t *qp = &multi->writer; *qpmp = NULL; REQUIRE(QP_VALID(qp)); @@ -1513,7 +1502,7 @@ isc_result_t dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival) { qp_ref_t new_ref, old_ref; qp_node_t new_leaf, old_node; - qp_node_t *new_twigs, *old_twigs; + qp_node_t *new_twigs = NULL, *old_twigs = NULL; qp_shift_t new_bit, old_bit; qp_weight_t old_size, new_size; dns_qpkey_t new_key, old_key; @@ -1522,7 +1511,7 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival) { uint64_t index; qp_shift_t bit; qp_weight_t pos; - qp_node_t *n; + qp_node_t *n = NULL; REQUIRE(QP_VALID(qp)); @@ -1639,15 +1628,6 @@ growbranch: isc_result_t dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, size_t search_keylen) { - dns_qpkey_t found_key; - size_t found_keylen; - qp_shift_t bit = 0; /* suppress warning */ - qp_weight_t pos, size; - qp_ref_t ref; - qp_node_t *twigs; - qp_node_t *parent; - qp_node_t *n; - REQUIRE(QP_VALID(qp)); REQUIRE(search_keylen < sizeof(dns_qpkey_t)); @@ -1655,8 +1635,9 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, return (ISC_R_NOTFOUND); } - parent = NULL; - n = make_root_mutable(qp); + qp_shift_t bit = 0; /* suppress warning */ + qp_node_t *parent = NULL; + qp_node_t *n = make_root_mutable(qp); while (is_branch(n)) { prefetch_twigs(qp, n); bit = branch_keybit(n, search_key, search_keylen); @@ -1668,7 +1649,8 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, n = branch_twig_ptr(qp, n, bit); } - found_keylen = leaf_qpkey(qp, n, found_key); + dns_qpkey_t found_key; + size_t found_keylen = leaf_qpkey(qp, n, found_key); if (qpkey_compare(search_key, search_keylen, found_key, found_keylen) != QPKEY_EQUAL) { @@ -1692,10 +1674,10 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, parent = NULL; INSIST(bit != 0); - size = branch_twigs_size(n); - pos = branch_twig_pos(n, bit); - ref = branch_twigs_ref(n); - twigs = ref_ptr(qp, ref); + qp_weight_t size = branch_twigs_size(n); + qp_weight_t pos = branch_twig_pos(n, bit); + qp_ref_t ref = branch_twigs_ref(n); + qp_node_t *twigs = ref_ptr(qp, ref); if (size == 2) { /* @@ -1801,7 +1783,7 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, dns_qpkey_t found_key; size_t found_keylen; qp_shift_t bit; - qp_node_t *n; + qp_node_t *n = NULL; REQUIRE(QP_VALID(qp)); REQUIRE(pval_r != NULL); @@ -1850,7 +1832,7 @@ dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name, size_t searchlen, foundlen; size_t offset; qp_shift_t bit; - qp_node_t *n, *twigs; + qp_node_t *n = NULL, *twigs = NULL; isc_result_t result; unsigned int labels = 0; struct offref { diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index 12029e72c2..90d05a20d8 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -894,34 +894,6 @@ unpack_reader(dns_qpreader_t *qp, qp_node_t *reader) { * method invocation helpers */ -#if 0 - -#define attach_leaf(qp, n) \ - do { \ - uint32_t iv = leaf_ival(n); \ - void *pv = leaf_pval(n); \ - uint32_t r = qp->methods->attach(qp->uctx, pv, iv); \ - fprintf(stderr, \ - "%s:%u:%s():t%u qp %p node %p leaf %p %u " \ - "(%u -> %u)\n", \ - __FILE__, __LINE__, __func__, isc_tid(), qp, n, pv, \ - iv, r, r + 1); \ - } while (0) - -#define detach_leaf(qp, n) \ - do { \ - uint32_t iv = leaf_ival(n); \ - void *pv = leaf_pval(n); \ - uint32_t r = qp->methods->detach(qp->uctx, pv, iv); \ - fprintf(stderr, \ - "%s:%u:%s():t%u qp %p node %p leaf %p %u " \ - "(%u -> %u)\n", \ - __FILE__, __LINE__, __func__, isc_tid(), qp, n, pv, \ - iv, r, r - 1); \ - } while (0) - -#else - static inline void attach_leaf(dns_qpreadable_t qpr, qp_node_t *n) { dns_qpreader_t *qp = dns_qpreader(qpr); @@ -934,8 +906,6 @@ detach_leaf(dns_qpreadable_t qpr, qp_node_t *n) { qp->methods->detach(qp->uctx, leaf_pval(n), leaf_ival(n)); } -#endif - static inline size_t leaf_qpkey(dns_qpreadable_t qpr, qp_node_t *n, dns_qpkey_t key) { dns_qpreader_t *qp = dns_qpreader(qpr); diff --git a/tests/bench/load-names.c b/tests/bench/load-names.c index f7d5aa79d2..babdd37ef1 100644 --- a/tests/bench/load-names.c +++ b/tests/bench/load-names.c @@ -37,11 +37,10 @@ struct { dns_fixedname_t fixed; } item[1024 * 1024]; -static uint32_t +static void item_check(void *ctx, void *pval, uint32_t ival) { UNUSED(ctx); assert(pval == &item[ival]); - return (1); } static size_t @@ -57,7 +56,7 @@ testname(void *ctx, char *buf, size_t size) { strlcpy(buf, "test", size); } -const struct dns_qpmethods qpmethods = { +const dns_qpmethods_t qpmethods = { item_check, item_check, item_makekey, @@ -201,7 +200,7 @@ static struct fun { #define FILE_CHECK(check, msg) \ do { \ if (!(check)) { \ - fprintf(stderr, "%s:%zu: %s\n", filename, count, msg); \ + fprintf(stderr, "%s:%zu: %s\n", filename, lines, msg); \ exit(1); \ } \ } while (0) @@ -209,6 +208,12 @@ static struct fun { int main(int argc, char *argv[]) { isc_result_t result; + const char *filename = NULL; + char *filetext = NULL; + off_t fileoff; + FILE *fp = NULL; + size_t filesize, lines = 0, wirebytes = 0, labels = 0; + char *pos = NULL, *file_end = NULL; isc_mem_create(&mctx); @@ -217,18 +222,17 @@ main(int argc, char *argv[]) { exit(1); } - const char *filename = argv[1]; - off_t fileoff; + filename = argv[1]; result = isc_file_getsize(filename, &fileoff); if (result != ISC_R_SUCCESS) { fprintf(stderr, "stat(%s): %s\n", filename, isc_result_totext(result)); exit(1); } - size_t filesize = (size_t)fileoff; + filesize = (size_t)fileoff; - char *filetext = isc_mem_get(mctx, filesize + 1); - FILE *fp = fopen(filename, "r"); + filetext = isc_mem_get(mctx, filesize + 1); + fp = fopen(filename, "r"); if (fp == NULL || fread(filetext, 1, filesize, fp) < filesize) { fprintf(stderr, "read(%s): %s\n", filename, strerror(errno)); exit(1); @@ -236,29 +240,28 @@ main(int argc, char *argv[]) { fclose(fp); filetext[filesize] = '\0'; - size_t count = 0; - size_t wirebytes = 0; - size_t labels = 0; - - char *pos = filetext; - char *file_end = pos + filesize; + pos = filetext; + file_end = pos + filesize; while (pos < file_end) { - FILE_CHECK(count < ARRAY_SIZE(item), "too many lines"); + char *domain = NULL, *newline = NULL; + size_t len; + + FILE_CHECK(lines < ARRAY_SIZE(item), "too many lines"); pos += strspn(pos, "0123456789"); FILE_CHECK(*pos++ == ',', "missing comma"); - char *domain = pos; + domain = pos; pos += strcspn(pos, "\r\n"); FILE_CHECK(*pos != '\0', "missing newline"); - char *newline = pos; + newline = pos; pos += strspn(pos, "\r\n"); - size_t len = newline - domain; + len = newline - domain; - item[count].text = domain; + item[lines].text = domain; domain[len] = '\0'; - dns_name_t *name = dns_fixedname_initname(&item[count].fixed); + dns_name_t *name = dns_fixedname_initname(&item[lines].fixed); isc_buffer_t buffer; isc_buffer_init(&buffer, domain, len); isc_buffer_add(&buffer, len); @@ -268,41 +271,35 @@ main(int argc, char *argv[]) { wirebytes += name->length; labels += name->labels; - count++; + lines++; } printf("names %g MB labels %g MB\n", (double)wirebytes / 1048576.0, (double)labels / 1048576.0); - size_t lines = count; - for (struct fun *fun = fun_list; fun->name != NULL; fun++) { - isc_time_t t0; - t0 = isc_time_now_hires(); - isc_mem_t *mem = NULL; - isc_mem_create(&mem); - void *map = fun->new (mem); + void *map = NULL; - for (count = 0; count < lines; count++) { - result = fun->add(map, count); + isc_mem_create(&mem); + map = fun->new (mem); + + isc_time_t t0 = isc_time_now_hires(); + for (size_t n = 0; n < lines; n++) { + result = fun->add(map, n); CHECK(result); } fun->sqz(map); - isc_time_t t1; - t1 = isc_time_now_hires(); - - for (count = 0; count < lines; count++) { + isc_time_t t1 = isc_time_now_hires(); + for (size_t n = 0; n < lines; n++) { void *pval = NULL; - result = fun->get(map, count, &pval); + result = fun->get(map, n, &pval); CHECK(result); - assert(pval == &item[count]); + assert(pval == &item[n]); } - isc_time_t t2; - t2 = isc_time_now_hires(); - + isc_time_t t2 = isc_time_now_hires(); printf("%f sec to load %s\n", (double)isc_time_microdiff(&t1, &t0) / (1000.0 * 1000.0), fun->name); diff --git a/tests/bench/qp-dump.c b/tests/bench/qp-dump.c index 9cf72c1661..855bbbc3dc 100644 --- a/tests/bench/qp-dump.c +++ b/tests/bench/qp-dump.c @@ -95,19 +95,17 @@ qpkey_from_smallname(dns_qpkey_t key, void *ctx, void *pval, uint32_t ival) { return (dns_qpkey_fromname(key, &name)); } -static uint32_t +static void smallname_attach(void *ctx, void *pval, uint32_t ival) { UNUSED(ctx); - return (isc_refcount_increment0(smallname_refcount(pval, ival))); + isc_refcount_increment0(smallname_refcount(pval, ival)); } -static uint32_t +static void smallname_detach(void *ctx, void *pval, uint32_t ival) { - uint32_t refs = isc_refcount_decrement(smallname_refcount(pval, ival)); - if (refs == 1) { + if (isc_refcount_decrement(smallname_refcount(pval, ival)) == 1) { isc_mem_free(ctx, pval); } - return (refs); } static void @@ -116,7 +114,7 @@ testname(void *ctx, char *buf, size_t size) { strlcpy(buf, "test", size); } -const struct dns_qpmethods methods = { +const dns_qpmethods_t methods = { smallname_attach, smallname_detach, qpkey_from_smallname, @@ -126,15 +124,23 @@ const struct dns_qpmethods methods = { static void usage(void) { fprintf(stderr, - "usage: qp_dump [-drt] \n" + "usage: qp_dump [-dt] \n" " -d output in graphviz dot format\n" " -t output in ad-hoc indented text format\n"); } int main(int argc, char *argv[]) { - bool dumpdot = false; - bool dumptxt = false; + isc_result_t result; + dns_qp_t *qp = NULL; + const char *filename = NULL; + char *filetext = NULL; + size_t filesize; + off_t fileoff; + FILE *fp = NULL; + size_t wirebytes = 0, labels = 0, names = 0; + char *pos = NULL, *file_end = NULL; + bool dumpdot = false, dumptxt = false; int opt; while ((opt = isc_commandline_parse(argc, argv, "dt")) != -1) { @@ -162,18 +168,17 @@ main(int argc, char *argv[]) { isc_mem_create(&mctx); - const char *filename = argv[0]; - off_t fileoff; - isc_result_t result = isc_file_getsize(filename, &fileoff); + filename = argv[0]; + result = isc_file_getsize(filename, &fileoff); if (result != ISC_R_SUCCESS) { fprintf(stderr, "stat(%s): %s\n", filename, isc_result_totext(result)); exit(1); } - size_t filesize = (size_t)fileoff; - char *filetext = isc_mem_get(mctx, filesize + 1); - FILE *fp = fopen(filename, "r"); + filesize = (size_t)fileoff; + filetext = isc_mem_get(mctx, filesize + 1); + fp = fopen(filename, "r"); if (fp == NULL || fread(filetext, 1, filesize, fp) < filesize) { fprintf(stderr, "read(%s): %s\n", filename, strerror(errno)); exit(1); @@ -181,31 +186,30 @@ main(int argc, char *argv[]) { fclose(fp); filetext[filesize] = '\0'; - dns_qp_t *qp = NULL; dns_qp_create(mctx, &methods, NULL, &qp); - size_t wirebytes = 0; - size_t labels = 0; - size_t names = 0; - char *pos = filetext; - char *file_end = pos + filesize; + pos = filetext; + file_end = pos + filesize; while (pos < file_end) { - char *domain = pos; - pos += strcspn(pos, "\r\n"); - char *newline = pos; - pos += strspn(pos, "\r\n"); - size_t len = newline - domain; - domain[len] = '\0'; - + void *pval = NULL; + uint32_t ival = 0; dns_fixedname_t fixed; dns_name_t *name = dns_fixedname_initname(&fixed); isc_buffer_t buffer; + char *newline = NULL, *domain = pos; + size_t len; + + pos += strcspn(pos, "\r\n"); + newline = pos; + pos += strspn(pos, "\r\n"); + + len = newline - domain; + domain[len] = '\0'; + isc_buffer_init(&buffer, domain, len); isc_buffer_add(&buffer, len); result = dns_name_fromtext(name, &buffer, dns_rootname, 0, NULL); - void *pval = NULL; - uint32_t ival = 0; if (result == ISC_R_SUCCESS) { smallname_from_name(name, &pval, &ival); result = dns_qp_insert(qp, pval, ival); @@ -226,15 +230,16 @@ main(int argc, char *argv[]) { } dns_qp_compact(qp, DNS_QPGC_ALL); - size_t smallbytes = wirebytes + labels + names * sizeof(isc_refcount_t); - dns_qp_memusage_t memusage = dns_qp_memusage(qp); - uint64_t compaction_us, recovery_us, rollback_us; - dns_qp_gctime(&compaction_us, &recovery_us, &rollback_us); - #define print_megabytes(label, value) \ printf("%6.2f MiB - " label "\n", (double)(value) / 1048576.0) if (!dumptxt && !dumpdot) { + size_t smallbytes = wirebytes + labels + + names * sizeof(isc_refcount_t); + dns_qp_memusage_t memusage = dns_qp_memusage(qp); + uint64_t compaction_us, recovery_us, rollback_us; + dns_qp_gctime(&compaction_us, &recovery_us, &rollback_us); + printf("leaves %zu\n" " nodes %zu\n" " used %zu\n" @@ -264,10 +269,12 @@ main(int argc, char *argv[]) { printf("%6zu - max key len\n", qp_test_maxkeylen(qp)); } - if (dumptxt) + if (dumptxt) { qp_test_dumptrie(qp); - if (dumpdot) + } + if (dumpdot) { qp_test_dumpdot(qp); + } return (0); } diff --git a/tests/bench/qpmulti.c b/tests/bench/qpmulti.c index f6150024a9..7034ac983f 100644 --- a/tests/bench/qpmulti.c +++ b/tests/bench/qpmulti.c @@ -89,12 +89,11 @@ static struct { dns_qpkey_t key; } *item; -static uint32_t +static void item_refcount(void *ctx, void *pval, uint32_t ival) { UNUSED(ctx); UNUSED(pval); UNUSED(ival); - return (1); } static size_t @@ -111,7 +110,7 @@ benchname(void *ctx, char *buf, size_t size) { strlcpy(buf, "bench", size); } -const struct dns_qpmethods item_methods = { +const dns_qpmethods_t item_methods = { item_refcount, item_refcount, item_makekey, @@ -128,13 +127,12 @@ init_items(isc_mem_t *mctx) { void *pval = NULL; uint32_t ival = ~0U; dns_qp_t *qp = NULL; - size_t bytes = ITEM_COUNT * sizeof(*item); + uint64_t start; + start = isc_time_monotonic(); item = isc_mem_allocatex(mctx, bytes, ISC_MEM_ZERO); - uint64_t start = isc_time_monotonic(); - /* ensure there are no duplicate names */ dns_qp_create(mctx, &item_methods, NULL, &qp); for (size_t i = 0; i < ITEM_COUNT; i++) { @@ -224,6 +222,7 @@ first_loop(void *varg) { static void next_loop(struct thread_args *args, isc_nanosecs_t start) { isc_nanosecs_t stop = isc_time_monotonic(); + args->worked += stop - start; args->stop = stop; if (args->stop - args->start < RUNTIME) { @@ -238,6 +237,9 @@ next_loop(struct thread_args *args, isc_nanosecs_t start) { static void read_zipf(uv_idle_t *idle) { struct thread_args *args = idle->data; + isc_nanosecs_t start; + void *pval = NULL; + uint32_t ival; /* outside time because it is v slow */ uint32_t r[args->tx_per_loop][args->ops_per_tx]; @@ -247,10 +249,7 @@ read_zipf(uv_idle_t *idle) { } } - isc_nanosecs_t start = isc_time_monotonic(); - void *pval; - uint32_t ival; - + start = isc_time_monotonic(); for (uint32_t tx = 0; tx < args->tx_per_loop; tx++) { args->transactions++; dns_qpread_t qp; @@ -277,7 +276,7 @@ static void read_transactions(uv_idle_t *idle) { struct thread_args *args = idle->data; isc_nanosecs_t start = isc_time_monotonic(); - void *pval; + void *pval = NULL; uint32_t ival; for (uint32_t tx = 0; tx < args->tx_per_loop; tx++) { @@ -379,13 +378,13 @@ static void load_multi(struct bench_state *bctx) { dns_qp_t *qp = NULL; size_t count = 0; - - uint64_t start = isc_time_monotonic(); + uint64_t start; dns_qpmulti_create(bctx->mctx, bctx->loopmgr, &item_methods, NULL, &bctx->multi); /* initial contents of the trie */ + start = isc_time_monotonic(); dns_qpmulti_update(bctx->multi, &qp); for (size_t i = 0; i < bctx->max_item; i++) { if (isc_random_uniform(2) == 0) { @@ -745,11 +744,18 @@ dispatch(struct bench_state *bctx) { static void collect(void *varg) { - TRACE(""); - struct thread_args *args = varg; struct bench_state *bctx = args->bctx; struct thread_args *thread = bctx->thread; + struct { + uint64_t worked, txns, ops, compactions; + } stats[2] = {}; + double load_time = bctx->load_time; + double elapsed = 0, mut_work, readers, read_work, elapsed_ms; + uint32_t nloops; + bool zipf; + + TRACE("collect"); bctx->waiting--; if (bctx->waiting > 0) { @@ -757,20 +763,15 @@ collect(void *varg) { } isc_barrier_destroy(&bctx->barrier); - struct { - uint64_t worked, txns, ops, compactions; - } stats[2] = {}; - - double load_time = bctx->load_time; load_time = load_time > 0 ? load_time / (double)NS_PER_SEC : NAN; - double elapsed = 0; - bool zipf = bctx->mutate == 0 && bctx->readers == 0; - uint32_t nloops = zipf ? bctx->nloops : bctx->readers + bctx->mutate; + zipf = bctx->mutate == 0 && bctx->readers == 0; + nloops = zipf ? bctx->nloops : bctx->readers + bctx->mutate; for (uint32_t t = 0; t < nloops; t++) { struct thread_args *tp = &thread[t]; elapsed = ISC_MAX(elapsed, (tp->stop - tp->start)); bool mut = t < bctx->mutate; + stats[mut].worked += tp->worked; stats[mut].txns += tp->transactions; stats[mut].ops += tp->transactions * tp->ops_per_tx; @@ -783,7 +784,7 @@ collect(void *varg) { printf("%7.2f\t", (double)bctx->qp_bytes / bctx->qp_items); printf("%7u\t", bctx->max_item); - double mut_work = stats[1].worked / (double)US_PER_MS; + mut_work = stats[1].worked / (double)US_PER_MS; printf("%7u\t", bctx->mutate); printf("%7u\t", bctx->mut_tx_per_loop); printf("%7u\t", bctx->mut_ops_per_tx); @@ -794,9 +795,9 @@ collect(void *varg) { printf("%7.2f\t", stats[1].txns / mut_work); printf("%7.2f\t", stats[1].ops / mut_work); - double readers = zipf ? bctx->nloops - bctx->mutate : bctx->readers; - double read_work = stats[0].worked / (double)US_PER_MS; - double elapsed_ms = elapsed / (double)US_PER_MS; + readers = zipf ? bctx->nloops - bctx->mutate : bctx->readers; + read_work = stats[0].worked / (double)US_PER_MS; + elapsed_ms = elapsed / (double)US_PER_MS; printf("%7u\t", bctx->readers); printf("%7u\t", bctx->read_tx_per_loop); printf("%7u\t", bctx->read_ops_per_tx); @@ -817,10 +818,8 @@ startup(void *arg) { isc_loop_t *loop = isc_loop_current(loopmgr); isc_mem_t *mctx = isc_loop_getmctx(loop); uint32_t nloops = isc_loopmgr_nloops(loopmgr); - size_t bytes = sizeof(struct bench_state) + sizeof(struct thread_args) * nloops; - struct bench_state *bctx = isc_mem_getx(mctx, bytes, ISC_MEM_ZERO); *bctx = (struct bench_state){ @@ -885,9 +884,9 @@ int main(void) { isc_loopmgr_t *loopmgr = NULL; isc_mem_t *mctx = NULL; - uint32_t nloops; const char *env_workers = getenv("ISC_TASK_WORKERS"); + if (env_workers != NULL) { nloops = atoi(env_workers); } else { diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 9391804b15..ee9b30da00 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -136,12 +136,11 @@ ISC_RUN_TEST_IMPL(qpkey_sort) { #define ITER_ITEMS 100 -static uint32_t +static void check_leaf(void *uctx, void *pval, uint32_t ival) { uint32_t *items = uctx; assert_in_range(ival, 1, ITER_ITEMS - 1); assert_ptr_equal(items + ival, pval); - return (1); } static size_t @@ -168,7 +167,7 @@ getname(void *uctx, char *buf, size_t size) { UNUSED(size); } -const struct dns_qpmethods qpiter_methods = { +const dns_qpmethods_t qpiter_methods = { check_leaf, check_leaf, qpiter_makekey, @@ -216,12 +215,11 @@ ISC_RUN_TEST_IMPL(qpiter) { dns_qp_destroy(&qp); } -static uint32_t +static void no_op(void *uctx, void *pval, uint32_t ival) { UNUSED(uctx); UNUSED(pval); UNUSED(ival); - return (1); } static size_t @@ -237,7 +235,7 @@ qpkey_fromstring(dns_qpkey_t key, void *uctx, void *pval, uint32_t ival) { return (dns_qpkey_fromname(key, dns_fixedname_name(&fixed))); } -const struct dns_qpmethods string_methods = { +const dns_qpmethods_t string_methods = { no_op, no_op, qpkey_fromstring, diff --git a/tests/dns/qpmulti_test.c b/tests/dns/qpmulti_test.c index 28605080e0..16e5350dec 100644 --- a/tests/dns/qpmulti_test.c +++ b/tests/dns/qpmulti_test.c @@ -104,19 +104,19 @@ static struct { dns_qpkey_t ascii; } item[ITEM_COUNT]; -static uint32_t +static void item_attach(void *ctx, void *pval, uint32_t ival) { - assert_null(ctx); - assert_ptr_equal(pval, &item[ival]); - return (item[ival].refcount++); + INSIST(ctx == NULL); + INSIST(pval == &item[ival]); + item[ival].refcount++; } -static uint32_t +static void item_detach(void *ctx, void *pval, uint32_t ival) { assert_null(ctx); assert_ptr_equal(pval, &item[ival]); assert_int_not_equal(item[ival].refcount, 0); - return (item[ival].refcount--); + item[ival].refcount--; } static size_t @@ -144,7 +144,7 @@ testname(void *ctx, char *buf, size_t size) { strlcpy(buf, "test", size); } -const struct dns_qpmethods test_methods = { +const dns_qpmethods_t test_methods = { item_attach, item_detach, item_makekey, From b171cacf4f0123ba96bef6eedfc92dfb608db6b7 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Tue, 14 Feb 2023 16:13:16 +0000 Subject: [PATCH 6/6] Use a qp-trie for the zone table This change makes the zone table lock-free for reads. Previously, the zone table used a red-black tree, which is not thread safe, so the hot read path acquired both the per-view mutex and the per-zonetable rwlock. (The double locking was to fix to cleanup races on shutdown.) One visible difference is that zones are not necessarily shut down promptly: it depends on when the qp-trie garbage collector cleans up the zone table. The `catz` system test checks several times that zones have been deleted; the test now checks for zones to be removed from the server configuration, instead of being fully shut down. The catz test does not churn through enough zones to trigger a gc, so the zones are not fully detached until the server exits. After this change, it is still possible to improve the way we handle changes to the zone table, for instance, batching changes, or better compaction heuristics. --- CHANGES | 3 + bin/delv/delv.c | 3 +- bin/named/server.c | 64 ++- bin/named/statschannel.c | 9 +- bin/tests/system/catz/tests.sh | 24 +- bin/tests/system/dyndb/driver/syncptr.c | 2 +- bin/tests/system/pipelined/pipequeries.c | 2 +- bin/tools/mdig.c | 2 +- fuzz/dns_message_checksig.c | 8 +- lib/dns/catz.c | 19 +- lib/dns/client.c | 3 +- lib/dns/include/dns/view.h | 25 +- lib/dns/include/dns/zone.h | 7 +- lib/dns/include/dns/zt.h | 79 ++-- lib/dns/resolver.c | 29 +- lib/dns/view.c | 126 +----- lib/dns/zone.c | 14 +- lib/dns/zt.c | 499 ++++++++++------------- lib/ns/notify.c | 8 +- lib/ns/query.c | 3 +- lib/ns/update.c | 9 +- lib/ns/xfrout.c | 4 +- tests/dns/zt_test.c | 32 +- tests/libtest/dns.c | 2 +- 24 files changed, 374 insertions(+), 602 deletions(-) diff --git a/CHANGES b/CHANGES index b1ce3a727b..4634446aa1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6146. [performance] Replace the zone table red-black tree and associated + locking with a lock-free qp-trie. [GL !7582] + 6145. [bug] Fix a possible use-after-free bug in the dns__catz_done_cb() function. [GL #3997] diff --git a/bin/delv/delv.c b/bin/delv/delv.c index a354a60e65..91ebe62348 100644 --- a/bin/delv/delv.c +++ b/bin/delv/delv.c @@ -2134,7 +2134,8 @@ run_server(void *arg) { CHECK(ns_interfacemgr_create(mctx, sctx, loopmgr, netmgr, dispatchmgr, NULL, false, &interfacemgr)); - CHECK(dns_view_create(mctx, dns_rdataclass_in, "_default", &view)); + CHECK(dns_view_create(mctx, loopmgr, dns_rdataclass_in, "_default", + &view)); CHECK(dns_cache_create(loopmgr, dns_rdataclass_in, "", &cache)); dns_view_setcache(view, cache, false); dns_cache_detach(&cache); diff --git a/bin/named/server.c b/bin/named/server.c index 8bc4e32db9..8e3ba56205 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -2708,7 +2708,7 @@ catz_addmodzone_cb(void *arg) { goto cleanup; } - result = dns_zt_find(cz->view->zonetable, name, 0, NULL, &zone); + result = dns_view_findzone(cz->view, name, &zone); if (cz->mod) { dns_catz_zone_t *parentcatz; @@ -2780,19 +2780,8 @@ catz_addmodzone_cb(void *arg) { nameb); } goto cleanup; - } else if (result != ISC_R_NOTFOUND && - result != DNS_R_PARTIALMATCH) - { - isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, - NAMED_LOGMODULE_SERVER, ISC_LOG_WARNING, - "catz: error \"%s\" while trying to " - "add zone '%s'", - isc_result_totext(result), nameb); - goto cleanup; - } else { /* this can happen in case of DNS_R_PARTIALMATCH */ - if (zone != NULL) { - dns_zone_detach(&zone); - } + } else { + RUNTIME_CHECK(result == ISC_R_NOTFOUND); } } RUNTIME_CHECK(zone == NULL); @@ -2845,7 +2834,7 @@ catz_addmodzone_cb(void *arg) { } /* Is it there yet? */ - CHECK(dns_zt_find(cz->view->zonetable, name, 0, NULL, &zone)); + CHECK(dns_view_findzone(cz->view, name, &zone)); /* * Load the zone from the master file. If this fails, we'll @@ -2901,8 +2890,8 @@ catz_delzone_cb(void *arg) { dns_name_format(dns_catz_entry_getname(cz->entry), cname, DNS_NAME_FORMATSIZE); - result = dns_zt_find(cz->view->zonetable, - dns_catz_entry_getname(cz->entry), 0, NULL, &zone); + result = dns_view_findzone(cz->view, dns_catz_entry_getname(cz->entry), + &zone); if (result != ISC_R_SUCCESS) { isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_SERVER, ISC_LOG_WARNING, @@ -6448,7 +6437,8 @@ create_view(const cfg_obj_t *vconfig, dns_viewlist_t *viewlist, } INSIST(view == NULL); - result = dns_view_create(named_g_mctx, viewclass, viewname, &view); + result = dns_view_create(named_g_mctx, named_g_loopmgr, viewclass, + viewname, &view); if (result != ISC_R_SUCCESS) { return (result); } @@ -9734,8 +9724,8 @@ cleanup_viewlist: if (result == ISC_R_SUCCESS && strcmp(view->name, "_bind") != 0) { dns_view_setviewrevert(view); - (void)dns_zt_apply(view->zonetable, isc_rwlocktype_read, - false, NULL, removed, view); + (void)dns_zt_apply(view->zonetable, false, NULL, + removed, view); } dns_view_detach(&view); } @@ -10564,8 +10554,7 @@ zone_from_args(named_server_t *server, isc_lex_t *lex, const char *zonetxt, result = ISC_R_NOTFOUND; } } else { - result = dns_zt_find(view->zonetable, name, 0, NULL, - zonep); + result = dns_view_findzone(view, name, zonep); } if (result != ISC_R_SUCCESS) { snprintf(problem, sizeof(problem), @@ -11304,8 +11293,8 @@ add_view_tolist(struct dumpcontext *dctx, dns_view_t *view) { ISC_LIST_INIT(vle->zonelist); ISC_LIST_APPEND(dctx->viewlist, vle, link); if (dctx->dumpzones) { - result = dns_zt_apply(view->zonetable, isc_rwlocktype_read, - true, NULL, add_zone_tolist, dctx); + result = dns_zt_apply(view->zonetable, true, NULL, + add_zone_tolist, dctx); } return (result); } @@ -12402,8 +12391,7 @@ named_server_sync(named_server_t *server, isc_lex_t *lex, isc_buffer_t **text) { for (view = ISC_LIST_HEAD(server->viewlist); view != NULL; view = ISC_LIST_NEXT(view, link)) { - result = dns_zt_apply(view->zonetable, - isc_rwlocktype_none, false, NULL, + result = dns_zt_apply(view->zonetable, false, NULL, synczone, &cleanup); if (result != ISC_R_SUCCESS && tresult == ISC_R_SUCCESS) { @@ -13430,19 +13418,15 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view, /* Zone shouldn't already exist */ if (redirect) { - result = (view->redirect != NULL) ? ISC_R_SUCCESS - : ISC_R_NOTFOUND; + result = (view->redirect == NULL) ? ISC_R_NOTFOUND + : ISC_R_EXISTS; } else { - result = dns_zt_find(view->zonetable, name, 0, NULL, &zone); + result = dns_view_findzone(view, name, &zone); + if (result == ISC_R_SUCCESS) { + result = ISC_R_EXISTS; + } } - if (result == ISC_R_SUCCESS) { - result = ISC_R_EXISTS; - goto cleanup; - } else if (result == DNS_R_PARTIALMATCH) { - /* Create our sub-zone anyway */ - dns_zone_detach(&zone); - zone = NULL; - } else if (result != ISC_R_NOTFOUND) { + if (result != ISC_R_NOTFOUND) { goto cleanup; } @@ -13501,7 +13485,7 @@ do_addzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view, } dns_zone_attach(view->redirect, &zone); } else { - result = dns_zt_find(view->zonetable, name, 0, NULL, &zone); + result = dns_view_findzone(view, name, &zone); if (result != ISC_R_SUCCESS) { isc_log_write(named_g_lctx, NAMED_LOGCATEGORY_GENERAL, NAMED_LOGMODULE_SERVER, ISC_LOG_ERROR, @@ -13617,7 +13601,7 @@ do_modzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view, result = ISC_R_NOTFOUND; } } else { - result = dns_zt_find(view->zonetable, name, 0, NULL, &zone); + result = dns_view_findzone(view, name, &zone); } if (result != ISC_R_SUCCESS) { goto cleanup; @@ -13686,7 +13670,7 @@ do_modzone(named_server_t *server, ns_cfgctx_t *cfg, dns_view_t *view, } dns_zone_attach(view->redirect, &zone); } else { - CHECK(dns_zt_find(view->zonetable, name, 0, NULL, &zone)); + CHECK(dns_view_findzone(view, name, &zone)); } #ifndef HAVE_LMDB diff --git a/bin/named/statschannel.c b/bin/named/statschannel.c index 7b595ec267..0b9eb6b862 100644 --- a/bin/named/statschannel.c +++ b/bin/named/statschannel.c @@ -1776,8 +1776,8 @@ generatexml(named_server_t *server, uint32_t flags, int *buflen, if ((flags & STATS_XML_ZONES) != 0) { TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "zones")); - CHECK(dns_zt_apply(view->zonetable, isc_rwlocktype_read, - true, NULL, zone_xmlrender, writer)); + CHECK(dns_zt_apply(view->zonetable, true, NULL, + zone_xmlrender, writer)); TRY0(xmlTextWriterEndElement(writer)); /* /zones */ } @@ -2490,9 +2490,8 @@ generatejson(named_server_t *server, size_t *msglen, const char **msg, CHECKMEM(za); if ((flags & STATS_JSON_ZONES) != 0) { - CHECK(dns_zt_apply(view->zonetable, - isc_rwlocktype_read, true, - NULL, zone_jsonrender, za)); + CHECK(dns_zt_apply(view->zonetable, true, NULL, + zone_jsonrender, za)); } if (json_object_array_length(za) != 0) { diff --git a/bin/tests/system/catz/tests.sh b/bin/tests/system/catz/tests.sh index ddab8f84f7..756edb9b18 100644 --- a/bin/tests/system/catz/tests.sh +++ b/bin/tests/system/catz/tests.sh @@ -294,7 +294,7 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "zone_shutdown: zone dom1.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom1.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -929,7 +929,7 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "zone_shutdown: zone dom5.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom5.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -987,7 +987,7 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "zone_shutdown: zone dom6.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom6.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -1483,7 +1483,7 @@ END n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 - wait_for_message ns2/named.run "zone_shutdown: zone ${special}/IN/default: shutting down" || ret=1 + wait_for_message ns2/named.run "catz: catz_delzone_cb: zone '${special}' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -1621,7 +1621,7 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "zone_shutdown: zone dom11.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom11.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -1653,7 +1653,7 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "zone_shutdown: zone subdomain.of.dom11.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'subdomain.of.dom11.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -2424,10 +2424,8 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "catz: deleting zone 'dom17.example' from catalog 'catalog1.example' - success" && -wait_for_message ns2/named.run "catz: deleting zone 'dom18.example' from catalog 'catalog1.example' - success" && -wait_for_message ns2/named.run "zone_shutdown: zone dom17.example/IN/default: shutting down" && -wait_for_message ns2/named.run "zone_shutdown: zone dom18.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom17.example' deleted" && +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom18.example' deleted" && if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) @@ -2515,10 +2513,8 @@ status=$((status+ret)) n=$((n+1)) echo_i "waiting for secondary to sync up ($n)" ret=0 -wait_for_message ns2/named.run "catz: deleting zone 'dom17.example' from catalog 'catalog2.example' - success" && -wait_for_message ns2/named.run "catz: deleting zone 'dom18.example' from catalog 'catalog2.example' - success" && -wait_for_message ns2/named.run "zone_shutdown: zone dom17.example/IN/default: shutting down" && -wait_for_message ns2/named.run "zone_shutdown: zone dom18.example/IN/default: shutting down" || ret=1 +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom17.example' deleted" && +wait_for_message ns2/named.run "catz: catz_delzone_cb: zone 'dom18.example' deleted" || ret=1 if [ $ret -ne 0 ]; then echo_i "failed"; fi status=$((status+ret)) diff --git a/bin/tests/system/dyndb/driver/syncptr.c b/bin/tests/system/dyndb/driver/syncptr.c index 15defbd2cc..270c7acca6 100644 --- a/bin/tests/system/dyndb/driver/syncptr.c +++ b/bin/tests/system/dyndb/driver/syncptr.c @@ -166,7 +166,7 @@ syncptr_find_zone(sample_instance_t *inst, dns_rdata_t *rdata, dns_name_t *name, } /* Find a zone containing owner name of the PTR record. */ - result = dns_zt_find(inst->view->zonetable, name, 0, NULL, zone); + result = dns_zt_find(inst->view->zonetable, name, 0, zone); if (result == DNS_R_PARTIALMATCH) { result = ISC_R_SUCCESS; } else if (result != ISC_R_SUCCESS) { diff --git a/bin/tests/system/pipelined/pipequeries.c b/bin/tests/system/pipelined/pipequeries.c index 805e3c5344..5569ad318d 100644 --- a/bin/tests/system/pipelined/pipequeries.c +++ b/bin/tests/system/pipelined/pipequeries.c @@ -256,7 +256,7 @@ main(int argc, char *argv[]) { RUNCHECK(dns_requestmgr_create(mctx, dispatchmgr, dispatchv4, NULL, &requestmgr)); - RUNCHECK(dns_view_create(mctx, 0, "_test", &view)); + RUNCHECK(dns_view_create(mctx, loopmgr, 0, "_test", &view)); isc_loopmgr_setup(loopmgr, sendqueries, NULL); isc_loopmgr_run(loopmgr); diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c index 98e72ffb2e..4193f06912 100644 --- a/bin/tools/mdig.c +++ b/bin/tools/mdig.c @@ -2141,7 +2141,7 @@ main(int argc, char *argv[]) { mctx, dispatchmgr, have_ipv4 ? dispatchvx : NULL, have_ipv6 ? dispatchvx : NULL, &requestmgr)); - RUNCHECK(dns_view_create(mctx, 0, "_test", &view)); + RUNCHECK(dns_view_create(mctx, loopmgr, 0, "_mdig", &view)); query = ISC_LIST_HEAD(queries); isc_loopmgr_setup(loopmgr, sendqueries, query); diff --git a/fuzz/dns_message_checksig.c b/fuzz/dns_message_checksig.c index 6679df60bd..c27bfebe8b 100644 --- a/fuzz/dns_message_checksig.c +++ b/fuzz/dns_message_checksig.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -85,6 +86,7 @@ static isc_mem_t *mctx = NULL; #define HMACSHA256 "\x0bhmac-sha256" static isc_stdtime_t fuzztime = 0x622acce1; +static isc_loopmgr_t *loopmgr = NULL; static dns_view_t *view = NULL; static dns_tsigkey_t *tsigkey = NULL; static dns_tsig_keyring_t *ring = NULL; @@ -232,7 +234,10 @@ LLVMFuzzerInitialize(int *argc ISC_ATTR_UNUSED, char ***argv ISC_ATTR_UNUSED) { } destroy_dst = true; - result = dns_view_create(mctx, dns_rdataclass_in, "view", &view); + isc_loopmgr_create(mctx, 1, &loopmgr); + + result = dns_view_create(mctx, loopmgr, dns_rdataclass_in, "view", + &view); if (result != ISC_R_SUCCESS) { fprintf(stderr, "dns_view_create failed: %s\n", isc_result_totext(result)); @@ -322,6 +327,7 @@ LLVMFuzzerInitialize(int *argc ISC_ATTR_UNUSED, char ***argv ISC_ATTR_UNUSED) { return (1); } + dns_zone_setview(zone, view); dns_view_freeze(view); dns_zone_detach(&zone); diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 70b160469f..f4b9c962fa 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -526,7 +526,7 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { result = delcur ? isc_ht_iter_delcurrent_next(iter1) : isc_ht_iter_next(iter1)) { - isc_result_t zt_find_result; + isc_result_t find_result; dns_catz_zone_t *parentcatz = NULL; dns_catz_entry_t *nentry = NULL; dns_catz_entry_t *oentry = NULL; @@ -559,10 +559,10 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { &catz->zoneoptions, &nentry->opts); /* Try to find the zone in the view */ - zt_find_result = dns_zt_find(catz->catzs->view->zonetable, - dns_catz_entry_getname(nentry), 0, - NULL, &zone); - if (zt_find_result == ISC_R_SUCCESS) { + find_result = dns_view_findzone(catz->catzs->view, + dns_catz_entry_getname(nentry), + &zone); + if (find_result == ISC_R_SUCCESS) { dns_catz_coo_t *coo = NULL; char pczname[DNS_NAME_FORMATSIZE]; bool parentcatz_locked = false; @@ -606,10 +606,6 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { UNLOCK(&parentcatz->lock); LOCK(&catz->lock); } - } - if (zt_find_result == ISC_R_SUCCESS || - zt_find_result == DNS_R_PARTIALMATCH) - { dns_zone_detach(&zone); } @@ -617,8 +613,7 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { result = isc_ht_find(catz->entries, key, (uint32_t)keysize, (void **)&oentry); if (result != ISC_R_SUCCESS) { - if (zt_find_result == ISC_R_SUCCESS && - parentcatz == catz) + if (find_result == ISC_R_SUCCESS && parentcatz == catz) { /* * This means that the zone's unique label @@ -645,7 +640,7 @@ dns__catz_zones_merge(dns_catz_zone_t *catz, dns_catz_zone_t *newcatz) { continue; } - if (zt_find_result != ISC_R_SUCCESS) { + if (find_result != ISC_R_SUCCESS) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_DEBUG(3), "catz: zone '%s' was expected to exist " diff --git a/lib/dns/client.c b/lib/dns/client.c index 579b9c3aab..5a3c0fadda 100644 --- a/lib/dns/client.c +++ b/lib/dns/client.c @@ -206,7 +206,8 @@ createview(isc_mem_t *mctx, dns_rdataclass_t rdclass, isc_loopmgr_t *loopmgr, isc_result_t result; dns_view_t *view = NULL; - result = dns_view_create(mctx, rdclass, DNS_CLIENTVIEW_NAME, &view); + result = dns_view_create(mctx, loopmgr, rdclass, DNS_CLIENTVIEW_NAME, + &view); if (result != ISC_R_SUCCESS) { return (result); } diff --git a/lib/dns/include/dns/view.h b/lib/dns/include/dns/view.h index a552327bad..b72a3dd44f 100644 --- a/lib/dns/include/dns/view.h +++ b/lib/dns/include/dns/view.h @@ -259,8 +259,8 @@ struct dns_view { #endif /* HAVE_LMDB */ isc_result_t -dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, - dns_view_t **viewp); +dns_view_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, + dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp); /*%< * Create a view. * @@ -364,24 +364,6 @@ dns_view_weakdetach(dns_view_t **targetp); *\li *viewp is NULL. */ -isc_result_t -dns_view_createzonetable(dns_view_t *view); -/*%< - * Create a zonetable for the view. - * - * Requires: - * - *\li 'view' is a valid, unfrozen view. - * - *\li 'view' does not have a zonetable already. - * - * Returns: - * - *\li #ISC_R_SUCCESS - * - *\li Any error that dns_zt_create() can return. - */ - isc_result_t dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *netmgr, @@ -782,14 +764,13 @@ dns_view_findzone(dns_view_t *view, const dns_name_t *name, dns_zone_t **zonep); * Returns: *\li #ISC_R_SUCCESS A matching zone was found. *\li #ISC_R_NOTFOUND No matching zone was found. - *\li others An error occurred. */ isc_result_t dns_view_load(dns_view_t *view, bool stop, bool newonly); isc_result_t -dns_view_asyncload(dns_view_t *view, bool newonly, dns_zt_allloaded_t callback, +dns_view_asyncload(dns_view_t *view, bool newonly, dns_zt_callback_t *callback, void *arg); /*%< * Load zones attached to this view. dns_view_load() loads diff --git a/lib/dns/include/dns/zone.h b/lib/dns/include/dns/zone.h index 0c71cfceaa..db67203797 100644 --- a/lib/dns/include/dns/zone.h +++ b/lib/dns/include/dns/zone.h @@ -424,15 +424,14 @@ dns_zone_loadandthaw(dns_zone_t *zone); */ isc_result_t -dns_zone_asyncload(dns_zone_t *zone, bool newonly, dns_zt_zoneloaded_t done, +dns_zone_asyncload(dns_zone_t *zone, bool newonly, dns_zt_callback_t done, void *arg); /*%< * Cause the database to be loaded from its backing store asynchronously. * Other zone maintenance functions are suspended until this is complete. * When finished, 'done' is called to inform the caller, with 'arg' as - * its first argument and 'zone' as its second. (Normally, 'arg' is - * expected to point to the zone table but is left undefined for testing - * purposes.) + * its argument. (Normally, 'arg' is expected to point to the zone table + * but is left undefined for testing purposes.) * * Require: *\li 'zone' to be a valid zone. diff --git a/lib/dns/include/dns/zt.h b/lib/dns/include/dns/zt.h index afe13ead85..1118d06428 100644 --- a/lib/dns/include/dns/zt.h +++ b/lib/dns/include/dns/zt.h @@ -22,35 +22,37 @@ #include -#define DNS_ZTFIND_NOEXACT 0x01 -#define DNS_ZTFIND_MIRROR 0x02 - ISC_LANG_BEGINDECLS -typedef isc_result_t (*dns_zt_allloaded_t)(void *arg); -/*%< - * Method prototype: when all pending zone loads are complete, - * the zone table can inform the caller via a callback function with - * this signature. - */ +typedef enum dns_ztfind { + DNS_ZTFIND_EXACT = 1 << 0, + DNS_ZTFIND_NOEXACT = 1 << 1, + DNS_ZTFIND_MIRROR = 1 << 2, +} dns_ztfind_t; -typedef isc_result_t (*dns_zt_zoneloaded_t)(dns_zt_t *zt, dns_zone_t *zone); -/*%< - * Method prototype: when a zone finishes loading, the zt object - * can be informed via a callback function with this signature. - */ +typedef isc_result_t +dns_zt_callback_t(void *arg); -isc_result_t -dns_zt_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, dns_zt_t **zt); +void +dns_zt_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, dns_view_t *view, + dns_zt_t **ztp); /*%< - * Creates a new zone table. + * Creates a new zone table for a view. * * Requires: * \li 'mctx' to be initialized. + * \li 'view' is non-NULL + * \li 'ztp' is non-NULL + * \li '*ztp' is NULL + */ + +void +dns_zt_compact(dns_zt_t *zt); +/*%< + * Reclaim unused memory in the zone table * - * Returns: - * \li #ISC_R_SUCCESS on success. - * \li #ISC_R_NOMEMORY + * Requires: + * \li 'zt' to be valid */ isc_result_t @@ -65,8 +67,6 @@ dns_zt_mount(dns_zt_t *zt, dns_zone_t *zone); * Returns: * \li #ISC_R_SUCCESS * \li #ISC_R_EXISTS - * \li #ISC_R_NOSPACE - * \li #ISC_R_NOMEMORY */ isc_result_t @@ -75,37 +75,38 @@ dns_zt_unmount(dns_zt_t *zt, dns_zone_t *zone); * Unmount the given zone from the table. * * Requires: - * 'zt' to be valid + * 'zt' to be valid * \li 'zone' to be valid * * Returns: * \li #ISC_R_SUCCESS * \li #ISC_R_NOTFOUND - * \li #ISC_R_NOMEMORY */ isc_result_t -dns_zt_find(dns_zt_t *zt, const dns_name_t *name, unsigned int options, - dns_name_t *foundname, dns_zone_t **zone); +dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, + dns_zone_t **zone); /*%< - * Find the best match for 'name' in 'zt'. If foundname is non NULL - * then the name of the zone found is returned. + * Find the best match for 'name' in 'zt'. * * Notes: - * \li If the DNS_ZTFIND_NOEXACT is set, the best partial match (if any) - * to 'name' will be returned. + * \li If the DNS_ZTFIND_EXACT option is set, only an exact match is + * returned. + * + * \li If the DNS_ZTFIND_NOEXACT option is set, the closest matching + * parent domain is returned, even when there is an exact match + * in the tree. * * Requires: * \li 'zt' to be valid * \li 'name' to be valid - * \li 'foundname' to be initialized and associated with a fixedname or NULL * \li 'zone' to be non NULL and '*zone' to be NULL + * \li DNS_ZTFIND_EXACT and DNS_ZTFIND_NOEXACT are not both set * * Returns: * \li #ISC_R_SUCCESS - * \li #DNS_R_PARTIALMATCH + * \li #DNS_R_PARTIALMATCH (if DNS_ZTFIND_EXACT is not set) * \li #ISC_R_NOTFOUND - * \li #ISC_R_NOSPACE */ void @@ -142,14 +143,14 @@ isc_result_t dns_zt_load(dns_zt_t *zt, bool stop, bool newonly); isc_result_t -dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_allloaded_t alldone, +dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_callback_t alldone, void *arg); /*%< - * Load all zones in the table. If 'stop' is true, - * stop on the first error and return it. If 'stop' - * is false, ignore errors. + * Load all zones in the table. If 'stop' is true, stop on the first + * error and return it. If 'stop' is false, ignore errors. + * + * If newonly is set only zones that were never loaded are loaded. * - * if newonly is set only zones that were never loaded are loaded. * dns_zt_asyncload() loads zones asynchronously; when all * zones in the zone table have finished loaded (or failed due * to errors), the caller is informed by calling 'alldone' @@ -168,7 +169,7 @@ dns_zt_freezezones(dns_zt_t *zt, dns_view_t *view, bool freeze); */ isc_result_t -dns_zt_apply(dns_zt_t *zt, isc_rwlocktype_t lock, bool stop, isc_result_t *sub, +dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, isc_result_t (*action)(dns_zone_t *, void *), void *uap); /*%< * Apply a given 'action' to all zone zones in the table. diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index dab6b404ed..194caf6539 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6544,9 +6544,8 @@ static inline bool name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { isc_result_t result; dns_forwarders_t *forwarders = NULL; - dns_fixedname_t fixed, zfixed; + dns_fixedname_t fixed; dns_name_t *fname = dns_fixedname_initname(&fixed); - dns_name_t *zfname = dns_fixedname_initname(&zfixed); dns_name_t *apex = NULL; dns_name_t suffix; dns_zone_t *zone = NULL; @@ -6584,25 +6583,17 @@ name_external(const dns_name_t *name, dns_rdatatype_t type, fetchctx_t *fctx) { * If there is a locally served zone between 'apex' and 'name' * then don't cache. */ - LOCK(&fctx->res->view->lock); - if (fctx->res->view->zonetable != NULL) { - unsigned int options = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_MIRROR; - result = dns_zt_find(fctx->res->view->zonetable, name, options, - zfname, &zone); - if (zone != NULL) { - dns_zone_detach(&zone); - } - if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { - if (dns_name_fullcompare(zfname, apex, &(int){ 0 }, - &(unsigned int){ 0U }) == - dns_namereln_subdomain) - { - UNLOCK(&fctx->res->view->lock); - return (true); - } + dns_ztfind_t options = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_MIRROR; + result = dns_zt_find(fctx->res->view->zonetable, name, options, &zone); + if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { + dns_name_t *zname = dns_zone_getorigin(zone); + dns_namereln_t reln = dns_name_fullcompare( + zname, apex, &(int){ 0 }, &(unsigned int){ 0U }); + dns_zone_detach(&zone); + if (reln == dns_namereln_subdomain) { + return (true); } } - UNLOCK(&fctx->res->view->lock); /* * Look for a forward declaration below 'name'. diff --git a/lib/dns/view.c b/lib/dns/view.c index 133d6775bb..5f58142627 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -79,7 +79,8 @@ #define DEFAULT_EDNS_BUFSIZE 1232 isc_result_t -dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, +dns_view_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, + dns_rdataclass_t rdclass, const char *name, dns_view_t **viewp) { dns_view_t *view = NULL; isc_result_t result; @@ -134,13 +135,7 @@ dns_view_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, const char *name, isc_rwlock_init(&view->sfd_lock); view->zonetable = NULL; - result = dns_zt_create(mctx, rdclass, &view->zonetable); - if (result != ISC_R_SUCCESS) { - UNEXPECTED_ERROR("dns_zt_create() failed: %s", - isc_result_totext(result)); - result = ISC_R_UNEXPECTED; - goto cleanup_mutex; - } + dns_zt_create(mctx, loopmgr, view, &view->zonetable); result = dns_fwdtable_create(mctx, &view->fwdtable); if (result != ISC_R_SUCCESS) { @@ -214,11 +209,8 @@ cleanup_weakrefs: } cleanup_zt: - if (view->zonetable != NULL) { - dns_zt_detach(&view->zonetable); - } + dns_zt_detach(&view->zonetable); -cleanup_mutex: isc_rwlock_destroy(&view->sfd_lock); isc_mutex_destroy(&view->lock); @@ -572,8 +564,7 @@ dns_view_dialup(dns_view_t *view) { REQUIRE(DNS_VIEW_VALID(view)); REQUIRE(view->zonetable != NULL); - (void)dns_zt_apply(view->zonetable, isc_rwlocktype_read, false, NULL, - dialup, NULL); + (void)dns_zt_apply(view->zonetable, false, NULL, dialup, NULL); } void @@ -602,15 +593,6 @@ dns_view_weakdetach(dns_view_t **viewp) { } } -isc_result_t -dns_view_createzonetable(dns_view_t *view) { - REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(!view->frozen); - REQUIRE(view->zonetable == NULL); - - return (dns_zt_create(view->mctx, view->rdclass, &view->zonetable)); -} - isc_result_t dns_view_createresolver(dns_view_t *view, isc_loopmgr_t *loopmgr, unsigned int ndisp, isc_nm_t *netmgr, @@ -784,7 +766,6 @@ dns_view_addzone(dns_view_t *view, dns_zone_t *zone) { REQUIRE(DNS_VIEW_VALID(view)); REQUIRE(!view->frozen); - REQUIRE(view->zonetable != NULL); result = dns_zt_mount(view->zonetable, zone); @@ -794,23 +775,9 @@ dns_view_addzone(dns_view_t *view, dns_zone_t *zone) { isc_result_t dns_view_findzone(dns_view_t *view, const dns_name_t *name, dns_zone_t **zonep) { - isc_result_t result; - REQUIRE(DNS_VIEW_VALID(view)); - LOCK(&view->lock); - if (view->zonetable != NULL) { - result = dns_zt_find(view->zonetable, name, 0, NULL, zonep); - if (result == DNS_R_PARTIALMATCH) { - dns_zone_detach(zonep); - result = ISC_R_NOTFOUND; - } - } else { - result = ISC_R_NOTFOUND; - } - UNLOCK(&view->lock); - - return (result); + return (dns_zt_find(view->zonetable, name, DNS_ZTFIND_EXACT, zonep)); } isc_result_t @@ -820,11 +787,11 @@ dns_view_find(dns_view_t *view, const dns_name_t *name, dns_rdatatype_t type, dns_name_t *foundname, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset) { isc_result_t result; - dns_db_t *db, *zdb; - dns_dbnode_t *node, *znode; + dns_db_t *db = NULL, *zdb = NULL; + dns_dbnode_t *node = NULL, *znode = NULL; bool is_cache, is_staticstub_zone; dns_rdataset_t zrdataset, zsigrdataset; - dns_zone_t *zone; + dns_zone_t *zone = NULL; /* * Find an rdataset whose owner name is 'name', and whose type is @@ -842,24 +809,12 @@ dns_view_find(dns_view_t *view, const dns_name_t *name, dns_rdatatype_t type, */ dns_rdataset_init(&zrdataset); dns_rdataset_init(&zsigrdataset); - zdb = NULL; - znode = NULL; /* * Find a database to answer the query. */ - db = NULL; - node = NULL; is_staticstub_zone = false; - zone = NULL; - LOCK(&view->lock); - if (view->zonetable != NULL) { - result = dns_zt_find(view->zonetable, name, DNS_ZTFIND_MIRROR, - NULL, &zone); - } else { - result = ISC_R_NOTFOUND; - } - UNLOCK(&view->lock); + result = dns_zt_find(view->zonetable, name, DNS_ZTFIND_MIRROR, &zone); if (zone != NULL && dns_zone_gettype(zone) == dns_zone_staticstub && !use_static_stub) { @@ -1109,10 +1064,10 @@ dns_view_findzonecut(dns_view_t *view, const dns_name_t *name, unsigned int options, bool use_hints, bool use_cache, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset) { isc_result_t result; - dns_db_t *db; - bool is_cache, use_zone, try_hints; - dns_zone_t *zone; - dns_name_t *zfname; + dns_db_t *db = NULL; + bool is_cache, use_zone = false, try_hints = false; + dns_zone_t *zone = NULL; + dns_name_t *zfname = NULL; dns_rdataset_t zrdataset, zsigrdataset; dns_fixedname_t zfixedname; unsigned int ztoptions = DNS_ZTFIND_MIRROR; @@ -1120,11 +1075,6 @@ dns_view_findzonecut(dns_view_t *view, const dns_name_t *name, REQUIRE(DNS_VIEW_VALID(view)); REQUIRE(view->frozen); - db = NULL; - use_zone = false; - try_hints = false; - zfname = NULL; - /* * Initialize. */ @@ -1135,18 +1085,10 @@ dns_view_findzonecut(dns_view_t *view, const dns_name_t *name, /* * Find the right database. */ - zone = NULL; - LOCK(&view->lock); - if (view->zonetable != NULL) { - if ((options & DNS_DBFIND_NOEXACT) != 0) { - ztoptions |= DNS_ZTFIND_NOEXACT; - } - result = dns_zt_find(view->zonetable, name, ztoptions, NULL, - &zone); - } else { - result = ISC_R_NOTFOUND; + if ((options & DNS_DBFIND_NOEXACT) != 0) { + ztoptions |= DNS_ZTFIND_NOEXACT; } - UNLOCK(&view->lock); + result = dns_zt_find(view->zonetable, name, ztoptions, &zone); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { result = dns_zone_getdb(zone, &db); } @@ -1339,7 +1281,6 @@ dns_viewlist_findzone(dns_viewlist_t *list, const dns_name_t *name, dns_view_t *view; isc_result_t result; dns_zone_t *zone1 = NULL, *zone2 = NULL; - dns_zone_t **zp = NULL; REQUIRE(list != NULL); REQUIRE(zonep != NULL && *zonep == NULL); @@ -1350,30 +1291,9 @@ dns_viewlist_findzone(dns_viewlist_t *list, const dns_name_t *name, if (!allclasses && view->rdclass != rdclass) { continue; } - - /* - * If the zone is defined in more than one view, - * treat it as not found. - */ - zp = (zone1 == NULL) ? &zone1 : &zone2; - LOCK(&view->lock); - if (view->zonetable != NULL) { - result = dns_zt_find(view->zonetable, name, 0, NULL, - zp); - } else { - result = ISC_R_NOTFOUND; - } - UNLOCK(&view->lock); - INSIST(result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND || - result == DNS_R_PARTIALMATCH); - - /* Treat a partial match as no match */ - if (result == DNS_R_PARTIALMATCH) { - dns_zone_detach(zp); - result = ISC_R_NOTFOUND; - POST(result); - } - + result = dns_zt_find(view->zonetable, name, DNS_ZTFIND_EXACT, + (zone1 == NULL) ? &zone1 : &zone2); + INSIST(result == ISC_R_SUCCESS || result == ISC_R_NOTFOUND); if (zone2 != NULL) { dns_zone_detach(&zone1); dns_zone_detach(&zone2); @@ -1393,17 +1313,13 @@ dns_viewlist_findzone(dns_viewlist_t *list, const dns_name_t *name, isc_result_t dns_view_load(dns_view_t *view, bool stop, bool newonly) { REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(view->zonetable != NULL); - return (dns_zt_load(view->zonetable, stop, newonly)); } isc_result_t -dns_view_asyncload(dns_view_t *view, bool newonly, dns_zt_allloaded_t callback, +dns_view_asyncload(dns_view_t *view, bool newonly, dns_zt_callback_t *callback, void *arg) { REQUIRE(DNS_VIEW_VALID(view)); - REQUIRE(view->zonetable != NULL); - return (dns_zt_asyncload(view->zonetable, newonly, callback, arg)); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 2e31a82963..5dd59da646 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -781,7 +781,7 @@ struct dns_nsfetch { struct dns_asyncload { dns_zone_t *zone; unsigned int flags; - dns_zt_zoneloaded_t loaded; + dns_zt_callback_t *loaded; void *loaded_arg; }; @@ -2371,7 +2371,7 @@ zone_asyncload(void *arg) { /* Inform the zone table we've finished loading */ if (asl->loaded != NULL) { - (asl->loaded)(asl->loaded_arg, zone); + asl->loaded(asl->loaded_arg); } isc_mem_put(zone->mctx, asl, sizeof(*asl)); @@ -2379,7 +2379,7 @@ zone_asyncload(void *arg) { } isc_result_t -dns_zone_asyncload(dns_zone_t *zone, bool newonly, dns_zt_zoneloaded_t done, +dns_zone_asyncload(dns_zone_t *zone, bool newonly, dns_zt_callback_t *done, void *arg) { dns_asyncload_t *asl = NULL; @@ -5617,15 +5617,7 @@ zone_destroy(dns_zone_t *zone) { * This zone is unmanaged; we're probably running in * named-checkzone or a unit test. There's no loop, so we * need to free it immediately. - * - * Unmanaged zones must not have null views; we have no way - * of detaching from the view here without causing deadlock - * because this code is called with the view already - * locked. */ - INSIST(isc_tid() == ISC_TID_UNKNOWN); - INSIST(zone->view == NULL); - zone_shutdown(zone); } else { /* diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 9cfe366ba1..cf91f7285b 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -22,38 +22,35 @@ #include #include #include +#include #include #include #include -#include +#include #include #include #include #include -struct zt_load_params { - dns_zt_zoneloaded_t dl; - bool newonly; -}; +#define ZTMAGIC ISC_MAGIC('Z', 'T', 'b', 'l') +#define VALID_ZT(zt) ISC_MAGIC_VALID(zt, ZTMAGIC) struct dns_zt { - /* Unlocked. */ unsigned int magic; isc_mem_t *mctx; - dns_rdataclass_t rdclass; - isc_rwlock_t rwlock; - dns_zt_allloaded_t loaddone; - void *loaddone_arg; - struct zt_load_params *loadparams; + dns_qpmulti_t *multi; - /* Atomic */ atomic_bool flush; isc_refcount_t references; isc_refcount_t loads_pending; +}; - /* Locked by lock. */ - dns_rbt_t *table; +struct zt_load_params { + dns_zt_t *zt; + dns_zt_callback_t *loaddone; + void *loaddone_arg; + bool newonly; }; struct zt_freeze_params { @@ -61,77 +58,93 @@ struct zt_freeze_params { bool freeze; }; -#define ZTMAGIC ISC_MAGIC('Z', 'T', 'b', 'l') -#define VALID_ZT(zt) ISC_MAGIC_VALID(zt, ZTMAGIC) +static void +ztqpattach(void *uctx ISC_ATTR_UNUSED, void *pval, + uint32_t ival ISC_ATTR_UNUSED) { + dns_zone_t *zone = pval; + dns_zone_ref(zone); +} static void -auto_detach(void *, void *); +ztqpdetach(void *uctx ISC_ATTR_UNUSED, void *pval, + uint32_t ival ISC_ATTR_UNUSED) { + dns_zone_t *zone = pval; + dns_zone_detach(&zone); +} -static isc_result_t -load(dns_zone_t *zone, void *uap); +static size_t +ztqpmakekey(dns_qpkey_t key, void *uctx ISC_ATTR_UNUSED, void *pval, + uint32_t ival ISC_ATTR_UNUSED) { + dns_zone_t *zone = pval; + dns_name_t *name = dns_zone_getorigin(zone); + return (dns_qpkey_fromname(key, name)); +} -static isc_result_t -asyncload(dns_zone_t *zone, void *callback); +static void +ztqptriename(void *uctx, char *buf, size_t size) { + dns_view_t *view = uctx; + snprintf(buf, size, "view %s zone table", view->name); +} -static isc_result_t -freezezones(dns_zone_t *zone, void *uap); +static dns_qpmethods_t ztqpmethods = { + ztqpattach, + ztqpdetach, + ztqpmakekey, + ztqptriename, +}; -static isc_result_t -doneloading(dns_zt_t *zt, dns_zone_t *zone); - -isc_result_t -dns_zt_create(isc_mem_t *mctx, dns_rdataclass_t rdclass, dns_zt_t **ztp) { - dns_zt_t *zt; - isc_result_t result; +void +dns_zt_create(isc_mem_t *mctx, isc_loopmgr_t *loopmgr, dns_view_t *view, + dns_zt_t **ztp) { + dns_qpmulti_t *multi = NULL; + dns_zt_t *zt = NULL; REQUIRE(ztp != NULL && *ztp == NULL); + REQUIRE(view != NULL); + + dns_qpmulti_create(mctx, loopmgr, &ztqpmethods, view, &multi); zt = isc_mem_get(mctx, sizeof(*zt)); + *zt = (dns_zt_t){ + .magic = ZTMAGIC, + .multi = multi, + .references = 1, + }; - zt->table = NULL; - result = dns_rbt_create(mctx, auto_detach, zt, &zt->table); - if (result != ISC_R_SUCCESS) { - goto cleanup_zt; - } - - isc_rwlock_init(&zt->rwlock); - zt->mctx = NULL; isc_mem_attach(mctx, &zt->mctx); - isc_refcount_init(&zt->references, 1); - atomic_init(&zt->flush, false); - zt->rdclass = rdclass; - zt->magic = ZTMAGIC; - zt->loaddone = NULL; - zt->loaddone_arg = NULL; - zt->loadparams = NULL; - isc_refcount_init(&zt->loads_pending, 0); + *ztp = zt; +} - return (ISC_R_SUCCESS); +/* + * XXXFANF it isn't clear whether this function will be useful. There + * is only one zone table per view, so it is probably enough to let + * the qp-trie auto-GC do its thing. However it might be problematic + * if a very large zone is replaced, and its database memory is + * retained for a long time. + */ +void +dns_zt_compact(dns_zt_t *zt) { + dns_qp_t *qp = NULL; -cleanup_zt: - isc_mem_put(mctx, zt, sizeof(*zt)); + REQUIRE(VALID_ZT(zt)); - return (result); + dns_qpmulti_write(zt->multi, &qp); + dns_qp_compact(qp, DNS_QPGC_ALL); + dns_qpmulti_commit(zt->multi, &qp); } isc_result_t dns_zt_mount(dns_zt_t *zt, dns_zone_t *zone) { isc_result_t result; - dns_name_t *name = NULL; + dns_qp_t *qp = NULL; REQUIRE(VALID_ZT(zt)); - name = dns_zone_getorigin(zone); - - RWLOCK(&zt->rwlock, isc_rwlocktype_write); - - result = dns_rbt_addname(zt->table, name, zone); - if (result == ISC_R_SUCCESS) { - dns_zone_ref(zone); - } - - RWUNLOCK(&zt->rwlock, isc_rwlocktype_write); + dns_qpmulti_write(zt->multi, &qp); + result = dns_qp_insert(qp, zone, 0); + dns_qp_compact(qp, DNS_QPGC_MAYBE); + dns_qpmulti_commit(zt->multi, &qp); return (result); } @@ -139,39 +152,48 @@ dns_zt_mount(dns_zt_t *zt, dns_zone_t *zone) { isc_result_t dns_zt_unmount(dns_zt_t *zt, dns_zone_t *zone) { isc_result_t result; - dns_name_t *name; + dns_qp_t *qp = NULL; REQUIRE(VALID_ZT(zt)); - name = dns_zone_getorigin(zone); - - RWLOCK(&zt->rwlock, isc_rwlocktype_write); - - result = dns_rbt_deletename(zt->table, name, false); - - RWUNLOCK(&zt->rwlock, isc_rwlocktype_write); + dns_qpmulti_write(zt->multi, &qp); + result = dns_qp_deletename(qp, dns_zone_getorigin(zone)); + dns_qp_compact(qp, DNS_QPGC_MAYBE); + dns_qpmulti_commit(zt->multi, &qp); return (result); } isc_result_t -dns_zt_find(dns_zt_t *zt, const dns_name_t *name, unsigned int options, - dns_name_t *foundname, dns_zone_t **zonep) { +dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, + dns_zone_t **zonep) { isc_result_t result; - dns_zone_t *dummy = NULL; - unsigned int rbtoptions = 0; + dns_qpread_t qpr; + void *pval = NULL; + uint32_t ival; + dns_ztfind_t exactmask = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_EXACT; + dns_ztfind_t exactopts = options & exactmask; REQUIRE(VALID_ZT(zt)); + REQUIRE(exactopts != exactmask); - if ((options & DNS_ZTFIND_NOEXACT) != 0) { - rbtoptions |= DNS_RBTFIND_NOEXACT; + if (isc_tid() == ISC_TID_UNKNOWN) { + dns_qpmulti_lockedread(zt->multi, &qpr); + } else { + dns_qpmulti_query(zt->multi, &qpr); } + if (exactopts == DNS_ZTFIND_EXACT) { + result = dns_qp_getname(&qpr, name, &pval, &ival); + } else if (exactopts == DNS_ZTFIND_NOEXACT) { + result = dns_qp_findname_parent(&qpr, name, DNS_QPFIND_NOEXACT, + &pval, &ival); + } else { + result = dns_qp_findname_parent(&qpr, name, 0, &pval, &ival); + } + dns_qpread_destroy(zt->multi, &qpr); - RWLOCK(&zt->rwlock, isc_rwlocktype_read); - - result = dns_rbt_findname(zt->table, name, rbtoptions, foundname, - (void **)&dummy); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { + dns_zone_t *zone = pval; /* * If DNS_ZTFIND_MIRROR is set and the zone which was * determined to be the deepest match for the supplied name is @@ -190,17 +212,15 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, unsigned int options, * arguably not worth the added complexity. */ if ((options & DNS_ZTFIND_MIRROR) != 0 && - dns_zone_gettype(dummy) == dns_zone_mirror && - !dns_zone_isloaded(dummy)) + dns_zone_gettype(zone) == dns_zone_mirror && + !dns_zone_isloaded(zone)) { result = ISC_R_NOTFOUND; } else { - dns_zone_attach(dummy, zonep); + dns_zone_attach(zone, zonep); } } - RWUNLOCK(&zt->rwlock, isc_rwlocktype_read); - return (result); } @@ -226,12 +246,10 @@ zt_destroy(dns_zt_t *zt) { isc_refcount_destroy(&zt->loads_pending); if (atomic_load_acquire(&zt->flush)) { - (void)dns_zt_apply(zt, isc_rwlocktype_none, false, NULL, flush, - NULL); + (void)dns_zt_apply(zt, false, NULL, flush, NULL); } - dns_rbt_destroy(&zt->table); - isc_rwlock_destroy(&zt->rwlock); + dns_qpmulti_destroy(&zt->multi); zt->magic = 0; isc_mem_putanddetach(&zt->mctx, zt, sizeof(*zt)); } @@ -256,22 +274,10 @@ dns_zt_flush(dns_zt_t *zt) { atomic_store_release(&zt->flush, true); } -isc_result_t -dns_zt_load(dns_zt_t *zt, bool stop, bool newonly) { - isc_result_t result; - struct zt_load_params params; - REQUIRE(VALID_ZT(zt)); - params.newonly = newonly; - result = dns_zt_apply(zt, isc_rwlocktype_read, stop, NULL, load, - ¶ms); - return (result); -} - static isc_result_t -load(dns_zone_t *zone, void *paramsv) { +load(dns_zone_t *zone, void *uap) { isc_result_t result; - struct zt_load_params *params = (struct zt_load_params *)paramsv; - result = dns_zone_load(zone, params->newonly); + result = dns_zone_load(zone, uap != NULL); if (result == DNS_R_CONTINUE || result == DNS_R_UPTODATE || result == DNS_R_DYNAMIC) { @@ -280,71 +286,41 @@ load(dns_zone_t *zone, void *paramsv) { return (result); } -static void -call_loaddone(dns_zt_t *zt) { - dns_zt_allloaded_t loaddone = zt->loaddone; - void *loaddone_arg = zt->loaddone_arg; - - /* - * Set zt->loaddone, zt->loaddone_arg and zt->loadparams to NULL - * before calling loaddone. - */ - zt->loaddone = NULL; - zt->loaddone_arg = NULL; - - isc_mem_put(zt->mctx, zt->loadparams, sizeof(struct zt_load_params)); - zt->loadparams = NULL; - - /* - * Call the callback last. - */ - if (loaddone != NULL) { - loaddone(loaddone_arg); - } +isc_result_t +dns_zt_load(dns_zt_t *zt, bool stop, bool newonly) { + REQUIRE(VALID_ZT(zt)); + return (dns_zt_apply(zt, stop, NULL, load, newonly ? &newonly : NULL)); } -isc_result_t -dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_allloaded_t alldone, - void *arg) { - isc_result_t result; - uint_fast32_t loads_pending; +static void +loaded_all(struct zt_load_params *params) { + if (params->loaddone != NULL) { + params->loaddone(params->loaddone_arg); + } + isc_mem_put(params->zt->mctx, params, sizeof(*params)); +} + +/* + * Decrement the loads_pending counter; when counter reaches + * zero, call the loaddone callback that was initially set by + * dns_zt_asyncload(). + */ +static isc_result_t +loaded_one(void *uap) { + struct zt_load_params *params = uap; + dns_zt_t *zt = params->zt; REQUIRE(VALID_ZT(zt)); - /* - * Obtain a reference to zt->loads_pending so that asyncload can - * safely decrement both zt->references and zt->loads_pending - * without going to zero. - */ - loads_pending = isc_refcount_increment0(&zt->loads_pending); - INSIST(loads_pending == 0); - - /* - * Only one dns_zt_asyncload call at a time should be active so - * these pointers should be NULL. They are set back to NULL - * before the zt->loaddone (alldone) is called in call_loaddone. - */ - INSIST(zt->loadparams == NULL); - INSIST(zt->loaddone == NULL); - INSIST(zt->loaddone_arg == NULL); - - zt->loadparams = isc_mem_get(zt->mctx, sizeof(struct zt_load_params)); - zt->loadparams->dl = doneloading; - zt->loadparams->newonly = newonly; - zt->loaddone = alldone; - zt->loaddone_arg = arg; - - result = dns_zt_apply(zt, isc_rwlocktype_read, false, NULL, asyncload, - zt); - - /* - * Have all the loads completed? - */ if (isc_refcount_decrement(&zt->loads_pending) == 1) { - call_loaddone(zt); + loaded_all(params); } - return (result); + if (isc_refcount_decrement(&zt->references) == 1) { + zt_destroy(zt); + } + + return (ISC_R_SUCCESS); } /* @@ -353,16 +329,18 @@ dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_allloaded_t alldone, * the zone loading is complete. */ static isc_result_t -asyncload(dns_zone_t *zone, void *zt_) { +asyncload(dns_zone_t *zone, void *uap) { + struct zt_load_params *params = uap; + struct dns_zt *zt = params->zt; isc_result_t result; - struct dns_zt *zt = (dns_zt_t *)zt_; + + REQUIRE(VALID_ZT(zt)); REQUIRE(zone != NULL); isc_refcount_increment(&zt->references); isc_refcount_increment(&zt->loads_pending); - result = dns_zone_asyncload(zone, zt->loadparams->newonly, - *zt->loadparams->dl, zt); + result = dns_zone_asyncload(zone, params->newonly, loaded_one, params); if (result != ISC_R_SUCCESS) { /* * Caller is holding a reference to zt->loads_pending @@ -375,18 +353,40 @@ asyncload(dns_zone_t *zone, void *zt_) { } isc_result_t -dns_zt_freezezones(dns_zt_t *zt, dns_view_t *view, bool freeze) { - isc_result_t result, tresult; - struct zt_freeze_params params = { view, freeze }; +dns_zt_asyncload(dns_zt_t *zt, bool newonly, dns_zt_callback_t *loaddone, + void *arg) { + isc_result_t result; + uint_fast32_t loads_pending; + struct zt_load_params *params = NULL; REQUIRE(VALID_ZT(zt)); - result = dns_zt_apply(zt, isc_rwlocktype_read, false, &tresult, - freezezones, ¶ms); - if (tresult == ISC_R_NOTFOUND) { - tresult = ISC_R_SUCCESS; + /* + * Obtain a reference to zt->loads_pending so that asyncload can + * safely decrement both zt->references and zt->loads_pending + * without going to zero. + */ + loads_pending = isc_refcount_increment0(&zt->loads_pending); + INSIST(loads_pending == 0); + + params = isc_mem_get(zt->mctx, sizeof(*params)); + *params = (struct zt_load_params){ + .zt = zt, + .newonly = newonly, + .loaddone = loaddone, + .loaddone_arg = arg, + }; + + result = dns_zt_apply(zt, false, NULL, asyncload, params); + + /* + * Have all the loads completed? + */ + if (isc_refcount_decrement(&zt->loads_pending) == 1) { + loaded_all(params); } - return ((result == ISC_R_SUCCESS) ? tresult : result); + + return (result); } static isc_result_t @@ -447,8 +447,8 @@ freezezones(dns_zone_t *zone, void *uap) { } } view = dns_zone_getview(zone); - if (strcmp(view->name, "_bind") == 0 || strcmp(view->name, "_defaul" - "t") == 0) + if (strcmp(view->name, "_bind") == 0 || + strcmp(view->name, "_default") == 0) { vname = ""; sep = ""; @@ -470,143 +470,70 @@ freezezones(dns_zone_t *zone, void *uap) { return (result); } -void -dns_zt_setviewcommit(dns_zt_t *zt) { - dns_rbtnode_t *node; - dns_rbtnodechain_t chain; - isc_result_t result; +isc_result_t +dns_zt_freezezones(dns_zt_t *zt, dns_view_t *view, bool freeze) { + isc_result_t result, tresult; + struct zt_freeze_params params = { view, freeze }; REQUIRE(VALID_ZT(zt)); - RWLOCK(&zt->rwlock, isc_rwlocktype_read); - dns_rbtnodechain_init(&chain); - - result = dns_rbtnodechain_first(&chain, zt->table, NULL, NULL); - while (result == DNS_R_NEWORIGIN || result == ISC_R_SUCCESS) { - result = dns_rbtnodechain_current(&chain, NULL, NULL, &node); - if (result == ISC_R_SUCCESS && node->data != NULL) { - dns_zone_setviewcommit(node->data); - } - - result = dns_rbtnodechain_next(&chain, NULL, NULL); + result = dns_zt_apply(zt, false, &tresult, freezezones, ¶ms); + if (tresult == ISC_R_NOTFOUND) { + tresult = ISC_R_SUCCESS; } + return ((result == ISC_R_SUCCESS) ? tresult : result); +} - dns_rbtnodechain_invalidate(&chain); - RWUNLOCK(&zt->rwlock, isc_rwlocktype_read); +typedef void +setview_cb(dns_zone_t *zone); + +static isc_result_t +setview(dns_zone_t *zone, void *arg) { + setview_cb *cb = arg; + cb(zone); + return (ISC_R_SUCCESS); +} + +void +dns_zt_setviewcommit(dns_zt_t *zt) { + dns_zt_apply(zt, false, NULL, setview, dns_zone_setviewcommit); } void dns_zt_setviewrevert(dns_zt_t *zt) { - dns_rbtnode_t *node; - dns_rbtnodechain_t chain; - isc_result_t result; - - REQUIRE(VALID_ZT(zt)); - - dns_rbtnodechain_init(&chain); - - result = dns_rbtnodechain_first(&chain, zt->table, NULL, NULL); - while (result == DNS_R_NEWORIGIN || result == ISC_R_SUCCESS) { - result = dns_rbtnodechain_current(&chain, NULL, NULL, &node); - if (result == ISC_R_SUCCESS && node->data != NULL) { - dns_zone_setviewrevert(node->data); - } - - result = dns_rbtnodechain_next(&chain, NULL, NULL); - } - - dns_rbtnodechain_invalidate(&chain); + dns_zt_apply(zt, false, NULL, setview, dns_zone_setviewrevert); } isc_result_t -dns_zt_apply(dns_zt_t *zt, isc_rwlocktype_t lock, bool stop, isc_result_t *sub, +dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, isc_result_t (*action)(dns_zone_t *, void *), void *uap) { - dns_rbtnode_t *node; - dns_rbtnodechain_t chain; - isc_result_t result, tresult = ISC_R_SUCCESS; - dns_zone_t *zone; + isc_result_t result = ISC_R_SUCCESS; + isc_result_t tresult = ISC_R_SUCCESS; + dns_qpiter_t qpi; + dns_qpread_t qpr; + void *zone = NULL; + uint32_t ival; REQUIRE(VALID_ZT(zt)); REQUIRE(action != NULL); - if (lock != isc_rwlocktype_none) { - RWLOCK(&zt->rwlock, lock); - } + dns_qpmulti_query(zt->multi, &qpr); + dns_qpiter_init(&qpr, &qpi); - dns_rbtnodechain_init(&chain); - result = dns_rbtnodechain_first(&chain, zt->table, NULL, NULL); - if (result == ISC_R_NOTFOUND) { - /* - * The tree is empty. - */ - tresult = result; - result = ISC_R_NOMORE; - } - while (result == DNS_R_NEWORIGIN || result == ISC_R_SUCCESS) { - result = dns_rbtnodechain_current(&chain, NULL, NULL, &node); - if (result == ISC_R_SUCCESS) { - zone = node->data; - if (zone != NULL) { - result = (action)(zone, uap); - } - if (result != ISC_R_SUCCESS && stop) { - tresult = result; - goto cleanup; /* don't break */ - } else if (result != ISC_R_SUCCESS && - tresult == ISC_R_SUCCESS) - { - tresult = result; - } + while (dns_qpiter_next(&qpi, &zone, &ival) == ISC_R_SUCCESS) { + result = action(zone, uap); + if (tresult == ISC_R_SUCCESS) { + tresult = result; + } + if (result != ISC_R_SUCCESS && stop) { + break; } - result = dns_rbtnodechain_next(&chain, NULL, NULL); - } - if (result == ISC_R_NOMORE) { - result = ISC_R_SUCCESS; } + dns_qpread_destroy(zt->multi, &qpr); -cleanup: - dns_rbtnodechain_invalidate(&chain); if (sub != NULL) { *sub = tresult; } - if (lock != isc_rwlocktype_none) { - RWUNLOCK(&zt->rwlock, lock); - } - return (result); } - -/* - * Decrement the loads_pending counter; when counter reaches - * zero, call the loaddone callback that was initially set by - * dns_zt_asyncload(). - */ -static isc_result_t -doneloading(dns_zt_t *zt, dns_zone_t *zone) { - REQUIRE(VALID_ZT(zt)); - - UNUSED(zone); - - if (isc_refcount_decrement(&zt->loads_pending) == 1) { - call_loaddone(zt); - } - - if (isc_refcount_decrement(&zt->references) == 1) { - zt_destroy(zt); - } - - return (ISC_R_SUCCESS); -} - -/*** - *** Private - ***/ - -static void -auto_detach(void *data, void *arg) { - dns_zone_t *zone = data; - - UNUSED(arg); - dns_zone_detach(&zone); -} diff --git a/lib/ns/notify.c b/lib/ns/notify.c index fd5b474c0c..dd3a2882f5 100644 --- a/lib/ns/notify.c +++ b/lib/ns/notify.c @@ -146,7 +146,7 @@ ns_notify_start(ns_client_t *client, isc_nmhandle_t *handle) { } dns_name_format(zonename, namebuf, sizeof(namebuf)); - result = dns_zt_find(client->view->zonetable, zonename, 0, NULL, &zone); + result = dns_view_findzone(client->view, zonename, &zone); if (result == ISC_R_SUCCESS) { dns_zonetype_t zonetype = dns_zone_gettype(zone); @@ -166,10 +166,10 @@ ns_notify_start(ns_client_t *client, isc_nmhandle_t *handle) { } } - notify_log(client, ISC_LOG_NOTICE, - "received notify for zone '%s'%s: not authoritative", - namebuf, tsigbuf); result = DNS_R_NOTAUTH; + notify_log(client, ISC_LOG_NOTICE, + "received notify for zone '%s'%s: %s", namebuf, tsigbuf, + isc_result_totext(result)); done: if (zone != NULL) { diff --git a/lib/ns/query.c b/lib/ns/query.c index a5fb82169a..83413ec0ec 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -1116,8 +1116,7 @@ query_getzonedb(ns_client_t *client, const dns_name_t *name, ztoptions |= DNS_ZTFIND_NOEXACT; } - result = dns_zt_find(client->view->zonetable, name, ztoptions, NULL, - &zone); + result = dns_zt_find(client->view->zonetable, name, ztoptions, &zone); if (result == DNS_R_PARTIALMATCH) { partial = true; diff --git a/lib/ns/update.c b/lib/ns/update.c index 9b0e325a98..e99afc1b01 100644 --- a/lib/ns/update.c +++ b/lib/ns/update.c @@ -1988,15 +1988,8 @@ ns_update_start(ns_client_t *client, isc_nmhandle_t *handle, "RRs"); } - result = dns_zt_find(client->view->zonetable, zonename, 0, NULL, &zone); + result = dns_view_findzone(client->view, zonename, &zone); if (result != ISC_R_SUCCESS) { - /* - * If we found a zone that is a parent of the update zonename, - * detach it so it isn't mentioned in log - it is irrelevant. - */ - if (zone != NULL) { - dns_zone_detach(&zone); - } FAILN(DNS_R_NOTAUTH, zonename, "not authoritative for update zone"); } diff --git a/lib/ns/xfrout.c b/lib/ns/xfrout.c index 23484caa5c..b42c08cc6a 100644 --- a/lib/ns/xfrout.c +++ b/lib/ns/xfrout.c @@ -797,9 +797,7 @@ ns_xfr_start(ns_client_t *client, dns_rdatatype_t reqtype) { FAILC(DNS_R_FORMERR, "multiple questions"); } - result = dns_zt_find(client->view->zonetable, question_name, 0, NULL, - &zone); - + result = dns_view_findzone(client->view, question_name, &zone); if (result != ISC_R_SUCCESS || dns_zone_gettype(zone) == dns_zone_dlz) { /* * The normal zone table does not have a match, or this is diff --git a/tests/dns/zt_test.c b/tests/dns/zt_test.c index 18a2848853..f3821abc2e 100644 --- a/tests/dns/zt_test.c +++ b/tests/dns/zt_test.c @@ -64,8 +64,8 @@ ISC_LOOP_TEST_IMPL(apply) { assert_non_null(view->zonetable); assert_int_equal(nzones, 0); - result = dns_zt_apply(view->zonetable, isc_rwlocktype_read, false, NULL, - count_zone, &nzones); + result = dns_zt_apply(view->zonetable, false, NULL, count_zone, + &nzones); assert_int_equal(result, ISC_R_SUCCESS); assert_int_equal(nzones, 1); @@ -83,12 +83,10 @@ ISC_LOOP_TEST_IMPL(apply) { } static isc_result_t -load_done_last(dns_zt_t *zt, dns_zone_t *zone) { +load_done_last(void *uap) { + dns_zone_t *zone = uap; isc_result_t result; - UNUSED(zt); - UNUSED(zone); - /* The zone should now be loaded; test it */ result = dns_zone_getdb(zone, &db); assert_int_equal(result, ISC_R_SUCCESS); @@ -110,29 +108,25 @@ load_done_last(dns_zt_t *zt, dns_zone_t *zone) { } static isc_result_t -load_done_new_only(dns_zt_t *zt, dns_zone_t *zone) { +load_done_new_only(void *uap) { + dns_zone_t *zone = uap; isc_result_t result; - UNUSED(zt); - UNUSED(zone); - /* The zone should now be loaded; test it */ result = dns_zone_getdb(zone, &db); assert_int_equal(result, ISC_R_SUCCESS); dns_db_detach(&db); - dns_zone_asyncload(zone, true, load_done_last, NULL); + dns_zone_asyncload(zone, true, load_done_last, zone); return (ISC_R_SUCCESS); } static isc_result_t -load_done_first(dns_zt_t *zt, dns_zone_t *zone) { - atomic_bool *done = (atomic_bool *)zt; +load_done_first(void *uap) { + dns_zone_t *zone = uap; isc_result_t result; - UNUSED(zone); - /* The zone should now be loaded; test it */ result = dns_zone_getdb(zone, &db); assert_int_equal(result, ISC_R_SUCCESS); @@ -146,7 +140,7 @@ load_done_first(dns_zt_t *zt, dns_zone_t *zone) { fflush(zonefile); fclose(zonefile); - dns_zone_asyncload(zone, true, load_done_new_only, &done); + dns_zone_asyncload(zone, true, load_done_new_only, zone); return (ISC_R_SUCCESS); } @@ -157,9 +151,6 @@ ISC_LOOP_TEST_IMPL(asyncload_zone) { int n; dns_zone_t *zone = NULL; char buf[4096]; - atomic_bool done; - - atomic_init(&done, false); result = dns_test_makezone("foo", &zone, NULL, true); assert_int_equal(result, ISC_R_SUCCESS); @@ -172,7 +163,6 @@ ISC_LOOP_TEST_IMPL(asyncload_zone) { assert_non_null(view->zonetable); assert_false(dns__zone_loadpending(zone)); - assert_false(atomic_load(&done)); zonefile = fopen("./zone.data", "wb"); assert_non_null(zonefile); origfile = fopen(TESTS_DIR "/testdata/zt/zone1.db", "r+b"); @@ -185,7 +175,7 @@ ISC_LOOP_TEST_IMPL(asyncload_zone) { dns_zone_setfile(zone, "./zone.data", dns_masterformat_text, &dns_master_style_default); - dns_zone_asyncload(zone, false, load_done_first, &done); + dns_zone_asyncload(zone, false, load_done_first, zone); } dns_zone_t *zone1 = NULL, *zone2 = NULL, *zone3 = NULL; diff --git a/tests/libtest/dns.c b/tests/libtest/dns.c index d398c5c954..b093b88c43 100644 --- a/tests/libtest/dns.c +++ b/tests/libtest/dns.c @@ -64,7 +64,7 @@ dns_test_makeview(const char *name, bool with_cache, dns_view_t **viewp) { dns_view_t *view = NULL; dns_cache_t *cache = NULL; - result = dns_view_create(mctx, dns_rdataclass_in, name, &view); + result = dns_view_create(mctx, loopmgr, dns_rdataclass_in, name, &view); if (result != ISC_R_SUCCESS) { return (result); }