From 29cf7dceb7f01db6da99d2155a0a09fbd8d57e04 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 6 Sep 2023 23:57:42 -0700 Subject: [PATCH 1/9] modify dns_qp_findname_ancestor() to return found name add a 'foundname' parameter to dns_qp_findname_ancestor(), and use it to set the found name in dns_nametree. this required adding a dns_qpkey_toname() function; that was done by moving qp_test_keytoname() from the test library to qp.c. added some more test cases and fixed bugs with the handling of relative and empty names. --- fuzz/dns_qpkey_name.c | 2 +- lib/dns/forward.c | 2 +- lib/dns/include/dns/qp.h | 18 ++++++- lib/dns/keytable.c | 4 +- lib/dns/nametree.c | 7 +-- lib/dns/nta.c | 2 +- lib/dns/qp.c | 99 ++++++++++++++++++++++++++++++++++-- lib/dns/zt.c | 5 +- tests/bench/qplookups.c | 4 +- tests/dns/qp_test.c | 107 ++++++++++++++++++++++++++++++++------- tests/include/tests/qp.h | 7 --- tests/libtest/qp.c | 74 --------------------------- 12 files changed, 214 insertions(+), 117 deletions(-) diff --git a/fuzz/dns_qpkey_name.c b/fuzz/dns_qpkey_name.c index ed9dd16b3c..cf818d5bc7 100644 --- a/fuzz/dns_qpkey_name.c +++ b/fuzz/dns_qpkey_name.c @@ -56,7 +56,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { /* verify round-trip conversion of first name */ size_t keylen = dns_qpkey_fromname(key, namein); - qp_test_keytoname(key, keylen, nameout); + dns_qpkey_toname(key, keylen, nameout); assert(dns_name_equal(namein, nameout)); diff --git a/lib/dns/forward.c b/lib/dns/forward.c index 20fdc5d4fb..1cc61115a8 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,7 +168,7 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 2801c49040..f855f12908 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -431,6 +431,17 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name); * \li the length of the key */ +void +dns_qpkey_toname(const dns_qpkey_t key, size_t keylen, dns_name_t *name); +/*%< + * Convert a trie lookup key back into a DNS name. + * + * Requires: + * \li `name` is a pointer to a valid `dns_name_t` + * \li `name->buffer` is not NULL + * \li `name->offsets` is not NULL + */ + isc_result_t dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, size_t search_keylen, void **pval_r, uint32_t *ival_r); @@ -469,7 +480,8 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, void **pval_r, uint32_t *ival_r); + dns_qpfind_t options, dns_name_t *foundname, + void **pval_r, uint32_t *ival_r); /*%< * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the * given DNS name. @@ -477,12 +489,16 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, * If the DNS_QPFIND_NOEXACT option is set, find the closest ancestor * domain that is not equal to the search name. * + * If 'foundname' is not NULL, it is updated to contain the name found. + * * The leaf values are assigned to whichever of `*pval_r` and `*ival_r` * are not null, unless the return value is ISC_R_NOTFOUND. * * Requires: * \li `qpr` is a pointer to a readable qp-trie * \li `name` is a pointer to a valid `dns_name_t` + * \li `foundname` is a pointer to a valid `dns_name_t` with + * buffer and offset space available, or is NULL. * * Returns: * \li ISC_R_SUCCESS if an exact match was found diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 10614431b3..0b243b3c55 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,7 +518,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -547,7 +547,7 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 08140371c0..0f906c58de 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,12 +289,9 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, (void **)&node, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, found, (void **)&node, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { - if (found != NULL) { - dns_name_copy(node->name, found); - } - switch (nametree->type) { case DNS_NAMETREE_BOOL: ret = node->set; diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 6df1a9f5ab..bcaf8eee57 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,7 +413,7 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index e416bf8085..b3fe7926bb 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -176,7 +176,7 @@ initialize_bits_for_byte(void) { qp_shift_t letter = byte - 'A'; qp_shift_t bit = after_esc + skip_punct + letter; dns_qp_bits_for_byte[byte] = bit; - /* to simplify reverse conversion in the tests */ + /* to simplify reverse conversion */ bit_two++; } else { /* non-hostname characters need to be escaped */ @@ -216,7 +216,11 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name) { dns_fixedname_t fixed; REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); - REQUIRE(name->labels > 0); + + if (name->labels == 0) { + key[0] = SHIFT_NOBYTE; + return (0); + } if (name->offsets == NULL) { dns_name_t *clone = dns_fixedname_initname(&fixed); @@ -245,6 +249,85 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name) { return (len); } +void +dns_qpkey_toname(const dns_qpkey_t key, size_t keylen, dns_name_t *name) { + size_t locs[DNS_NAME_MAXLABELS]; + size_t loc = 0, opos = 0; + size_t offset; + + REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); + REQUIRE(name->buffer != NULL); + REQUIRE(name->offsets != NULL); + + if (keylen == 0) { + dns_name_reset(name); + return; + } + + isc_buffer_clear(name->buffer); + + /* Scan the key looking for label boundaries */ + for (offset = 0; offset <= keylen; offset++) { + INSIST(key[offset] >= SHIFT_NOBYTE && + key[offset] < SHIFT_OFFSET); + INSIST(loc < DNS_NAME_MAXLABELS); + if (qpkey_bit(key, keylen, offset) == SHIFT_NOBYTE) { + if (qpkey_bit(key, keylen, offset + 1) == SHIFT_NOBYTE) + { + locs[loc] = offset + 1; + goto scanned; + } + locs[loc++] = offset + 1; + } else if (offset == 0) { + /* This happens for a relative name */ + locs[loc++] = offset; + } + } + UNREACHABLE(); +scanned: + + /* + * In the key the labels are encoded in reverse order, so + * we step backward through the label boundaries, then forward + * through the labels, to create the DNS wire format data. + */ + name->labels = loc; + while (loc-- > 0) { + uint8_t len = 0, *lenp = NULL; + + /* Add a length byte to the name data and set an offset */ + lenp = isc_buffer_used(name->buffer); + isc_buffer_putuint8(name->buffer, 0); + name->offsets[opos++] = name->length++; + + /* Convert from escaped byte ranges to ASCII */ + for (offset = locs[loc]; offset < locs[loc + 1] - 1; offset++) { + uint8_t bit = qpkey_bit(key, keylen, offset); + uint8_t byte = dns_qp_byte_for_bit[bit]; + if (qp_common_character(byte)) { + isc_buffer_putuint8(name->buffer, byte); + } else { + byte += key[++offset] - SHIFT_BITMAP; + isc_buffer_putuint8(name->buffer, byte); + } + len++; + } + + name->length += len; + *lenp = len; + } + + /* Add a root label for absolute names */ + if (key[0] == SHIFT_NOBYTE) { + name->attributes.absolute = true; + isc_buffer_putuint8(name->buffer, 0); + name->offsets[opos++] = name->length++; + name->labels++; + } + + name->ndata = isc_buffer_base(name->buffer); +} + /* * Sentinel value for equal keys */ @@ -1811,8 +1894,8 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, void **pval_r, - uint32_t *ival_r) { + dns_qpfind_t options, dns_name_t *foundname, + void **pval_r, uint32_t *ival_r) { dns_qpreader_t *qp = dns_qpreader(qpr); dns_qpkey_t search, found; size_t searchlen, foundlen; @@ -1827,6 +1910,7 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, } label[DNS_NAME_MAXLABELS]; REQUIRE(QP_VALID(qp)); + REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); searchlen = dns_qpkey_fromname(search, name); if ((options & DNS_QPFIND_NOEXACT) != 0) { @@ -1888,6 +1972,9 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, if (offset == QPKEY_EQUAL || offset == foundlen) { SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); + if (foundname != NULL) { + dns_qpkey_toname(found, foundlen, foundname); + } if (offset == QPKEY_EQUAL) { return (result); } else { @@ -1899,6 +1986,10 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, n = ref_ptr(qp, label[labels].ref); SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); + if (foundname != NULL) { + foundlen = leaf_qpkey(qp, n, found); + dns_qpkey_toname(found, foundlen, foundname); + } return (DNS_R_PARTIALMATCH); } } diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 7f2cedadd3..a65d527b42 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -181,9 +181,10 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, result = dns_qp_getname(&qpr, name, &pval, NULL); } else if (exactopts == DNS_ZTFIND_NOEXACT) { result = dns_qp_findname_ancestor( - &qpr, name, DNS_QPFIND_NOEXACT, &pval, NULL); + &qpr, name, DNS_QPFIND_NOEXACT, NULL, &pval, NULL); } else { - result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, + NULL); } dns_qpread_destroy(zt->multi, &qpr); diff --git a/tests/bench/qplookups.c b/tests/bench/qplookups.c index f7473ba07f..71564d6cab 100644 --- a/tests/bench/qplookups.c +++ b/tests/bench/qplookups.c @@ -254,7 +254,7 @@ main(int argc, char **argv) { start = isc_time_monotonic(); for (i = 0; i < n; i++) { name = dns_fixedname_name(&items[i]); - dns_qp_findname_ancestor(qp, name, 0, NULL, NULL); + dns_qp_findname_ancestor(qp, name, 0, NULL, NULL, NULL); } stop = isc_time_monotonic(); @@ -280,7 +280,7 @@ main(int argc, char **argv) { ++search->ndata[1]; } - dns_qp_findname_ancestor(qp, search, 0, NULL, NULL); + dns_qp_findname_ancestor(qp, search, 0, NULL, NULL, NULL); } stop = isc_time_monotonic(); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index fb95350f1f..80d076a4a3 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -44,6 +44,11 @@ ISC_RUN_TEST_IMPL(qpkey_name) { uint8_t key[512]; size_t len; } testcases[] = { + { + .namestr = "", + .key = { 0x02 }, + .len = 0, + }, { .namestr = ".", .key = { 0x02, 0x02 }, @@ -54,6 +59,16 @@ ISC_RUN_TEST_IMPL(qpkey_name) { .key = { 0x03, 0x03, 0x02, 0x02 }, .len = 3, }, + { + .namestr = "com", + .key = { 0x16, 0x22, 0x20, 0x02 }, + .len = 4, + }, + { + .namestr = "com.", + .key = { 0x02, 0x16, 0x22, 0x20, 0x02 }, + .len = 5, + }, { .namestr = "example.com.", .key = { 0x02, 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, @@ -80,15 +95,21 @@ ISC_RUN_TEST_IMPL(qpkey_name) { dns_fixedname_t fn1, fn2; dns_name_t *in = NULL, *out = NULL; - dns_test_namefromstring(testcases[i].namestr, &fn1); - in = dns_fixedname_name(&fn1); + in = dns_fixedname_initname(&fn1); + if (testcases[i].len != 0) { + dns_test_namefromstring(testcases[i].namestr, &fn1); + } len = dns_qpkey_fromname(key, in); assert_int_equal(testcases[i].len, len); assert_memory_equal(testcases[i].key, key, len); + /* also check key correctness for empty name */ + if (len == 0) { + assert_int_equal(testcases[i].key[0], ((char *)key)[0]); + } out = dns_fixedname_initname(&fn2); - qp_test_keytoname(key, len, out); + dns_qpkey_toname(key, len, out); assert_true(dns_name_equal(in, out)); } } @@ -258,19 +279,47 @@ 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); + dns_fixedname_t fn1, fn2; + dns_name_t *name = dns_fixedname_initname(&fn1); + dns_name_t *foundname = dns_fixedname_initname(&fn2); void *pval = NULL; + dns_test_namefromstring(check[i].query, &fn1); + result = dns_qp_findname_ancestor(qp, name, check[i].options, + foundname, &pval, NULL); + #if 0 - fprintf(stderr, "%s %u %s %s\n", check[i].query, - check[i].options, isc_result_totext(check[i].result), + fprintf(stderr, "%s (flags %u) %s (expected %s) " + "value \"%s\" (expected \"%s\")\n", + check[i].query, check[i].options, + isc_result_totext(result), + isc_result_totext(check[i].result), (char *)pval, check[i].found); #endif - dns_test_namefromstring(check[i].query, &fixed); - result = dns_qp_findname_ancestor(qp, name, check[i].options, - &pval, NULL); + assert_int_equal(result, check[i].result); + if (result == ISC_R_SUCCESS) { + assert_true(dns_name_equal(name, foundname)); + } else if (result == DNS_R_PARTIALMATCH) { + /* + * there are cases where we may have passed a + * query name that was relative to the zone apex, + * and gotten back an absolute name from the + * partial match. it's also possible for an + * absolute query to get a partial match on a + * node that had an empty name. in these cases, + * sanity checking the relations between name + * and foundname can trigger an assertion, so + * let's just skip them. + */ + if (dns_name_isabsolute(name) == + dns_name_isabsolute(foundname)) + { + assert_false(dns_name_equal(name, foundname)); + assert_true( + dns_name_issubdomain(name, foundname)); + } + } if (check[i].found == NULL) { assert_null(pval); } else { @@ -291,6 +340,7 @@ insert_str(dns_qp_t *qp, const char *str) { ISC_RUN_TEST_IMPL(partialmatch) { isc_result_t result; dns_qp_t *qp = NULL; + int i = 0; dns_qp_create(mctx, &string_methods, NULL, &qp); @@ -302,7 +352,10 @@ ISC_RUN_TEST_IMPL(partialmatch) { "fooo.bar.", "web.foo.bar.", ".", "", }; - int i = 0; + /* + * omit the root node for now, otherwise we'll get "partial match" + * results when we want "not found". + */ while (insert[i][0] != '.') { insert_str(qp, insert[i++]); } @@ -340,21 +393,41 @@ ISC_RUN_TEST_IMPL(partialmatch) { { "bar.", 0, DNS_R_PARTIALMATCH, "." }, { "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." }, { "foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, + { "bar", 0, ISC_R_NOTFOUND, NULL }, + { "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? */ + /* + * what if entries in the trie are relative to the zone apex + * and there's no root node? + */ dns_qpkey_t rootkey = { SHIFT_NOBYTE }; result = dns_qp_deletekey(qp, rootkey, 1, NULL, NULL); assert_int_equal(result, ISC_R_SUCCESS); + check_partialmatch( + qp, + (struct check_partialmatch[]){ + { "bar", 0, ISC_R_NOTFOUND, NULL }, + { "bar", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, + { "bar.", 0, ISC_R_NOTFOUND, NULL }, + { "bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, + { NULL, 0, 0, NULL }, + }); + + /* what if there's a root node with an empty key? */ 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 }, - }); + check_partialmatch( + qp, + (struct check_partialmatch[]){ + { "bar", 0, DNS_R_PARTIALMATCH, "" }, + { "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, + { "bar.", 0, DNS_R_PARTIALMATCH, "" }, + { "bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, + { NULL, 0, 0, NULL }, + }); dns_qp_destroy(&qp); } diff --git a/tests/include/tests/qp.h b/tests/include/tests/qp.h index 8e165001aa..d0d7357315 100644 --- a/tests/include/tests/qp.h +++ b/tests/include/tests/qp.h @@ -29,13 +29,6 @@ qp_test_bittoascii(uint8_t bit); const char * qp_test_keytoascii(dns_qpkey_t key, size_t len); -/* - * Convert a trie lookup key back into a DNS name. Unlike the previous - * functions, this is a complete inverse of dns_qpkey_fromname(). - */ -void -qp_test_keytoname(const dns_qpkey_t key, size_t len, dns_name_t *name); - /* * The maximum height of the trie */ diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index 392297746a..f2b914472f 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -61,80 +61,6 @@ qp_test_keytoascii(dns_qpkey_t key, size_t len) { return ((const char *)key); } -void -qp_test_keytoname(const dns_qpkey_t key, size_t keylen, dns_name_t *name) { - size_t locs[DNS_NAME_MAXLABELS]; - size_t loc = 0, opos = 0; - size_t offset; - - REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); - REQUIRE(name->buffer != NULL); - REQUIRE(name->offsets != NULL); - - isc_buffer_clear(name->buffer); - - /* Scan the key looking for label boundaries */ - for (offset = 0; offset <= keylen; offset++) { - INSIST(key[offset] >= SHIFT_NOBYTE && - key[offset] < SHIFT_OFFSET); - INSIST(loc < DNS_NAME_MAXLABELS); - if (qpkey_bit(key, keylen, offset) == SHIFT_NOBYTE) { - if (qpkey_bit(key, keylen, offset + 1) == SHIFT_NOBYTE) - { - locs[loc] = offset + 1; - goto scanned; - } - locs[loc++] = offset + 1; - } else if (offset == 0) { - /* This happens for a relative name */ - locs[loc++] = offset; - } - } - UNREACHABLE(); -scanned: - - /* - * In the key the labels are encoded in reverse order, so - * we step backward through the label boundaries, then forward - * through the labels, to create the DNS wire format data. - */ - name->labels = loc; - while (loc-- > 0) { - uint8_t len = 0, *lenp = NULL; - - /* Add a length byte to the name data and set an offset */ - lenp = isc_buffer_used(name->buffer); - isc_buffer_putuint8(name->buffer, 0); - name->offsets[opos++] = name->length++; - - /* Convert from escaped byte ranges to ASCII */ - for (offset = locs[loc]; offset < locs[loc + 1] - 1; offset++) { - uint8_t bit = qpkey_bit(key, keylen, offset); - uint8_t byte = dns_qp_byte_for_bit[bit]; - if (qp_common_character(byte)) { - isc_buffer_putuint8(name->buffer, byte); - } else { - byte += key[++offset] - SHIFT_BITMAP; - isc_buffer_putuint8(name->buffer, byte); - } - len++; - } - - name->length += len; - *lenp = len; - } - - /* Add a root label for absolute names */ - if (key[0] == SHIFT_NOBYTE) { - name->attributes.absolute = true; - isc_buffer_putuint8(name->buffer, 0); - name->length++; - name->labels++; - } - - name->ndata = isc_buffer_base(name->buffer); -} - /*********************************************************************** * * trie properties From 7f766ba7c45729cb3a066e7158731ab29a3ef16e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 8 Sep 2023 17:44:53 -0700 Subject: [PATCH 2/9] add a node chain traversal mechanism dns_qp_findname_ancestor() now takes an optional 'chain' parameter; if set, the dns_qpchain object it points to will be updated with an array of pointers to the populated nodes between the tree root and the requested name. the number of nodes in the chain can then be accessed using dns_qpchain_length() and the individual nodes using dns_qpchain_node(). --- lib/dns/forward.c | 3 +- lib/dns/include/dns/qp.h | 61 ++++++++++++++- lib/dns/keytable.c | 6 +- lib/dns/nametree.c | 4 +- lib/dns/nta.c | 3 +- lib/dns/qp.c | 162 ++++++++++++++++++++++++++++++--------- lib/dns/qp_p.h | 2 + lib/dns/zt.c | 9 ++- tests/bench/qplookups.c | 4 +- tests/dns/qp_test.c | 99 +++++++++++++++++++++++- 10 files changed, 299 insertions(+), 54 deletions(-) diff --git a/lib/dns/forward.c b/lib/dns/forward.c index 1cc61115a8..d885df68b4 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,7 +168,8 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index f855f12908..07850d176c 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -86,6 +86,7 @@ #include +#include #include /*% @@ -182,8 +183,9 @@ typedef union dns_qpreadable { 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()`. + * A QP iterator traverses a trie starting with the root and passing + * though each leaf node in lexicographic order; it is used with + * `dns_qpiter_init()` and `dns_qpiter_next()`. */ typedef struct dns_qpiter { unsigned int magic; @@ -195,6 +197,22 @@ typedef struct dns_qpiter { } stack[DNS_QP_MAXKEY]; } dns_qpiter_t; +/*% + * A QP chain holds references to each populated node between + * the root and a given leaf. It is set while running + * `dns_qp_findname_ancestor()`, and can optionally be passed + * back to the caller so that individual nodes can be accessed. + */ +typedef struct dns_qpchain { + unsigned int magic; + dns_qpreader_t *qp; + uint8_t len; + struct { + void *node; + size_t offset; + } chain[DNS_NAME_MAXLABELS]; +} dns_qpchain_t; + /*% * These leaf methods allow the qp-trie code to call back to the code * responsible for the leaf values that are stored in the trie. The @@ -481,7 +499,7 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, dns_qpfind_t options, dns_name_t *foundname, - void **pval_r, uint32_t *ival_r); + dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); /*%< * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the * given DNS name. @@ -491,6 +509,10 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, * * If 'foundname' is not NULL, it is updated to contain the name found. * + * If 'chain' is not NULL, it is updated to contain a QP chain with + * references to the populated nodes in the tree between the root and + * the name found. + * * The leaf values are assigned to whichever of `*pval_r` and `*ival_r` * are not null, unless the return value is ISC_R_NOTFOUND. * @@ -600,6 +622,39 @@ dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r); * \li ISC_R_NOMORE otherwise */ +void +dns_qpchain_init(dns_qpreadable_t qpr, dns_qpchain_t *chain); +/*%< + * Initialize a QP chain. + * + * Requires: + * \li `qpr` is a pointer to a valid qp-trie + * \li `chain` is not NULL. + */ + +unsigned int +dns_qpchain_length(dns_qpchain_t *chain); +/*%< + * Returns the length of a QP chain. + * + * Requires: + * \li `chain` is a pointer to an initialized QP chain object. + */ + +void +dns_qpchain_node(dns_qpchain_t *chain, unsigned int level, dns_name_t *name, + void **pval_r, uint32_t *ival_r); +/*%< + * Sets 'name' to the name of the leaf referenced at `chain->stack[level]`. + * + * The leaf values are assigned to whichever of `*pval_r` and `*ival_r` + * are not null. + * + * Requires: + * \li `chain` is a pointer to an initialized QP chain object. + * \li `level` is less than `chain->len`. + */ + /*********************************************************************** * * functions - transactions diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 0b243b3c55..8d5b51e9b0 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,7 +518,8 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, + NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -547,7 +548,8 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 0f906c58de..0fd1110a16 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,8 +289,8 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, found, (void **)&node, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, found, NULL, + (void **)&node, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { switch (nametree->type) { case DNS_NAMETREE_BOOL: diff --git a/lib/dns/nta.c b/lib/dns/nta.c index bcaf8eee57..0d5a90ac34 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,7 +413,8 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, + NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index b3fe7926bb..82bc399dac 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1778,8 +1778,56 @@ dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name, void **pval_r, } /*********************************************************************** - * - * iterate + * chains + */ +static void +maybe_set_name(dns_qpreader_t *qp, qp_node_t *node, dns_name_t *name) { + dns_qpkey_t key; + size_t len; + + if (name == NULL) { + return; + } + + len = leaf_qpkey(qp, node, key); + dns_qpkey_toname(key, len, name); +} + +void +dns_qpchain_init(dns_qpreadable_t qpr, dns_qpchain_t *chain) { + dns_qpreader_t *qp = dns_qpreader(qpr); + REQUIRE(QP_VALID(qp)); + REQUIRE(chain != NULL); + + *chain = (dns_qpchain_t){ + .magic = QPCHAIN_MAGIC, + .qp = qp, + }; +} + +unsigned int +dns_qpchain_length(dns_qpchain_t *chain) { + REQUIRE(QPCHAIN_VALID(chain)); + + return (chain->len); +} + +void +dns_qpchain_node(dns_qpchain_t *chain, unsigned int level, dns_name_t *name, + void **pval_r, uint32_t *ival_r) { + qp_node_t *node = NULL; + + REQUIRE(QPCHAIN_VALID(chain)); + REQUIRE(level < chain->len); + + node = chain->chain[level].node; + maybe_set_name(chain->qp, node, name); + SET_IF_NOT_NULL(pval_r, leaf_pval(node)); + SET_IF_NOT_NULL(ival_r, leaf_ival(node)); +} + +/*********************************************************************** + * iterators */ void @@ -1892,22 +1940,26 @@ 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)); } +static inline void +add_link(dns_qpchain_t *chain, qp_node_t *node, size_t offset) { + chain->chain[chain->len].node = node; + chain->chain[chain->len].offset = offset; + chain->len++; + INSIST(chain->len <= DNS_NAME_MAXLABELS); +} + isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, dns_qpfind_t options, dns_name_t *foundname, - void **pval_r, uint32_t *ival_r) { + dns_qpchain_t *chain, 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 = NULL, *twigs = NULL; + qp_node_t *n = NULL; isc_result_t result; - unsigned int labels = 0; - struct offref { - uint32_t off; - qp_ref_t ref; - } label[DNS_NAME_MAXLABELS]; + dns_qpchain_t oc; REQUIRE(QP_VALID(qp)); REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); @@ -1920,6 +1972,11 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, result = ISC_R_SUCCESS; } + if (chain == NULL) { + chain = &oc; + } + dns_qpchain_init(qp, chain); + n = get_root(qp); if (n == NULL) { return (ISC_R_NOTFOUND); @@ -1931,37 +1988,52 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, * 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); + + qp_node_t *twigs = branch_twigs_vector(qp, n); offset = branch_key_offset(n); - bit = qpkey_bit(search, searchlen, offset); + qp_shift_t bit = qpkey_bit(search, searchlen, offset); + + /* + * 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 `off - 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 `off == 0`. + * + * Note 2: Any SHIFT_NOBYTE twig is always `twigs[0]`. + */ 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); + add_link(chain, twigs, offset); } if (branch_has_twig(n, bit)) { + /* we have the twig we wanted */ n = branch_twig_ptr(qp, n, bit); - } else if (labels == 0) { - /* any twig will do */ + } else if (chain->len == 0) { + /* + * the twig we're looking for isn't here, + * and we haven't found an ancestor yet either. + * continue the search with whatever this branch's + * first twig is. + */ n = &twigs[0]; } else { - n = ref_ptr(qp, label[labels - 1].ref); - break; + /* + * this branch is a dead end, but we do have + * a previous leaf node saved in the chain. + * we go back to that. it's a leaf, so we'll + * fall out of the loop here. + */ + n = chain->chain[chain->len - 1].node; } } @@ -1972,27 +2044,41 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, if (offset == QPKEY_EQUAL || offset == foundlen) { SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); - if (foundname != NULL) { - dns_qpkey_toname(found, foundlen, foundname); - } + maybe_set_name(qp, n, foundname); if (offset == QPKEY_EQUAL) { + if ((options & DNS_QPFIND_NOEXACT) == 0) { + /* add the exact match to the chain */ + add_link(chain, n, offset); + } return (result); } else { return (DNS_R_PARTIALMATCH); } } - while (labels-- > 0) { - if (offset > label[labels].off) { - n = ref_ptr(qp, label[labels].ref); + + /* + * the requested name was not found, but if an ancestor + * was, we can retrieve that from the chain. + */ + int len = chain->len; + while (len-- > 0) { + if (offset >= chain->chain[len].offset) { + n = chain->chain[len].node; SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); - if (foundname != NULL) { - foundlen = leaf_qpkey(qp, n, found); - dns_qpkey_toname(found, foundlen, foundname); - } + maybe_set_name(qp, n, foundname); return (DNS_R_PARTIALMATCH); + } else { + /* + * oops, during the search we found and added + * a leaf that's longer than the requested + * name; remove it from the chain. + */ + chain->len--; } } + + /* nothing was found at all */ return (ISC_R_NOTFOUND); } diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index c0f69c3568..2a92227d9b 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -393,6 +393,7 @@ 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 QPCHAIN_MAGIC ISC_MAGIC('q', 'p', 'c', 'h') #define QPMULTI_MAGIC ISC_MAGIC('q', 'p', 'm', 'v') #define QPREADER_MAGIC ISC_MAGIC('q', 'p', 'r', 'x') #define QPBASE_MAGIC ISC_MAGIC('q', 'p', 'b', 'p') @@ -400,6 +401,7 @@ ref_ptr(dns_qpreadable_t qpr, qp_ref_t ref) { #define QP_VALID(qp) ISC_MAGIC_VALID(qp, QP_MAGIC) #define QPITER_VALID(qp) ISC_MAGIC_VALID(qp, QPITER_MAGIC) +#define QPCHAIN_VALID(qp) ISC_MAGIC_VALID(qp, QPCHAIN_MAGIC) #define QPMULTI_VALID(qp) ISC_MAGIC_VALID(qp, QPMULTI_MAGIC) #define QPBASE_VALID(qp) ISC_MAGIC_VALID(qp, QPBASE_MAGIC) #define QPRCU_VALID(qp) ISC_MAGIC_VALID(qp, QPRCU_MAGIC) diff --git a/lib/dns/zt.c b/lib/dns/zt.c index a65d527b42..2f21896015 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -180,11 +180,12 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, if (exactopts == DNS_ZTFIND_EXACT) { result = dns_qp_getname(&qpr, name, &pval, NULL); } else if (exactopts == DNS_ZTFIND_NOEXACT) { - result = dns_qp_findname_ancestor( - &qpr, name, DNS_QPFIND_NOEXACT, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, + DNS_QPFIND_NOEXACT, NULL, + NULL, &pval, NULL); } else { - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, + &pval, NULL); } dns_qpread_destroy(zt->multi, &qpr); diff --git a/tests/bench/qplookups.c b/tests/bench/qplookups.c index 71564d6cab..c269edb750 100644 --- a/tests/bench/qplookups.c +++ b/tests/bench/qplookups.c @@ -254,7 +254,7 @@ main(int argc, char **argv) { start = isc_time_monotonic(); for (i = 0; i < n; i++) { name = dns_fixedname_name(&items[i]); - dns_qp_findname_ancestor(qp, name, 0, NULL, NULL, NULL); + dns_qp_findname_ancestor(qp, name, 0, NULL, NULL, NULL, NULL); } stop = isc_time_monotonic(); @@ -280,7 +280,7 @@ main(int argc, char **argv) { ++search->ndata[1]; } - dns_qp_findname_ancestor(qp, search, 0, NULL, NULL, NULL); + dns_qp_findname_ancestor(qp, search, 0, NULL, NULL, NULL, NULL); } stop = isc_time_monotonic(); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 80d076a4a3..a456e5e639 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -286,7 +286,7 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { dns_test_namefromstring(check[i].query, &fn1); result = dns_qp_findname_ancestor(qp, name, check[i].options, - foundname, &pval, NULL); + foundname, NULL, &pval, NULL); #if 0 fprintf(stderr, "%s (flags %u) %s (expected %s) " @@ -432,11 +432,108 @@ ISC_RUN_TEST_IMPL(partialmatch) { dns_qp_destroy(&qp); } +struct check_qpchain { + const char *query; + dns_qpfind_t options; + isc_result_t result; + unsigned int length; + const char *names[10]; +}; + +static void +check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { + for (int i = 0; check[i].query != NULL; i++) { + isc_result_t result; + dns_fixedname_t fn1; + dns_name_t *name = dns_fixedname_initname(&fn1); + dns_qpchain_t chain; + + dns_qpchain_init(qp, &chain); + dns_test_namefromstring(check[i].query, &fn1); + result = dns_qp_findname_ancestor(qp, name, check[i].options, + NULL, &chain, NULL, NULL); + +#if 0 + fprintf(stderr, "%s (%d) %s (expected %s), " + "len %d (expected %d)\n", + check[i].query, check[i].options, + isc_result_totext(result), + isc_result_totext(check[i].result), + dns_qpchain_length(&chain), check[i].length); +#endif + assert_int_equal(result, check[i].result); + assert_int_equal(dns_qpchain_length(&chain), check[i].length); + for (unsigned int j = 0; j < check[i].length; j++) { + dns_fixedname_t fn2, fn3; + dns_name_t *expected = dns_fixedname_initname(&fn2); + dns_name_t *found = dns_fixedname_initname(&fn3); + + dns_test_namefromstring(check[i].names[j], &fn2); + dns_qpchain_node(&chain, j, found, NULL, NULL); +#if 0 + char nb[DNS_NAME_FORMATSIZE]; + dns_name_format(found, nb, sizeof(nb)); + fprintf(stderr, "got %s, expected %s\n", nb, + check[i].names[j]); +#endif + assert_true(dns_name_equal(found, expected)); + } + } +} + +ISC_RUN_TEST_IMPL(qpchain) { + dns_qp_t *qp = NULL; + int i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + + /* + * Fixed size strings [16] should ensure leaf-compatible alignment. + */ + const char insert[][16] = { ".", "a.", "b.", "c.b.a.", + "e.d.c.b.a.", "c.b.b.", "c.d.", "a.b.c.d.", + "a.b.c.d.e.", "" }; + + while (insert[i][0] != '\0') { + insert_str(qp, insert[i++]); + } + + static struct check_qpchain check1[] = { + { "b.", 0, ISC_R_SUCCESS, 2 }, + { "c.", 0, DNS_R_PARTIALMATCH, 1 }, + { "e.d.c.b.a.", 0, ISC_R_SUCCESS, 4 }, + { "a.b.c.d.", 0, ISC_R_SUCCESS, 3 }, + { "a.b.c.d.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, 2 }, + { NULL, 0, 0, 0 }, + }; + + check1[0].names[0] = "."; + check1[0].names[1] = "b."; + + check1[1].names[0] = "."; + + check1[2].names[0] = "."; + check1[2].names[1] = "a."; + check1[2].names[2] = "c.b.a."; + check1[2].names[3] = "e.d.c.b.a."; + + check1[3].names[0] = "."; + check1[3].names[1] = "c.d."; + check1[3].names[2] = "a.b.c.d."; + + check1[4].names[0] = "."; + check1[4].names[1] = "c.d."; + + check_qpchain(qp, check1); + 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_ENTRY(qpchain) ISC_TEST_LIST_END ISC_TEST_MAIN From 7f0242b8c7edfda7d000fd2e343be73b0b4d9d13 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 27 Sep 2023 09:08:37 -0700 Subject: [PATCH 3/9] tidy the helper functions for retrieving twigs - the helper functions for accessing twigs beneath a branch (branch_twig_pos(), branch_twig_ptr(), etc) were somewhat confusing to read, since several of them were implemented by calling other helper functions. they now all show what they're really doing. - branch_twigs_vector() has been renamed to simply branch_twigs(). - revised some unrelated comments in qp_p.h for clarity. --- lib/dns/qp.c | 4 +-- lib/dns/qp_p.h | 69 +++++++++++++++++++++++++--------------------- tests/libtest/qp.c | 6 ++-- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 82bc399dac..663ffe1a53 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1611,7 +1611,7 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival) { prefetch_twigs(qp, n); bit = branch_keybit(n, new_key, new_keylen); pos = branch_has_twig(n, bit) ? branch_twig_pos(n, bit) : 0; - n = branch_twigs_vector(qp, n) + pos; + n = branch_twigs(qp, n) + pos; } /* do the keys differ, and if so, where? */ @@ -1992,7 +1992,7 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, while (is_branch(n)) { prefetch_twigs(qp, n); - qp_node_t *twigs = branch_twigs_vector(qp, n); + qp_node_t *twigs = branch_twigs(qp, n); offset = branch_key_offset(n); qp_shift_t bit = qpkey_bit(search, searchlen, offset); diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index 2a92227d9b..c012492bc6 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -25,29 +25,39 @@ */ /* - * A qp-trie node is normally either a branch or a leaf. It consists of - * three 32-bit words into which the components are packed. They are used - * as a 64-bit word and a 32-bit word, but they are not declared like that - * to avoid unwanted padding, keeping the size down to 12 bytes. They are - * in native endian order so getting the 64-bit part should compile down to - * an unaligned load. + * A qp-trie node is almost always one of two types: branch or leaf. + * (A third type is used only to anchor the root of a trie; see below.) * - * The type of node is identified by the tag in the least significant bits - * of the 64-bit word. + * A node contains a 64-bit word and a 32-bit word. In order to avoid + * unwanted padding, they are declared as three 32-bit words; this keeps + * the size down to 12 bytes. They are in native endian order, so getting + * the 64-bit part should compile down to an unaligned load. * - * In a branch the 64-bit word is described by the enum below. The 32-bit - * word is a reference to the packed sparse vector of "twigs", i.e. child - * nodes. A branch node has at least 2 and less than SHIFT_OFFSET twigs - * (see the enum below). The qp-trie update functions ensure that branches - * actually branch, i.e. branches cannot have only 1 child. + * The node type is identified by the least significant bits of the 64-bit + * word. * - * The contents of each leaf are set by the trie's user. The 64-bit word - * contains a pointer value (which must be word-aligned, so the tag bits - * are zero), and the 32-bit word is an arbitrary integer value. + * In a leaf node: + * - The 64-bit word is used to store a pointer value. (Pointers must be + * word-aligned so the least significant bits are zero; those bits can + * then act as a node tag to indicate that this is a leaf. This + * requirement is enforced by the make_leaf() constructor.) + * - The 32-bit word is used to store an integer value. Both the + * pointer and integer values can be retrieved when looking up a key. * - * There is a third kind of node, reader nodes, which anchor the root of a - * trie. A pair of reader nodes together contain a packed `dns_qpreader_t`. - * See the section on "packed reader nodes" below. + * In a branch node: + * - The 64-bit word is subdivided into three portions: the least + * significant bits are the node type (for a branch, 0x1); the + * most sigificant 15 bits are an offset value into the key, and + * the 47 bits in the middle are a bitmap; see the documentation + * for the SHIFT_* enum below. + * - The 32-bit word is a reference (qp_ref_t) to the packed sparse + * vector of "twigs", i.e. child nodes. A branch node has at least + * two and at most 47 twigs. (The qp-trie update functions ensure that + * branches actually branch, i.e. a branch cannot have only one child.) + * + * A third node type, reader nodes, anchors the root of a trie. + * A pair of reader nodes together contain a packed `dns_qpreader_t`. + * See the section on "packed reader nodes" for details. */ typedef struct qp_node { #if WORDS_BIGENDIAN @@ -78,17 +88,13 @@ STATIC_ASSERT(sizeof(void *) <= sizeof(uint64_t), "pointers must fit in 64 bits"); /* - * A branch node contains a 64-bit word comprising the type tag, the + * The 64-bit word in a branch node is comprised of a node type tag, a * bitmap, and an offset into the key. It is called an "index word" because * it describes how to access the twigs vector (think "database index"). * The following enum sets up the bit positions of these parts. * - * In a leaf, the same 64-bit word contains a pointer. The pointer - * must be word-aligned so that the branch/leaf tag bit is zero. - * This requirement is checked by the newleaf() constructor. - * - * The bitmap is just above the type tag. The `bits_for_byte[]` table is - * used to fill in a key so that bit tests can work directly against the + * The bitmap is just above the type tag. The `dns_qp_bits_for_byte[]` table + * is used to fill in a key so that bit tests can work directly against the * index word without superfluous masking or shifting; we don't need to * mask out the bitmap before testing a bit, but we do need to mask the * bitmap before calling popcount. @@ -716,10 +722,11 @@ branch_keybit(qp_node_t *n, const dns_qpkey_t key, size_t len) { } /* - * Get a pointer to a branch node's twigs vector. + * Get a pointer to a the first twig of a branch (this also functions + * as a pointer to the entire twig vector). */ static inline qp_node_t * -branch_twigs_vector(dns_qpreadable_t qpr, qp_node_t *n) { +branch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { return (ref_ptr(qpr, branch_twigs_ref(n))); } @@ -728,7 +735,7 @@ branch_twigs_vector(dns_qpreadable_t qpr, qp_node_t *n) { */ static inline void prefetch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { - __builtin_prefetch(branch_twigs_vector(qpr, n)); + __builtin_prefetch(ref_ptr(qpr, branch_twigs_ref(n))); } /* root node **********************************************************/ @@ -798,11 +805,11 @@ branch_twig_pos(qp_node_t *n, qp_shift_t bit) { } /* - * Get a pointer to a particular twig. + * Get a pointer to the twig for a given bit number. */ static inline qp_node_t * branch_twig_ptr(dns_qpreadable_t qpr, qp_node_t *n, qp_shift_t bit) { - return (branch_twigs_vector(qpr, n) + branch_twig_pos(n, bit)); + return (ref_ptr(qpr, branch_twigs_ref(n) + branch_twig_pos(n, bit))); } /* diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index f2b914472f..be39156be1 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -73,7 +73,7 @@ getheight(dns_qp_t *qp, qp_node_t *n) { } size_t max_height = 0; qp_weight_t size = branch_twigs_size(n); - qp_node_t *twigs = branch_twigs_vector(qp, n); + qp_node_t *twigs = branch_twigs(qp, n); for (qp_weight_t pos = 0; pos < size; pos++) { size_t height = getheight(qp, &twigs[pos]); max_height = ISC_MAX(max_height, height); @@ -95,7 +95,7 @@ maxkeylen(dns_qp_t *qp, qp_node_t *n) { } size_t max_len = 0; qp_weight_t size = branch_twigs_size(n); - qp_node_t *twigs = branch_twigs_vector(qp, n); + qp_node_t *twigs = branch_twigs(qp, n); for (qp_weight_t pos = 0; pos < size; pos++) { size_t len = maxkeylen(qp, &twigs[pos]); max_len = ISC_MAX(max_len, len); @@ -300,7 +300,7 @@ dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { printf("}}\"];\n"); qp_weight_t size = branch_twigs_size(n); - qp_node_t *twigs = branch_twigs_vector(qp, n); + qp_node_t *twigs = branch_twigs(qp, n); for (qp_weight_t pos = 0; pos < size; pos++) { dumpdot_name(n); From 3bf23fadb0cd9c39319d6db5d12b2a1ab1c27f44 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 19 Sep 2023 00:41:57 -0700 Subject: [PATCH 4/9] improvements to the QP iterator - make iterators reversible: refactor dns_qpiter_next() and add a new dns_qpiter_prev() function to support iterating both forwards and backwards through a QP trie. - added a 'name' parameter to dns_qpiter_next() (as well as _prev()) to make it easier to retrieve the nodename while iterating, without having to construct it from pointer value data. --- lib/dns/include/dns/qp.h | 18 +++-- lib/dns/keytable.c | 6 +- lib/dns/nta.c | 6 +- lib/dns/qp.c | 157 +++++++++++++++++++++++++++++---------- lib/dns/zt.c | 2 +- tests/bench/qplookups.c | 8 +- tests/dns/qp_test.c | 73 +++++++++++++++++- tests/libtest/qp.c | 10 ++- 8 files changed, 210 insertions(+), 70 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 07850d176c..86ddbd3f45 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -191,10 +191,7 @@ 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]; + void *stack[DNS_QP_MAXKEY]; } dns_qpiter_t; /*% @@ -595,12 +592,17 @@ dns_qpiter_init(dns_qpreadable_t qpr, dns_qpiter_t *qpi); */ isc_result_t -dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r); +dns_qpiter_next(dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, + uint32_t *ival_r); +isc_result_t +dns_qpiter_prev(dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, + uint32_t *ival_r); /*%< - * Get the next leaf object of a trie in lexicographic order of its keys. + * Iterate forward/backward through a QP trie in lexicographic order. * * The leaf values are assigned to whichever of `*pval_r` and `*ival_r` - * are not null, unless the return value is ISC_R_NOMORE. + * are not null, unless the return value is ISC_R_NOMORE. Similarly, + * if `name` is not null, it is updated to contain the node name. * * NOTE: see the safety note under `dns_qpiter_init()`. * @@ -610,7 +612,7 @@ dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r); * void *pval; * uint32_t ival; * dns_qpiter_init(qp, &qpi); - * while (dns_qpiter_next(&qpi, &pval, &ival)) { + * while (dns_qpiter_next(&qpi, &pval, &ival) == ISC_R_SUCCESS) { * // do something with pval and ival * } * diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 8d5b51e9b0..12866f9d9f 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -155,7 +155,7 @@ destroy_keytable(dns_keytable_t *keytable) { dns_qpmulti_query(keytable->table, &qpr); dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns_keynode_t *n = pval; dns_keynode_detach(&n); } @@ -666,7 +666,7 @@ dns_keytable_totext(dns_keytable_t *keytable, isc_buffer_t **text) { dns_qpmulti_query(keytable->table, &qpr); dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns_keynode_t *knode = pval; if (knode->dslist != NULL) { result = keynode_dslist_totext(knode, text); @@ -694,7 +694,7 @@ dns_keytable_forall(dns_keytable_t *keytable, dns_qpmulti_query(keytable->table, &qpr); dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns_keynode_t *knode = pval; (*func)(keytable, knode, knode->name, arg); } diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 0d5a90ac34..51364142ea 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -480,7 +480,7 @@ dns_ntatable_totext(dns_ntatable_t *ntatable, const char *view, dns_qpmulti_query(ntatable->table, &qpr); dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns__nta_t *n = pval; char nbuf[DNS_NAME_FORMATSIZE]; char tbuf[ISC_FORMATHTTPTIMESTAMP_SIZE]; @@ -536,7 +536,7 @@ dns_ntatable_save(dns_ntatable_t *ntatable, FILE *fp) { dns_qpmulti_query(ntatable->table, &qpr); dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns__nta_t *n = pval; isc_buffer_t b; char nbuf[DNS_NAME_FORMATSIZE + 1], tbuf[80]; @@ -620,7 +620,7 @@ dns_ntatable_shutdown(dns_ntatable_t *ntatable) { ntatable->shuttingdown = true; dns_qpiter_init(&qpr, &iter); - while (dns_qpiter_next(&iter, &pval, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&iter, NULL, &pval, NULL) == ISC_R_SUCCESS) { dns__nta_t *n = pval; dns__nta_shutdown(n); dns__nta_detach(&n); diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 663ffe1a53..163793c6c7 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1609,9 +1609,10 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival) { n = ref_ptr(qp, qp->root_ref); while (is_branch(n)) { prefetch_twigs(qp, n); + qp_ref_t ref = branch_twigs_ref(n); bit = branch_keybit(n, new_key, new_keylen); pos = branch_has_twig(n, bit) ? branch_twig_pos(n, bit) : 0; - n = branch_twigs(qp, n) + pos; + n = ref_ptr(qp, ref + pos); } /* do the keys differ, and if so, where? */ @@ -1835,60 +1836,132 @@ 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; + *qpi = (dns_qpiter_t){ + .qp = qp, + .magic = QPITER_MAGIC, + }; } /* + * are we at the last twig in this branch (in whichever direction + * we're iterating)? + */ +static bool +last_twig(dns_qpiter_t *qpi, bool forward) { + qp_weight_t pos = 0, max = 0; + if (qpi->sp > 0) { + qp_node_t *child = qpi->stack[qpi->sp]; + qp_node_t *parent = qpi->stack[qpi->sp - 1]; + pos = child - ref_ptr(qpi->qp, branch_twigs_ref(parent)); + if (forward) { + max = branch_twigs_size(parent) - 1; + } + } + return (pos == max); +} + +/* + * move a QP iterator forward or back to the next or previous leaf. * 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) { +static isc_result_t +iterate(bool forward, dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, + uint32_t *ival_r) { + qp_node_t *node = NULL; + bool initial_branch = true; + REQUIRE(QPITER_VALID(qpi)); - REQUIRE(QP_VALID(qpi->qp)); dns_qpreader_t *qp = qpi->qp; - if (qpi->stack[qpi->sp].ref == INVALID_REF) { - INSIST(qpi->sp == 0); - qpi->magic = 0; + REQUIRE(QP_VALID(qp)); + + node = get_root(qp); + if (node == NULL) { 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) { - SET_IF_NOT_NULL(pval_r, leaf_pval(n)); - SET_IF_NOT_NULL(ival_r, leaf_ival(n)); - break; + do { + if (qpi->stack[qpi->sp] == NULL) { + /* newly initialized iterator: use the root node */ + INSIST(qpi->sp == 0); + qpi->stack[0] = node; + } else if (!initial_branch) { + /* + * in a prior loop, we reached a branch; from + * here we just need to get the highest or lowest + * leaf in the subtree; we don't need to bother + * stepping forward or backward through twigs + * anymore. + */ + INSIST(qpi->sp > 0); + } else if (last_twig(qpi, forward)) { + /* + * we've stepped to the end (or the beginning, + * if we're iterating backwards) of a set of twigs. + */ + if (qpi->sp == 0) { + /* + * we've finished iterating. reinitialize + * the iterator, then return ISC_R_NOMORE. + */ + dns_qpiter_init(qpi->qp, qpi); + return (ISC_R_NOMORE); + } + + /* + * pop the stack, and resume at the parent branch. + */ + qpi->stack[qpi->sp] = NULL; + qpi->sp--; + continue; + } else { + /* + * there are more twigs in the current branch, + * so step the node pointer forward (or back). + */ + node = qpi->stack[qpi->sp]; + node += (forward ? 1 : -1); + qpi->stack[qpi->sp] = node; } - 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--; - } + /* + * if we're at a branch now, push its first (or last) twig + * onto the stack and loop again. + */ + if (is_branch(node)) { + qpi->sp++; + INSIST(qpi->sp < DNS_QP_MAXKEY); - /* 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; - } + qp_node_t *twigs = ref_ptr(qp, branch_twigs_ref(node)); + if (!forward) { + twigs += branch_twigs_size(node) - 1; + } + node = qpi->stack[qpi->sp] = twigs; + initial_branch = false; + } + } while (is_branch(node)); + + /* we're at a leaf: return its data to the caller */ + SET_IF_NOT_NULL(pval_r, leaf_pval(node)); + SET_IF_NOT_NULL(ival_r, leaf_ival(node)); + maybe_set_name(qpi->qp, node, name); return (ISC_R_SUCCESS); } +isc_result_t +dns_qpiter_next(dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, + uint32_t *ival_r) { + return (iterate(true, qpi, name, pval_r, ival_r)); +} + +isc_result_t +dns_qpiter_prev(dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, + uint32_t *ival_r) { + return (iterate(false, qpi, name, pval_r, ival_r)); +} + /*********************************************************************** * * search @@ -1992,9 +2065,9 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, while (is_branch(n)) { prefetch_twigs(qp, n); - qp_node_t *twigs = branch_twigs(qp, n); offset = branch_key_offset(n); qp_shift_t bit = qpkey_bit(search, searchlen, offset); + qp_node_t *twigs = branch_twigs(qp, n); /* * A shorter key that can be a parent domain always has a @@ -2007,11 +2080,12 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, * `qpkey_bit()` will return SHIFT_NOBYTE, which is what we * want when `off == 0`. * - * Note 2: Any SHIFT_NOBYTE twig is always `twigs[0]`. + * Note 2: If SHIFT_NOBYTE twig is present, it will always + * be in position 0, the first localtion in 'twigs'. */ if (bit != SHIFT_NOBYTE && branch_has_twig(n, SHIFT_NOBYTE) && qpkey_bit(search, searchlen, offset - 1) == SHIFT_NOBYTE && - !is_branch(&twigs[0])) + !is_branch(twigs)) { add_link(chain, twigs, offset); } @@ -2022,10 +2096,11 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, /* * the twig we're looking for isn't here, * and we haven't found an ancestor yet either. - * continue the search with whatever this branch's + * but we need to end the loop at a leaf, so + * continue down from whatever this branch's * first twig is. */ - n = &twigs[0]; + n = twigs; } else { /* * this branch is a dead end, but we do have diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 2f21896015..60e5d52a08 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -516,7 +516,7 @@ dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub, dns_qpmulti_query(zt->multi, &qpr); dns_qpiter_init(&qpr, &qpi); - while (dns_qpiter_next(&qpi, &zone, NULL) == ISC_R_SUCCESS) { + while (dns_qpiter_next(&qpi, NULL, &zone, NULL) == ISC_R_SUCCESS) { result = action(zone, uap); if (tresult == ISC_R_SUCCESS) { tresult = result; diff --git a/tests/bench/qplookups.c b/tests/bench/qplookups.c index c269edb750..fd50ad37ca 100644 --- a/tests/bench/qplookups.c +++ b/tests/bench/qplookups.c @@ -204,8 +204,6 @@ main(int argc, char **argv) { dns_name_t *name = NULL; size_t i = 0, n = 0; char buf[BUFSIZ]; - void *pval = NULL; - uint32_t ival; if (argc != 2) { usage(); @@ -228,12 +226,10 @@ main(int argc, char **argv) { start = isc_time_monotonic(); for (i = 0;; i++) { - if (dns_qpiter_next(&it, &pval, &ival) != ISC_R_SUCCESS) { + name = dns_fixedname_initname(&items[i]); + if (dns_qpiter_next(&it, name, NULL, NULL) != ISC_R_SUCCESS) { break; } - - name = dns_fixedname_initname(&items[i]); - name_from_smallname(name, pval, ival); } stop = isc_time_monotonic(); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index a456e5e639..8f05cfbcdf 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -199,13 +199,21 @@ const dns_qpmethods_t qpiter_methods = { ISC_RUN_TEST_IMPL(qpiter) { dns_qp_t *qp = NULL; uint32_t item[ITER_ITEMS] = { 0 }; + uint32_t order[ITER_ITEMS] = { 0 }; + dns_qpiter_t qpi; + int inserted, n; + uint32_t ival; + void *pval = NULL; 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]; + ival = isc_random_uniform(ITER_ITEMS - 1) + 1; + pval = &item[ival]; + item[ival] = ival; + inserted = n = 0; + /* randomly insert or remove */ dns_qpkey_t key; size_t len = qpiter_makekey(key, item, pval, ival); @@ -220,12 +228,14 @@ ISC_RUN_TEST_IMPL(qpiter) { /* 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) { + while (dns_qpiter_next(&qpi, NULL, &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]); + order[inserted++] = ival; item[ival] = ~ival; prev = ival; } @@ -237,7 +247,62 @@ ISC_RUN_TEST_IMPL(qpiter) { item[ival] = ival; } } + + /* now iterate backward and check correctness */ + n = inserted; + while (dns_qpiter_prev(&qpi, NULL, NULL, &ival) == + ISC_R_SUCCESS) + { + assert_int_equal(ival, order[--n]); + } + assert_int_equal(n, 0); + + /* ...and forward again */ + while (dns_qpiter_next(&qpi, NULL, NULL, &ival) == + ISC_R_SUCCESS) + { + assert_int_equal(ival, order[n++]); + } + + assert_int_equal(n, inserted); + + /* + * if there are enough items inserted, try going + * forward a few steps, then back to the start, + * to confirm we can change directions while iterating. + */ + if (tests > 3) { + assert_int_equal( + dns_qpiter_next(&qpi, NULL, NULL, &ival), + ISC_R_SUCCESS); + assert_int_equal(ival, order[0]); + + assert_int_equal( + dns_qpiter_next(&qpi, NULL, NULL, &ival), + ISC_R_SUCCESS); + assert_int_equal(ival, order[1]); + + assert_int_equal( + dns_qpiter_prev(&qpi, NULL, NULL, &ival), + ISC_R_SUCCESS); + assert_int_equal(ival, order[0]); + + assert_int_equal( + dns_qpiter_next(&qpi, NULL, NULL, &ival), + ISC_R_SUCCESS); + assert_int_equal(ival, order[1]); + + assert_int_equal( + dns_qpiter_prev(&qpi, NULL, NULL, &ival), + ISC_R_SUCCESS); + assert_int_equal(ival, order[0]); + + assert_int_equal( + dns_qpiter_prev(&qpi, NULL, NULL, &ival), + ISC_R_NOMORE); + } } + dns_qp_destroy(&qp); } diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index be39156be1..f11d34385a 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -72,8 +72,8 @@ getheight(dns_qp_t *qp, qp_node_t *n) { return (0); } size_t max_height = 0; - qp_weight_t size = branch_twigs_size(n); qp_node_t *twigs = branch_twigs(qp, n); + qp_weight_t size = branch_twigs_size(n); for (qp_weight_t pos = 0; pos < size; pos++) { size_t height = getheight(qp, &twigs[pos]); max_height = ISC_MAX(max_height, height); @@ -94,8 +94,8 @@ maxkeylen(dns_qp_t *qp, qp_node_t *n) { return (leaf_qpkey(qp, n, key)); } size_t max_len = 0; - qp_weight_t size = branch_twigs_size(n); qp_node_t *twigs = branch_twigs(qp, n); + qp_weight_t size = branch_twigs_size(n); for (qp_weight_t pos = 0; pos < size; pos++) { size_t len = maxkeylen(qp, &twigs[pos]); max_len = ISC_MAX(max_len, len); @@ -263,8 +263,10 @@ qp_test_dumptrie(dns_qpreadable_t qpr) { --sp; } - n = ref_ptr(qp, stack[sp].ref) + stack[sp].pos; stack[sp].pos++; + fprintf(stderr, "pos %d/%d, ref+%d\n", stack[sp].pos, + stack[sp].max, stack[sp].pos - 1); + n = ref_ptr(qp, stack[sp].ref) + stack[sp].pos - 1; } } @@ -299,8 +301,8 @@ dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { } printf("}}\"];\n"); - qp_weight_t size = branch_twigs_size(n); qp_node_t *twigs = branch_twigs(qp, n); + qp_weight_t size = branch_twigs_size(n); for (qp_weight_t pos = 0; pos < size; pos++) { dumpdot_name(n); From 606232b8d5f28012fdfef870718bc1e6b5fd3ef1 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 23 Sep 2023 01:02:17 -0700 Subject: [PATCH 5/9] remove DNS_QPFIND_NOEXACT since dns_qp_findname_ancestor() can now return a chain object, it is no longer necessary to provide a _NOEXACT search option. if we want to look up the closest ancestor of a name, we can just do a normal search, and if successful, retrieve the second-to-last node from the QP chain. this makes ancestor lookups slightly more complicated for the caller, but allows us to simplify the code in dns_qp_findname_ancestor(), making it easier to ensure correctness. this was a fairly rare use case: outside of unit tests, DNS_QPFIND_NOEXACT was only used in the zone table, which has now been updated to use the QP chain. the equivalent RBT feature is only used by the resolver for cache lookups of 'atparent' types (i.e, DS records). --- lib/dns/forward.c | 3 +- lib/dns/include/dns/qp.h | 14 +---- lib/dns/keytable.c | 6 +- lib/dns/nametree.c | 2 +- lib/dns/nta.c | 3 +- lib/dns/qp.c | 39 ++---------- lib/dns/zt.c | 23 ++++--- tests/dns/qp_test.c | 127 +++++++++++++++------------------------ 8 files changed, 73 insertions(+), 144 deletions(-) diff --git a/lib/dns/forward.c b/lib/dns/forward.c index d885df68b4..fc21a1a4ce 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,8 +168,7 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 86ddbd3f45..51c46cf964 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -281,13 +281,6 @@ typedef enum dns_qpgc { DNS_QPGC_ALL, } dns_qpgc_t; -/*% - * Options for fancy searches such as `dns_qp_findname_ancestor()` - */ -typedef enum dns_qpfind { - DNS_QPFIND_NOEXACT = 1 << 0, -} dns_qpfind_t; - /*********************************************************************** * * functions - create, destory, enquire @@ -495,15 +488,12 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, dns_name_t *foundname, - dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); + dns_name_t *foundname, dns_qpchain_t *chain, + void **pval_r, uint32_t *ival_r); /*%< * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the * given DNS name. * - * If the DNS_QPFIND_NOEXACT option is set, find the closest ancestor - * domain that is not equal to the search name. - * * If 'foundname' is not NULL, it is updated to contain the name found. * * If 'chain' is not NULL, it is updated to contain a QP chain with diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 12866f9d9f..0b805fd9c4 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,8 +518,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -548,8 +547,7 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 0fd1110a16..661253f3aa 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,7 +289,7 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, found, NULL, + result = dns_qp_findname_ancestor(&qpr, name, found, NULL, (void **)&node, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { switch (nametree->type) { diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 51364142ea..4223ec27f5 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,8 +413,7 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 163793c6c7..3f25e8bae6 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -360,25 +360,6 @@ 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 @@ -2023,27 +2004,19 @@ add_link(dns_qpchain_t *chain, qp_node_t *node, size_t offset) { isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, dns_name_t *foundname, - dns_qpchain_t *chain, void **pval_r, - uint32_t *ival_r) { + dns_name_t *foundname, dns_qpchain_t *chain, + 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_node_t *n = NULL; - isc_result_t result; dns_qpchain_t oc; REQUIRE(QP_VALID(qp)); REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); 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; - } if (chain == NULL) { chain = &oc; @@ -2121,11 +2094,9 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, SET_IF_NOT_NULL(ival_r, leaf_ival(n)); maybe_set_name(qp, n, foundname); if (offset == QPKEY_EQUAL) { - if ((options & DNS_QPFIND_NOEXACT) == 0) { - /* add the exact match to the chain */ - add_link(chain, n, offset); - } - return (result); + /* add the exact match to the chain */ + add_link(chain, n, offset); + return (ISC_R_SUCCESS); } else { return (DNS_R_PARTIALMATCH); } diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 60e5d52a08..7d317506fb 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -171,21 +171,26 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, void *pval = NULL; dns_ztfind_t exactmask = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_EXACT; dns_ztfind_t exactopts = options & exactmask; + dns_qpchain_t chain; REQUIRE(VALID_ZT(zt)); REQUIRE(exactopts != exactmask); dns_qpmulti_query(zt->multi, &qpr); - if (exactopts == DNS_ZTFIND_EXACT) { - result = dns_qp_getname(&qpr, name, &pval, NULL); - } else if (exactopts == DNS_ZTFIND_NOEXACT) { - result = dns_qp_findname_ancestor(&qpr, name, - DNS_QPFIND_NOEXACT, NULL, - NULL, &pval, NULL); - } else { - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, - &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, &chain, &pval, + NULL); + if (exactopts == DNS_ZTFIND_EXACT && result == DNS_R_PARTIALMATCH) { + result = ISC_R_NOTFOUND; + } else if (exactopts == DNS_ZTFIND_NOEXACT && result == ISC_R_SUCCESS) { + /* get pval from the previous chain link */ + int len = dns_qpchain_length(&chain); + if (len >= 2) { + dns_qpchain_node(&chain, len - 2, NULL, &pval, NULL); + result = DNS_R_PARTIALMATCH; + } else { + result = ISC_R_NOTFOUND; + } } dns_qpread_destroy(zt->multi, &qpr); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 8f05cfbcdf..782383d1a3 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -335,7 +335,6 @@ const dns_qpmethods_t string_methods = { struct check_partialmatch { const char *query; - dns_qpfind_t options; isc_result_t result; const char *found; }; @@ -350,13 +349,13 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { void *pval = NULL; dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, check[i].options, - foundname, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(qp, name, foundname, NULL, + &pval, NULL); #if 0 - fprintf(stderr, "%s (flags %u) %s (expected %s) " + fprintf(stderr, "%s %s (expected %s) " "value \"%s\" (expected \"%s\")\n", - check[i].query, check[i].options, + check[i].query, isc_result_totext(result), isc_result_totext(check[i].result), (char *)pval, check[i].found); @@ -426,26 +425,19 @@ ISC_RUN_TEST_IMPL(partialmatch) { } 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." }, - { "my.other.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, - "foo.bar." }, - { NULL, 0, 0, NULL }, + { "a.b.", ISC_R_SUCCESS, "a.b." }, + { "b.c.", ISC_R_NOTFOUND, NULL }, + { "bar.", ISC_R_NOTFOUND, NULL }, + { "f.bar.", ISC_R_NOTFOUND, NULL }, + { "foo.bar.", ISC_R_SUCCESS, "foo.bar." }, + { "foooo.bar.", ISC_R_NOTFOUND, NULL }, + { "w.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "www.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "web.foo.bar.", ISC_R_SUCCESS, "web.foo.bar." }, + { "webby.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "my.web.foo.bar.", DNS_R_PARTIALMATCH, "web.foo.bar." }, + { "my.other.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { NULL }, }; check_partialmatch(qp, check1); @@ -454,13 +446,11 @@ ISC_RUN_TEST_IMPL(partialmatch) { 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, "." }, - { "bar", 0, ISC_R_NOTFOUND, NULL }, - { "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, - { NULL, 0, 0, NULL }, + { "b.c.", DNS_R_PARTIALMATCH, "." }, + { "bar.", DNS_R_PARTIALMATCH, "." }, + { "foo.bar.", ISC_R_SUCCESS, "foo.bar." }, + { "bar", ISC_R_NOTFOUND, NULL }, + { NULL }, }; check_partialmatch(qp, check2); @@ -471,35 +461,26 @@ ISC_RUN_TEST_IMPL(partialmatch) { dns_qpkey_t rootkey = { SHIFT_NOBYTE }; result = dns_qp_deletekey(qp, rootkey, 1, NULL, NULL); assert_int_equal(result, ISC_R_SUCCESS); - check_partialmatch( - qp, - (struct check_partialmatch[]){ - { "bar", 0, ISC_R_NOTFOUND, NULL }, - { "bar", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { "bar.", 0, ISC_R_NOTFOUND, NULL }, - { "bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { NULL, 0, 0, NULL }, - }); + check_partialmatch(qp, (struct check_partialmatch[]){ + { "bar", ISC_R_NOTFOUND, NULL }, + { "bar.", ISC_R_NOTFOUND, NULL }, + { NULL }, + }); /* what if there's a root node with an empty key? */ INSIST(insert[i][0] == '\0'); insert_str(qp, insert[i++]); - check_partialmatch( - qp, - (struct check_partialmatch[]){ - { "bar", 0, DNS_R_PARTIALMATCH, "" }, - { "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, - { "bar.", 0, DNS_R_PARTIALMATCH, "" }, - { "bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, - { NULL, 0, 0, NULL }, - }); + check_partialmatch(qp, (struct check_partialmatch[]){ + { "bar", DNS_R_PARTIALMATCH, "" }, + { "bar.", DNS_R_PARTIALMATCH, "" }, + { NULL }, + }); dns_qp_destroy(&qp); } struct check_qpchain { const char *query; - dns_qpfind_t options; isc_result_t result; unsigned int length; const char *names[10]; @@ -515,13 +496,12 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { dns_qpchain_init(qp, &chain); dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, check[i].options, - NULL, &chain, NULL, NULL); + result = dns_qp_findname_ancestor(qp, name, NULL, &chain, NULL, + NULL); #if 0 - fprintf(stderr, "%s (%d) %s (expected %s), " - "len %d (expected %d)\n", - check[i].query, check[i].options, + fprintf(stderr, "%s %s (expected %s), " + "len %d (expected %d)\n", check[i].query, isc_result_totext(result), isc_result_totext(check[i].result), dns_qpchain_length(&chain), check[i].length); @@ -557,38 +537,25 @@ ISC_RUN_TEST_IMPL(qpchain) { */ const char insert[][16] = { ".", "a.", "b.", "c.b.a.", "e.d.c.b.a.", "c.b.b.", "c.d.", "a.b.c.d.", - "a.b.c.d.e.", "" }; + "a.b.c.d.e.", "b.a.", "" }; while (insert[i][0] != '\0') { insert_str(qp, insert[i++]); } static struct check_qpchain check1[] = { - { "b.", 0, ISC_R_SUCCESS, 2 }, - { "c.", 0, DNS_R_PARTIALMATCH, 1 }, - { "e.d.c.b.a.", 0, ISC_R_SUCCESS, 4 }, - { "a.b.c.d.", 0, ISC_R_SUCCESS, 3 }, - { "a.b.c.d.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, 2 }, - { NULL, 0, 0, 0 }, + { "b.", ISC_R_SUCCESS, 2, { ".", "b." } }, + { "b.a.", ISC_R_SUCCESS, 3, { ".", "a.", "b.a." } }, + { "c.", DNS_R_PARTIALMATCH, 1, { "." } }, + { "e.d.c.b.a.", + ISC_R_SUCCESS, + 5, + { ".", "a.", "b.a.", "c.b.a.", "e.d.c.b.a." } }, + { "a.b.c.d.", ISC_R_SUCCESS, 3, { ".", "c.d.", "a.b.c.d." } }, + { "b.c.d.", DNS_R_PARTIALMATCH, 2, { ".", "c.d." } }, + { NULL, 0, 0, { NULL } }, }; - check1[0].names[0] = "."; - check1[0].names[1] = "b."; - - check1[1].names[0] = "."; - - check1[2].names[0] = "."; - check1[2].names[1] = "a."; - check1[2].names[2] = "c.b.a."; - check1[2].names[3] = "e.d.c.b.a."; - - check1[3].names[0] = "."; - check1[3].names[1] = "c.d."; - check1[3].names[2] = "a.b.c.d."; - - check1[4].names[0] = "."; - check1[4].names[1] = "c.d."; - check_qpchain(qp, check1); dns_qp_destroy(&qp); } From 4e3e61806ca642714311408c446a618d5879652d Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 23 Sep 2023 15:58:46 -0700 Subject: [PATCH 6/9] get predecessor name in dns_qp_findname_ancestor() dns_qp_findname_ancestor() now takes an optional 'predecessor' parameter, which if non-NULL is updated to contain the DNSSEC predecessor of the name searched for. this is done by constructing an iterator stack while carrying out the search, so it can be used to step backward if needed. --- lib/dns/forward.c | 3 +- lib/dns/include/dns/qp.h | 7 ++- lib/dns/keytable.c | 6 +- lib/dns/nametree.c | 2 +- lib/dns/nta.c | 3 +- lib/dns/qp.c | 111 +++++++++++++++++++++++++++------ lib/dns/zt.c | 2 +- tests/dns/qp_test.c | 128 ++++++++++++++++++++++++++++++++++----- 8 files changed, 223 insertions(+), 39 deletions(-) diff --git a/lib/dns/forward.c b/lib/dns/forward.c index fc21a1a4ce..0be252f3ac 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,7 +168,8 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 51c46cf964..3e057ceccd 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -488,14 +488,17 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_name_t *foundname, dns_qpchain_t *chain, - void **pval_r, uint32_t *ival_r); + dns_name_t *foundname, dns_name_t *predecessor, + dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); /*%< * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the * given DNS name. * * If 'foundname' is not NULL, it is updated to contain the name found. * + * If 'predecessor' is not NULL, it is updated to contain the DNSSEC + * predecessor of the searched-for name. + * * If 'chain' is not NULL, it is updated to contain a QP chain with * references to the populated nodes in the tree between the root and * the name found. diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 0b805fd9c4..e8ce21fa19 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,7 +518,8 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, + NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -547,7 +548,8 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 661253f3aa..60fa9a97f0 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,7 +289,7 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, found, NULL, + result = dns_qp_findname_ancestor(&qpr, name, found, NULL, NULL, (void **)&node, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { switch (nametree->type) { diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 4223ec27f5..c86b0ebf7b 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,7 +413,8 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, + NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 3f25e8bae6..299272cf6c 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1771,6 +1771,7 @@ maybe_set_name(dns_qpreader_t *qp, qp_node_t *node, dns_name_t *name) { return; } + dns_name_reset(name); len = leaf_qpkey(qp, node, key); dns_qpkey_toname(key, len, name); } @@ -1996,22 +1997,41 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, static inline void add_link(dns_qpchain_t *chain, qp_node_t *node, size_t offset) { + /* + * prevent duplication, which could occur if we're adding a link + * for the predecessor node and it's the same as the parent. + */ + if (chain->len > 0 && node == chain->chain[chain->len - 1].node) { + return; + } chain->chain[chain->len].node = node; chain->chain[chain->len].offset = offset; chain->len++; INSIST(chain->len <= DNS_NAME_MAXLABELS); } +static inline void +prevleaf(dns_qpiter_t *it) { + isc_result_t result = dns_qpiter_prev(it, NULL, NULL, NULL); + if (result == ISC_R_NOMORE) { + result = dns_qpiter_prev(it, NULL, NULL, NULL); + } + RUNTIME_CHECK(result == ISC_R_SUCCESS); +} + isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_name_t *foundname, dns_qpchain_t *chain, - void **pval_r, uint32_t *ival_r) { + dns_name_t *foundname, dns_name_t *predecessor, + dns_qpchain_t *chain, 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_node_t *n = NULL; dns_qpchain_t oc; + dns_qpiter_t it; + bool matched = true; REQUIRE(QP_VALID(qp)); REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); @@ -2022,11 +2042,13 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, chain = &oc; } dns_qpchain_init(qp, chain); + dns_qpiter_init(qp, &it); n = get_root(qp); if (n == NULL) { return (ISC_R_NOTFOUND); } + it.stack[0] = n; /* * Like `dns_qp_insert()`, we must find a leaf. However, we don't make a @@ -2062,33 +2084,88 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, { add_link(chain, twigs, offset); } - if (branch_has_twig(n, bit)) { - /* we have the twig we wanted */ - n = branch_twig_ptr(qp, n, bit); - } else if (chain->len == 0) { + + matched = branch_has_twig(n, bit); + if (matched) { /* - * the twig we're looking for isn't here, - * and we haven't found an ancestor yet either. - * but we need to end the loop at a leaf, so - * continue down from whatever this branch's - * first twig is. + * found a match: if it's a branch, we keep + * searching, and if it's a leaf, we drop out of + * the loop. */ - n = twigs; + n = branch_twig_ptr(qp, n, bit); + } else if (predecessor != NULL) { + /* + * this branch is a dead end, but the caller wants + * the predecessor to the name we were searching + * for, so let's go find that. + */ + qp_weight_t pos = branch_twig_pos(n, bit); + if (pos == 0) { + /* + * this entire branch is greater than + * the key we wanted, so we step back to + * the predecessor using the iterator. + */ + prevleaf(&it); + n = it.stack[it.sp]; + } else { + /* + * the name we want would've been between + * two twigs in this branch. point n to the + * lesser of those, then walk down to the + * highest leaf in that subtree to get the + * predecessor. + */ + n = twigs + pos - 1; + while (is_branch(n)) { + prefetch_twigs(qp, n); + it.stack[++it.sp] = n; + pos = branch_twigs_size(n) - 1; + n = ref_ptr(qp, + branch_twigs_ref(n) + pos); + } + } } else { /* - * this branch is a dead end, but we do have - * a previous leaf node saved in the chain. - * we go back to that. it's a leaf, so we'll - * fall out of the loop here. + * this branch is a dead end, and the predecessor + * doesn't matter. now we just need to find a leaf + * to end on so qpkey_leaf() will work below. */ - n = chain->chain[chain->len - 1].node; + if (chain->len > 0) { + /* we saved an ancestor leaf: use that */ + n = chain->chain[chain->len - 1].node; + } else { + /* walk down to find the leftmost leaf */ + while (is_branch(twigs)) { + twigs = branch_twigs(qp, twigs); + } + n = twigs; + } } + + it.stack[++it.sp] = n; } /* do the keys differ, and if so, where? */ foundlen = leaf_qpkey(qp, n, found); offset = qpkey_compare(search, searchlen, found, foundlen); + /* + * if we've been asked to return the predecessor name, we + * work that out here. + * + * if 'matched' is true, the search ended at a leaf. it's either + * an exact match or the immediate successor of the searched-for + * name, and in either case, we can use the qpiter stack we've + * constructed to step back to the predecessor. if 'matched' is + * false, then the search failed at a branch node, and we would + * have already found the predecessor. + */ + if (predecessor != NULL && matched) { + prevleaf(&it); + } + maybe_set_name(qp, it.stack[it.sp], predecessor); + if (offset == QPKEY_EQUAL || offset == foundlen) { SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 7d317506fb..147e6fc830 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -178,7 +178,7 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, dns_qpmulti_query(zt->multi, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, &chain, &pval, + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &chain, &pval, NULL); if (exactopts == DNS_ZTFIND_EXACT && result == DNS_R_PARTIALMATCH) { result = ISC_R_NOTFOUND; diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 782383d1a3..6c83f8f365 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -350,7 +350,7 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { dns_test_namefromstring(check[i].query, &fn1); result = dns_qp_findname_ancestor(qp, name, foundname, NULL, - &pval, NULL); + NULL, &pval, NULL); #if 0 fprintf(stderr, "%s %s (expected %s) " @@ -437,7 +437,7 @@ ISC_RUN_TEST_IMPL(partialmatch) { { "webby.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, { "my.web.foo.bar.", DNS_R_PARTIALMATCH, "web.foo.bar." }, { "my.other.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, - { NULL }, + { NULL, 0, NULL }, }; check_partialmatch(qp, check1); @@ -450,7 +450,7 @@ ISC_RUN_TEST_IMPL(partialmatch) { { "bar.", DNS_R_PARTIALMATCH, "." }, { "foo.bar.", ISC_R_SUCCESS, "foo.bar." }, { "bar", ISC_R_NOTFOUND, NULL }, - { NULL }, + { NULL, 0, NULL }, }; check_partialmatch(qp, check2); @@ -464,7 +464,7 @@ ISC_RUN_TEST_IMPL(partialmatch) { check_partialmatch(qp, (struct check_partialmatch[]){ { "bar", ISC_R_NOTFOUND, NULL }, { "bar.", ISC_R_NOTFOUND, NULL }, - { NULL }, + { NULL, 0, NULL }, }); /* what if there's a root node with an empty key? */ @@ -473,7 +473,7 @@ ISC_RUN_TEST_IMPL(partialmatch) { check_partialmatch(qp, (struct check_partialmatch[]){ { "bar", DNS_R_PARTIALMATCH, "" }, { "bar.", DNS_R_PARTIALMATCH, "" }, - { NULL }, + { NULL, 0, NULL }, }); dns_qp_destroy(&qp); @@ -496,8 +496,8 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { dns_qpchain_init(qp, &chain); dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, NULL, &chain, NULL, - NULL); + result = dns_qp_findname_ancestor(qp, name, NULL, NULL, &chain, + NULL, NULL); #if 0 fprintf(stderr, "%s %s (expected %s), " @@ -528,17 +528,14 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { ISC_RUN_TEST_IMPL(qpchain) { dns_qp_t *qp = NULL; + const char insert[][16] = { ".", "a.", "b.", + "c.b.a.", "e.d.c.b.a.", "c.b.b.", + "c.d.", "a.b.c.d.", "a.b.c.d.e.", + "b.a.", "x.k.c.d.", "" }; int i = 0; dns_qp_create(mctx, &string_methods, NULL, &qp); - /* - * Fixed size strings [16] should ensure leaf-compatible alignment. - */ - const char insert[][16] = { ".", "a.", "b.", "c.b.a.", - "e.d.c.b.a.", "c.b.b.", "c.d.", "a.b.c.d.", - "a.b.c.d.e.", "b.a.", "" }; - while (insert[i][0] != '\0') { insert_str(qp, insert[i++]); } @@ -560,12 +557,115 @@ ISC_RUN_TEST_IMPL(qpchain) { dns_qp_destroy(&qp); } +struct check_predecessors { + const char *query; + const char *predecessor; + isc_result_t result; +}; + +static void +check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { + isc_result_t result; + dns_fixedname_t fn1, fn2; + dns_name_t *name = dns_fixedname_initname(&fn1); + dns_name_t *pred = dns_fixedname_initname(&fn2); + + for (int i = 0; check[i].query != NULL; i++) { + char *predname = NULL; + + dns_test_namefromstring(check[i].query, &fn1); + result = dns_qp_findname_ancestor(qp, name, NULL, pred, NULL, + NULL, NULL); +#if 0 + fprintf(stderr, "%s: expected %s got %s\n", check[i].query, + isc_result_totext(check[i].result), + isc_result_totext(result)); +#endif + assert_int_equal(result, check[i].result); + result = dns_name_tostring(pred, &predname, mctx); +#if 0 + fprintf(stderr, "... expected predecessor %s got %s\n", + check[i].predecessor, predname); +#endif + assert_int_equal(result, ISC_R_SUCCESS); + + assert_string_equal(predname, check[i].predecessor); + isc_mem_free(mctx, predname); + } +} + +ISC_RUN_TEST_IMPL(predecessors) { + dns_qp_t *qp = NULL; + const char insert[][16] = { + "a.", "b.", "c.b.a.", "e.d.c.b.a.", + "c.b.b.", "c.d.", "a.b.c.d.", "a.b.c.d.e.", + "b.a.", "x.k.c.d.", "" + }; + int i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + while (insert[i][0] != '\0') { + insert_str(qp, insert[i++]); + } + + /* first check: no root label in the database */ + static struct check_predecessors check1[] = { + { ".", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "a.", "a.b.c.d.e.", ISC_R_SUCCESS }, + { "b.a.", "a.", ISC_R_SUCCESS }, + { "b.", "e.d.c.b.a.", ISC_R_SUCCESS }, + { "aaa.a.", "a.", DNS_R_PARTIALMATCH }, + { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, + { "d.c.", "c.b.b.", ISC_R_NOTFOUND }, + { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH }, + { "a.b.c.e.f.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "z.y.x.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, + { "z.z.z.z.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, + { "0.b.c.d.e.", "x.k.c.d.", ISC_R_NOTFOUND }, + { "b.d.", "c.b.b.", ISC_R_NOTFOUND }, + { NULL, NULL, 0 } + }; + + check_predecessors(qp, check1); + + /* second check: add a root label and try again */ + const char root[16] = "."; + insert_str(qp, root); + + static struct check_predecessors check2[] = { + { ".", "a.b.c.d.e.", ISC_R_SUCCESS }, + { "a.", ".", ISC_R_SUCCESS }, + { "b.a.", "a.", ISC_R_SUCCESS }, + { "b.", "e.d.c.b.a.", ISC_R_SUCCESS }, + { "aaa.a.", "a.", DNS_R_PARTIALMATCH }, + { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, + { "d.c.", "c.b.b.", DNS_R_PARTIALMATCH }, + { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH }, + { "a.b.c.e.f.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "z.y.x.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, + { "z.z.z.z.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, + { "0.b.c.d.e.", "x.k.c.d.", DNS_R_PARTIALMATCH }, + { NULL, NULL, 0 } + }; + + check_predecessors(qp, check2); + + 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_ENTRY(qpchain) +ISC_TEST_ENTRY(predecessors) ISC_TEST_LIST_END ISC_TEST_MAIN From 6231fd66af86dbf765267c1895857ac6b346a114 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 26 Sep 2023 13:38:12 -0700 Subject: [PATCH 7/9] rename QP-related types to use standard BIND nomenclature changed type names in QP trie code to match the usual convention: - qp_node_t -> dns_qpnode_t - qp_ref_t -> dns_qpref_t - qp_shift_t -> dns_qpshift_t - qp_weight_t -> dns_qpweight_t - qp_chunk_t -> dns_qpchunk_t - qp_cell_t -> dns_qpcell_t --- lib/dns/include/dns/qp.h | 64 +++++++++-- lib/dns/include/dns/types.h | 1 + lib/dns/qp.c | 214 +++++++++++++++++------------------- lib/dns/qp_p.h | 198 +++++++++++++-------------------- tests/libtest/qp.c | 68 ++++++------ 5 files changed, 266 insertions(+), 279 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 3e057ceccd..f38eee37ac 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -116,12 +116,18 @@ typedef struct dns_qpmulti dns_qpmulti_t; */ #define DNS_QPREADER_FIELDS \ uint32_t magic; \ - uint32_t root_ref; \ + dns_qpref_t root_ref; \ dns_qpbase_t *base; \ void *uctx; \ const struct dns_qpmethods *methods -typedef struct dns_qpbase dns_qpbase_t; /* private */ +typedef struct dns_qpbase dns_qpbase_t; /* private, declared in qp_p.h */ + +/*% + * A unique twig reference; this can be converted to chunk and cell + * values to find a specific location. + */ +typedef uint32_t dns_qpref_t; typedef struct dns_qpreader { DNS_QPREADER_FIELDS; @@ -174,39 +180,73 @@ typedef union dns_qpreadable { */ #define DNS_QP_MAXKEY 512 +/* + * C is not strict enough with its integer types for the following typedefs + * to improve type safety, but it helps to have annotations saying what + * particular kind of number we are dealing with. + */ + +/*% + * The bit number, or position of a bit inside a word. (Valid values 0..63) + * A dns_qpkey_t (below) is an array of these; each element within dns_qpkey + * must satisfy: + * + * SHIFT_NOBYTE <= key[off] && key[off] < SHIFT_OFFSET + */ +typedef uint8_t dns_qpshift_t; + +/*% + * The number of bits set in a word (i.e, Hamming weight or popcount). + * This is used to determine the position of a node in the packed sparse + * vector of twigs. Valid values are 0..47 (because our bitmap does not + * fill the entire word). + */ +typedef uint8_t dns_qpweight_t; + +/* + * Chunk and cell numbers, used to identify a specific location in + * one of the chunks stored in the QP base pointer array. Each cell + * within a chunk can contain a node. + */ +typedef uint32_t dns_qpchunk_t; +typedef uint32_t dns_qpcell_t; + /*% * 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]; +typedef dns_qpshift_t dns_qpkey_t[DNS_QP_MAXKEY]; /*% * A QP iterator traverses a trie starting with the root and passing - * though each leaf node in lexicographic order; it is used with - * `dns_qpiter_init()` and `dns_qpiter_next()`. + * though each leaf node in lexicographic order; it is used by + * `dns_qpiter_init()` and `dns_qpiter_next()`. It is also used + * internally by `dns_qp_findname_iterator()` to locate the predecessor + * of a searched-for name. */ typedef struct dns_qpiter { unsigned int magic; dns_qpreader_t *qp; uint16_t sp; - void *stack[DNS_QP_MAXKEY]; + dns_qpnode_t *stack[DNS_QP_MAXKEY]; } dns_qpiter_t; /*% - * A QP chain holds references to each populated node between - * the root and a given leaf. It is set while running - * `dns_qp_findname_ancestor()`, and can optionally be passed - * back to the caller so that individual nodes can be accessed. + * A QP chain holds references to each populated node between the root and + * a given leaf. It is used internally by `dns_qp_findname_ancestor()` to + * return a partial match if the specific name requested is not found; + * optionally it can be passed back to the caller so that individual nodes + * can be accessed. */ typedef struct dns_qpchain { unsigned int magic; dns_qpreader_t *qp; uint8_t len; struct { - void *node; - size_t offset; + dns_qpnode_t *node; + size_t offset; } chain[DNS_NAME_MAXLABELS]; } dns_qpchain_t; diff --git a/lib/dns/include/dns/types.h b/lib/dns/include/dns/types.h index f3fd6f630e..89e6b15f78 100644 --- a/lib/dns/include/dns/types.h +++ b/lib/dns/include/dns/types.h @@ -144,6 +144,7 @@ typedef struct dns_request dns_request_t; typedef struct dns_requestmgr dns_requestmgr_t; typedef struct dns_resolver dns_resolver_t; typedef struct dns_rpsdb dns_rpsdb_t; +typedef struct dns_qpnode dns_qpnode_t; typedef uint8_t dns_secalg_t; typedef uint8_t dns_secproto_t; typedef struct dns_signature dns_signature_t; diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 299272cf6c..c554c85d23 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -158,9 +158,9 @@ initialize_bits_for_byte(void) { /* zero common character marker not a valid shift position */ INSIST(0 < SHIFT_BITMAP); /* first bit is common byte or escape byte */ - qp_shift_t bit_one = SHIFT_BITMAP; + dns_qpshift_t bit_one = SHIFT_BITMAP; /* second bit is position in escaped range */ - qp_shift_t bit_two = SHIFT_BITMAP; + dns_qpshift_t bit_two = SHIFT_BITMAP; bool escaping = true; for (unsigned int byte = 0; byte < BYTE_VALUES; byte++) { @@ -171,10 +171,10 @@ initialize_bits_for_byte(void) { dns_qp_bits_for_byte[byte] = bit_one; } else if ('A' <= byte && byte <= 'Z') { /* map upper case to lower case */ - qp_shift_t after_esc = bit_one + 1; - qp_shift_t skip_punct = 'a' - '_'; - qp_shift_t letter = byte - 'A'; - qp_shift_t bit = after_esc + skip_punct + letter; + dns_qpshift_t after_esc = bit_one + 1; + dns_qpshift_t skip_punct = 'a' - '_'; + dns_qpshift_t letter = byte - 'A'; + dns_qpshift_t bit = after_esc + skip_punct + letter; dns_qp_bits_for_byte[byte] = bit; /* to simplify reverse conversion */ bit_two++; @@ -413,7 +413,7 @@ chunk_shrink_raw(dns_qp_t *qp, void *ptr, size_t bytes) { } static void -write_protect(dns_qp_t *qp, qp_chunk_t chunk) { +write_protect(dns_qp_t *qp, dns_qpchunk_t chunk) { if (qp->write_protect) { /* see transaction_open() wrt this special case */ if (qp->transaction_mode == QP_WRITE && chunk == qp->bump) { @@ -447,9 +447,9 @@ write_protect(dns_qp_t *qp, qp_chunk_t chunk) { * it can have an immutable prefix and a mutable suffix. */ static inline bool -cells_immutable(dns_qp_t *qp, qp_ref_t ref) { - qp_chunk_t chunk = ref_chunk(ref); - qp_cell_t cell = ref_cell(ref); +cells_immutable(dns_qp_t *qp, dns_qpref_t ref) { + dns_qpchunk_t chunk = ref_chunk(ref); + dns_qpcell_t cell = ref_cell(ref); if (chunk == qp->bump) { return (cell < qp->fender); } else { @@ -460,8 +460,8 @@ cells_immutable(dns_qp_t *qp, qp_ref_t ref) { /* * Create a fresh bump chunk and allocate some twigs from it. */ -static qp_ref_t -chunk_alloc(dns_qp_t *qp, qp_chunk_t chunk, qp_weight_t size) { +static dns_qpref_t +chunk_alloc(dns_qp_t *qp, dns_qpchunk_t chunk, dns_qpweight_t size) { INSIST(qp->base->ptr[chunk] == NULL); INSIST(qp->usage[chunk].used == 0); INSIST(qp->usage[chunk].free == 0); @@ -486,7 +486,7 @@ chunk_alloc(dns_qp_t *qp, qp_chunk_t chunk, qp_weight_t size) { * The isc_refcount_init() and qpbase_unref() in this function are a pair. */ static void -realloc_chunk_arrays(dns_qp_t *qp, qp_chunk_t newmax) { +realloc_chunk_arrays(dns_qp_t *qp, dns_qpchunk_t newmax) { size_t oldptrs = sizeof(qp->base->ptr[0]) * qp->chunk_max; size_t newptrs = sizeof(qp->base->ptr[0]) * newmax; size_t size = STRUCT_FLEX_SIZE(qp->base, ptr, newmax); @@ -517,9 +517,9 @@ realloc_chunk_arrays(dns_qp_t *qp, qp_chunk_t newmax) { * There was no space in the bump chunk, so find a place to put a fresh * chunk in the chunk arrays, then allocate some twigs from it. */ -static qp_ref_t -alloc_slow(dns_qp_t *qp, qp_weight_t size) { - qp_chunk_t chunk; +static dns_qpref_t +alloc_slow(dns_qp_t *qp, dns_qpweight_t size) { + dns_qpchunk_t chunk; for (chunk = 0; chunk < qp->chunk_max; chunk++) { if (!qp->usage[chunk].exists) { @@ -542,10 +542,10 @@ alloc_reset(dns_qp_t *qp) { /* * Allocate some fresh twigs. This is the bump allocator fast path. */ -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; +static inline dns_qpref_t +alloc_twigs(dns_qp_t *qp, dns_qpweight_t size) { + dns_qpchunk_t chunk = qp->bump; + dns_qpcell_t cell = qp->usage[chunk].used; if (cell + size <= QP_CHUNK_SIZE) { qp->usage[chunk].used += size; @@ -567,8 +567,8 @@ alloc_twigs(dns_qp_t *qp, qp_weight_t size) { * leaves as required. */ static inline bool -free_twigs(dns_qp_t *qp, qp_ref_t twigs, qp_weight_t size) { - qp_chunk_t chunk = ref_chunk(twigs); +free_twigs(dns_qp_t *qp, dns_qpref_t twigs, dns_qpweight_t size) { + dns_qpchunk_t chunk = ref_chunk(twigs); qp->free_count += size; qp->usage[chunk].free += size; @@ -591,8 +591,8 @@ free_twigs(dns_qp_t *qp, qp_ref_t twigs, qp_weight_t size) { * on any leaves that were duplicated. */ static void -attach_twigs(dns_qp_t *qp, qp_node_t *twigs, qp_weight_t size) { - for (qp_weight_t pos = 0; pos < size; pos++) { +attach_twigs(dns_qp_t *qp, dns_qpnode_t *twigs, dns_qpweight_t size) { + for (dns_qpweight_t pos = 0; pos < size; pos++) { if (node_tag(&twigs[pos]) == LEAF_TAG) { attach_leaf(qp, &twigs[pos]); } @@ -607,8 +607,8 @@ attach_twigs(dns_qp_t *qp, qp_node_t *twigs, qp_weight_t size) { /* * Is any of this chunk still in use? */ -static inline qp_cell_t -chunk_usage(dns_qp_t *qp, qp_chunk_t chunk) { +static inline dns_qpcell_t +chunk_usage(dns_qp_t *qp, dns_qpchunk_t chunk) { return (qp->usage[chunk].used - qp->usage[chunk].free); } @@ -618,7 +618,7 @@ chunk_usage(dns_qp_t *qp, qp_chunk_t chunk) { * the chunk's phase to avoid discounting it twice in the latter case. */ static void -chunk_discount(dns_qp_t *qp, qp_chunk_t chunk) { +chunk_discount(dns_qp_t *qp, dns_qpchunk_t chunk) { if (qp->usage[chunk].discounted) { return; } @@ -634,13 +634,15 @@ chunk_discount(dns_qp_t *qp, qp_chunk_t chunk) { * remain, and free any `base` arrays that have been marked as unused. */ static void -chunk_free(dns_qp_t *qp, qp_chunk_t chunk) { +chunk_free(dns_qp_t *qp, dns_qpchunk_t chunk) { if (qp->write_protect) { TRACE("chunk %u base %p", chunk, qp->base->ptr[chunk]); } - qp_node_t *n = qp->base->ptr[chunk]; - for (qp_cell_t count = qp->usage[chunk].used; count > 0; count--, n++) { + dns_qpnode_t *n = qp->base->ptr[chunk]; + for (dns_qpcell_t count = qp->usage[chunk].used; count > 0; + count--, n++) + { if (node_tag(n) == LEAF_TAG && node_pointer(n) != NULL) { detach_leaf(qp, n); } else if (count > 1 && reader_valid(n)) { @@ -667,7 +669,7 @@ recycle(dns_qp_t *qp) { isc_nanosecs_t start = isc_time_monotonic(); - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (chunk != qp->bump && chunk_usage(qp, chunk) == 0 && qp->usage[chunk].exists && !qp->usage[chunk].immutable) { @@ -706,7 +708,7 @@ reclaim_chunks_cb(struct rcu_head *arg) { isc_nanosecs_t start = isc_time_monotonic(); for (unsigned int i = 0; i < rcuctx->count; i++) { - qp_chunk_t chunk = rcuctx->chunk[i]; + dns_qpchunk_t chunk = rcuctx->chunk[i]; if (qp->usage[chunk].snapshot) { /* cleanup when snapshot is destroyed */ qp->usage[chunk].snapfree = true; @@ -741,7 +743,7 @@ reclaim_chunks(dns_qpmulti_t *multi) { dns_qp_t *qp = &multi->writer; unsigned int count = 0; - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (chunk != qp->bump && chunk_usage(qp, chunk) == 0 && qp->usage[chunk].exists && qp->usage[chunk].immutable && !qp->usage[chunk].discounted) @@ -764,7 +766,7 @@ reclaim_chunks(dns_qpmulti_t *multi) { isc_mem_attach(qp->mctx, &rcuctx->mctx); unsigned int i = 0; - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (chunk != qp->bump && chunk_usage(qp, chunk) == 0 && qp->usage[chunk].exists && qp->usage[chunk].immutable && !qp->usage[chunk].discounted) @@ -794,7 +796,7 @@ marksweep_chunks(dns_qpmulti_t *multi) { for (dns_qpsnap_t *qps = ISC_LIST_HEAD(multi->snapshots); qps != NULL; qps = ISC_LIST_NEXT(qps, link)) { - for (qp_chunk_t chunk = 0; chunk < qps->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qps->chunk_max; chunk++) { if (qps->base->ptr[chunk] != NULL) { INSIST(qps->base->ptr[chunk] == qpw->base->ptr[chunk]); @@ -803,7 +805,7 @@ marksweep_chunks(dns_qpmulti_t *multi) { } } - for (qp_chunk_t chunk = 0; chunk < qpw->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qpw->chunk_max; chunk++) { qpw->usage[chunk].snapshot = qpw->usage[chunk].snapmark; qpw->usage[chunk].snapmark = false; if (qpw->usage[chunk].snapfree && !qpw->usage[chunk].snapshot) { @@ -838,13 +840,13 @@ marksweep_chunks(dns_qpmulti_t *multi) { * If free_twigs() could not immediately destroy the old twigs, we have * to re-attach to any leaves. */ -static qp_ref_t -evacuate(dns_qp_t *qp, qp_node_t *n) { - qp_weight_t size = branch_twigs_size(n); - qp_ref_t old_ref = branch_twigs_ref(n); - qp_ref_t new_ref = alloc_twigs(qp, size); - qp_node_t *old_twigs = ref_ptr(qp, old_ref); - qp_node_t *new_twigs = ref_ptr(qp, new_ref); +static dns_qpref_t +evacuate(dns_qp_t *qp, dns_qpnode_t *n) { + dns_qpweight_t size = branch_twigs_size(n); + dns_qpref_t old_ref = branch_twigs_ref(n); + dns_qpref_t new_ref = alloc_twigs(qp, size); + dns_qpnode_t *old_twigs = ref_ptr(qp, old_ref); + dns_qpnode_t *new_twigs = ref_ptr(qp, new_ref); move_twigs(new_twigs, old_twigs, size); if (!free_twigs(qp, old_ref, size)) { @@ -861,7 +863,7 @@ evacuate(dns_qp_t *qp, qp_node_t *n) { * copied to a mutable chunk. */ -static inline qp_node_t * +static inline dns_qpnode_t * make_root_mutable(dns_qp_t *qp) { if (cells_immutable(qp, qp->root_ref)) { qp->root_ref = evacuate(qp, MOVABLE_ROOT(qp)); @@ -870,7 +872,7 @@ make_root_mutable(dns_qp_t *qp) { } static inline void -make_twigs_mutable(dns_qp_t *qp, qp_node_t *n) { +make_twigs_mutable(dns_qp_t *qp, dns_qpnode_t *n) { if (cells_immutable(qp, branch_twigs_ref(n))) { *n = make_node(branch_index(n), evacuate(qp, n)); } @@ -891,11 +893,11 @@ make_twigs_mutable(dns_qp_t *qp, qp_node_t *n) { * algorithm introduces ref changes, that then bubble up towards the * root through the logic inside the loop. */ -static qp_ref_t -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); +static dns_qpref_t +compact_recursive(dns_qp_t *qp, dns_qpnode_t *parent) { + dns_qpweight_t size = branch_twigs_size(parent); + dns_qpref_t twigs_ref = branch_twigs_ref(parent); + dns_qpchunk_t chunk = ref_chunk(twigs_ref); if (qp->compact_all || (chunk != qp->bump && chunk_usage(qp, chunk) < QP_MIN_USED)) @@ -903,13 +905,13 @@ compact_recursive(dns_qp_t *qp, qp_node_t *parent) { twigs_ref = evacuate(qp, parent); } bool immutable = cells_immutable(qp, twigs_ref); - for (qp_weight_t pos = 0; pos < size; pos++) { - qp_node_t *child = ref_ptr(qp, twigs_ref) + pos; + for (dns_qpweight_t pos = 0; pos < size; pos++) { + dns_qpnode_t *child = ref_ptr(qp, twigs_ref) + pos; if (!is_branch(child)) { continue; } - qp_ref_t old_grandtwigs = branch_twigs_ref(child); - qp_ref_t new_grandtwigs = compact_recursive(qp, child); + dns_qpref_t old_grandtwigs = branch_twigs_ref(child); + dns_qpref_t new_grandtwigs = compact_recursive(qp, child); if (old_grandtwigs == new_grandtwigs) { continue; } @@ -985,7 +987,7 @@ dns_qp_compact(dns_qp_t *qp, dns_qpgc_t mode) { * write transactions, we can check qp->transaction_mode here. */ static inline bool -squash_twigs(dns_qp_t *qp, qp_ref_t twigs, qp_weight_t size) { +squash_twigs(dns_qp_t *qp, dns_qpref_t twigs, dns_qpweight_t size) { bool destroyed = free_twigs(qp, twigs, size); if (destroyed && QP_AUTOGC(qp)) { compact(qp); @@ -1025,12 +1027,12 @@ dns_qp_memusage(dns_qp_t *qp) { .used = qp->used_count, .hold = qp->hold_count, .free = qp->free_count, - .node_size = sizeof(qp_node_t), + .node_size = sizeof(dns_qpnode_t), .chunk_size = QP_CHUNK_SIZE, .fragmented = QP_NEEDGC(qp), }; - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (qp->base->ptr[chunk] != NULL) { memusage.chunk_count += 1; } @@ -1059,7 +1061,8 @@ dns_qpmulti_memusage(dns_qpmulti_t *multi) { if (qp->transaction_mode == QP_UPDATE) { memusage.bytes -= QP_CHUNK_BYTES; - memusage.bytes += qp->usage[qp->bump].used * sizeof(qp_node_t); + memusage.bytes += qp->usage[qp->bump].used * + sizeof(dns_qpnode_t); } UNLOCK(&multi->mutex); @@ -1099,7 +1102,7 @@ transaction_open(dns_qpmulti_t *multi, dns_qp_t **qptp) { * first part continues to be treated as immutable. (And the * rest of the chunk too, but that's OK.) */ - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (qp->usage[chunk].exists) { qp->usage[chunk].immutable = true; write_protect(qp, chunk); @@ -1222,13 +1225,13 @@ dns_qpmulti_commit(dns_qpmulti_t *multi, dns_qp_t **qptp) { multi->reader_ref = alloc_twigs(qp, READER_SIZE); qp->base->ptr[qp->bump] = chunk_shrink_raw( qp, qp->base->ptr[qp->bump], - qp->usage[qp->bump].used * sizeof(qp_node_t)); + qp->usage[qp->bump].used * sizeof(dns_qpnode_t)); } else { multi->reader_ref = alloc_twigs(qp, READER_SIZE); } /* anchor a new version of the trie */ - qp_node_t *reader = ref_ptr(qp, multi->reader_ref); + dns_qpnode_t *reader = ref_ptr(qp, multi->reader_ref); make_reader(reader, multi); /* paired with chunk_free() */ isc_refcount_increment(&qp->base->refcount); @@ -1263,7 +1266,7 @@ dns_qpmulti_rollback(dns_qpmulti_t *multi, dns_qp_t **qptp) { isc_nanosecs_t start = isc_time_monotonic(); - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (qp->base->ptr[chunk] != NULL && !qp->usage[chunk].immutable) { chunk_free(qp, chunk); @@ -1312,7 +1315,7 @@ dns_qpmulti_rollback(dns_qpmulti_t *multi, dns_qp_t **qptp) { static dns_qpmulti_t * reader_open(dns_qpmulti_t *multi, dns_qpreadable_t qpr) { dns_qpreader_t *qp = dns_qpreader(qpr); - qp_node_t *reader = rcu_dereference(multi->reader); + dns_qpnode_t *reader = rcu_dereference(multi->reader); if (reader == NULL) { QP_INIT(qp, multi->writer.methods, multi->writer.uctx); } else { @@ -1373,7 +1376,7 @@ dns_qpmulti_snapshot(dns_qpmulti_t *multi, dns_qpsnap_t **qpsp) { * reclaim unused memory in dns_qpsnap_destroy() */ qps->chunk_max = qpw->chunk_max; - for (qp_chunk_t chunk = 0; chunk < qpw->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qpw->chunk_max; chunk++) { if (qpw->usage[chunk].exists && chunk_usage(qpw, chunk) > 0) { qpw->usage[chunk].snapshot = true; qps->base->ptr[chunk] = qpw->base->ptr[chunk]; @@ -1462,7 +1465,7 @@ destroy_guts(dns_qp_t *qp) { if (qp->chunk_max == 0) { return; } - for (qp_chunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { + for (dns_qpchunk_t chunk = 0; chunk < qp->chunk_max; chunk++) { if (qp->base->ptr[chunk] != NULL) { chunk_free(qp, chunk); } @@ -1550,18 +1553,18 @@ dns_qpmulti_destroy(dns_qpmulti_t **qpmp) { 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 = NULL, *old_twigs = NULL; - qp_shift_t new_bit, old_bit; - qp_weight_t old_size, new_size; + dns_qpref_t new_ref, old_ref; + dns_qpnode_t new_leaf, old_node; + dns_qpnode_t *new_twigs = NULL, *old_twigs = NULL; + dns_qpshift_t new_bit, old_bit; + dns_qpweight_t old_size, new_size; dns_qpkey_t new_key, old_key; size_t new_keylen, old_keylen; size_t offset; uint64_t index; - qp_shift_t bit; - qp_weight_t pos; - qp_node_t *n = NULL; + dns_qpshift_t bit; + dns_qpweight_t pos; + dns_qpnode_t *n = NULL; REQUIRE(QP_VALID(qp)); @@ -1590,7 +1593,7 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival) { n = ref_ptr(qp, qp->root_ref); while (is_branch(n)) { prefetch_twigs(qp, n); - qp_ref_t ref = branch_twigs_ref(n); + dns_qpref_t ref = branch_twigs_ref(n); bit = branch_keybit(n, new_key, new_keylen); pos = branch_has_twig(n, bit) ? branch_twig_pos(n, bit) : 0; n = ref_ptr(qp, ref + pos); @@ -1686,9 +1689,9 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, return (ISC_R_NOTFOUND); } - qp_shift_t bit = 0; /* suppress warning */ - qp_node_t *parent = NULL; - qp_node_t *n = make_root_mutable(qp); + dns_qpshift_t bit = 0; /* suppress warning */ + dns_qpnode_t *parent = NULL; + dns_qpnode_t *n = make_root_mutable(qp); while (is_branch(n)) { prefetch_twigs(qp, n); bit = branch_keybit(n, search_key, search_keylen); @@ -1727,10 +1730,10 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, parent = NULL; INSIST(bit != 0); - 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); + dns_qpweight_t size = branch_twigs_size(n); + dns_qpweight_t pos = branch_twig_pos(n, bit); + dns_qpref_t ref = branch_twigs_ref(n); + dns_qpnode_t *twigs = ref_ptr(qp, ref); if (size == 2) { /* @@ -1763,7 +1766,7 @@ dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name, void **pval_r, * chains */ static void -maybe_set_name(dns_qpreader_t *qp, qp_node_t *node, dns_name_t *name) { +maybe_set_name(dns_qpreader_t *qp, dns_qpnode_t *node, dns_name_t *name) { dns_qpkey_t key; size_t len; @@ -1798,7 +1801,7 @@ dns_qpchain_length(dns_qpchain_t *chain) { void dns_qpchain_node(dns_qpchain_t *chain, unsigned int level, dns_name_t *name, void **pval_r, uint32_t *ival_r) { - qp_node_t *node = NULL; + dns_qpnode_t *node = NULL; REQUIRE(QPCHAIN_VALID(chain)); REQUIRE(level < chain->len); @@ -1830,10 +1833,10 @@ dns_qpiter_init(dns_qpreadable_t qpr, dns_qpiter_t *qpi) { */ static bool last_twig(dns_qpiter_t *qpi, bool forward) { - qp_weight_t pos = 0, max = 0; + dns_qpweight_t pos = 0, max = 0; if (qpi->sp > 0) { - qp_node_t *child = qpi->stack[qpi->sp]; - qp_node_t *parent = qpi->stack[qpi->sp - 1]; + dns_qpnode_t *child = qpi->stack[qpi->sp]; + dns_qpnode_t *parent = qpi->stack[qpi->sp - 1]; pos = child - ref_ptr(qpi->qp, branch_twigs_ref(parent)); if (forward) { max = branch_twigs_size(parent) - 1; @@ -1850,7 +1853,7 @@ last_twig(dns_qpiter_t *qpi, bool forward) { static isc_result_t iterate(bool forward, dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, uint32_t *ival_r) { - qp_node_t *node = NULL; + dns_qpnode_t *node = NULL; bool initial_branch = true; REQUIRE(QPITER_VALID(qpi)); @@ -1903,24 +1906,20 @@ iterate(bool forward, dns_qpiter_t *qpi, dns_name_t *name, void **pval_r, * there are more twigs in the current branch, * so step the node pointer forward (or back). */ + qpi->stack[qpi->sp] += (forward ? 1 : -1); node = qpi->stack[qpi->sp]; - node += (forward ? 1 : -1); - qpi->stack[qpi->sp] = node; } /* - * if we're at a branch now, push its first (or last) twig - * onto the stack and loop again. + * if we're at a branch now, we loop down to the + * left- or rightmost leaf. */ if (is_branch(node)) { qpi->sp++; INSIST(qpi->sp < DNS_QP_MAXKEY); - - qp_node_t *twigs = ref_ptr(qp, branch_twigs_ref(node)); - if (!forward) { - twigs += branch_twigs_size(node) - 1; - } - node = qpi->stack[qpi->sp] = twigs; + node = ref_ptr(qp, branch_twigs_ref(node)) + + (forward ? 0 : branch_twigs_size(node) - 1); + qpi->stack[qpi->sp] = node; initial_branch = false; } } while (is_branch(node)); @@ -1955,8 +1954,8 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, dns_qpreader_t *qp = dns_qpreader(qpr); dns_qpkey_t found_key; size_t found_keylen; - qp_shift_t bit; - qp_node_t *n = NULL; + dns_qpshift_t bit; + dns_qpnode_t *n = NULL; REQUIRE(QP_VALID(qp)); REQUIRE(search_keylen < sizeof(dns_qpkey_t)); @@ -1996,14 +1995,7 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, } static inline void -add_link(dns_qpchain_t *chain, qp_node_t *node, size_t offset) { - /* - * prevent duplication, which could occur if we're adding a link - * for the predecessor node and it's the same as the parent. - */ - if (chain->len > 0 && node == chain->chain[chain->len - 1].node) { - return; - } +add_link(dns_qpchain_t *chain, dns_qpnode_t *node, size_t offset) { chain->chain[chain->len].node = node; chain->chain[chain->len].offset = offset; chain->len++; @@ -2028,7 +2020,7 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, dns_qpkey_t search, found; size_t searchlen, foundlen; size_t offset; - qp_node_t *n = NULL; + dns_qpnode_t *n = NULL; dns_qpchain_t oc; dns_qpiter_t it; bool matched = true; @@ -2061,8 +2053,8 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, prefetch_twigs(qp, n); offset = branch_key_offset(n); - qp_shift_t bit = qpkey_bit(search, searchlen, offset); - qp_node_t *twigs = branch_twigs(qp, n); + dns_qpshift_t bit = qpkey_bit(search, searchlen, offset); + dns_qpnode_t *twigs = branch_twigs(qp, n); /* * A shorter key that can be a parent domain always has a @@ -2099,7 +2091,7 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, * the predecessor to the name we were searching * for, so let's go find that. */ - qp_weight_t pos = branch_twig_pos(n, bit); + dns_qpweight_t pos = branch_twig_pos(n, bit); if (pos == 0) { /* * this entire branch is greater than diff --git a/lib/dns/qp_p.h b/lib/dns/qp_p.h index c012492bc6..a6cf41f996 100644 --- a/lib/dns/qp_p.h +++ b/lib/dns/qp_p.h @@ -50,7 +50,7 @@ * most sigificant 15 bits are an offset value into the key, and * the 47 bits in the middle are a bitmap; see the documentation * for the SHIFT_* enum below. - * - The 32-bit word is a reference (qp_ref_t) to the packed sparse + * - The 32-bit word is a reference (dns_qpref_t) to the packed sparse * vector of "twigs", i.e. child nodes. A branch node has at least * two and at most 47 twigs. (The qp-trie update functions ensure that * branches actually branch, i.e. a branch cannot have only one child.) @@ -59,13 +59,13 @@ * A pair of reader nodes together contain a packed `dns_qpreader_t`. * See the section on "packed reader nodes" for details. */ -typedef struct qp_node { +struct dns_qpnode { #if WORDS_BIGENDIAN uint32_t bighi, biglo, small; #else uint32_t biglo, bighi, small; #endif -} qp_node_t; +}; /* * The possible values of the node type tag. Type tags must fit in two bits @@ -102,7 +102,7 @@ STATIC_ASSERT(sizeof(void *) <= sizeof(uint64_t), * The byte offset into the key is at the top of the word, so that it * can be extracted with just a shift, with no masking needed. * - * The names are SHIFT_thing because they are qp_shift_t values. (See + * The names are SHIFT_thing because they are dns_qpshift_t values. (See * below for the various `qp_*` type declarations.) * * These values are relatively fixed in practice: SHIFT_NOBYTE needs @@ -122,7 +122,7 @@ enum { */ /* - * A "cell" is a location that can contain a `qp_node_t`, and a "chunk" + * A "cell" is a location that can contain a `dns_qpnode_t`, and a "chunk" * is a moderately large array of cells. A big trie can occupy * multiple chunks. (Unlike other nodes, a trie's root node lives in * its `struct dns_qp` instead of being allocated in a cell.) @@ -148,7 +148,7 @@ STATIC_ASSERT(6 <= QP_CHUNK_LOG && QP_CHUNK_LOG <= 20, "qp-trie chunk size is unreasonable"); #define QP_CHUNK_SIZE (1U << QP_CHUNK_LOG) -#define QP_CHUNK_BYTES (QP_CHUNK_SIZE * sizeof(qp_node_t)) +#define QP_CHUNK_BYTES (QP_CHUNK_SIZE * sizeof(dns_qpnode_t)) /* * We need a bitfield this big to count how much of a chunk is in use: @@ -189,71 +189,23 @@ STATIC_ASSERT(6 <= QP_CHUNK_LOG && QP_CHUNK_LOG <= 20, */ #define GROWTH_FACTOR(size) ((size) + (size) / 2 + 2) -/*********************************************************************** - * - * helper types - */ - /* - * C is not strict enough with its integer types for these typedefs to - * improve type safety, but it helps to have annotations saying what - * particular kind of number we are dealing with. + * Constructors and accessors for dns_qpref_t values, defined here to show + * how the dns_qpref_t, dns_qpchunk_t, dns_qpcell_t types relate to each other */ -/* - * The number or position of a bit inside a word. (0..63) - * - * Note: A dns_qpkey_t is logically an array of qp_shift_t values, but it - * isn't declared that way because dns_qpkey_t is a public type whereas - * qp_shift_t is private. - * - * A dns_qpkey element key[off] must satisfy - * - * SHIFT_NOBYTE <= key[off] && key[off] < SHIFT_OFFSET - */ -typedef uint8_t qp_shift_t; - -/* - * The number of bits set in a word (as in Hamming weight or popcount) - * which is used for the position of a node in the packed sparse - * vector of twigs. (0..47) because our bitmap does not fill the word. - */ -typedef uint8_t qp_weight_t; - -/* - * A chunk number, i.e. an index into the chunk arrays. - */ -typedef uint32_t qp_chunk_t; - -/* - * Cell offset within a chunk, or a count of cells. Each cell in a - * chunk can contain a node. - */ -typedef uint32_t qp_cell_t; - -/* - * A twig reference is used to refer to a twigs vector, which occupies a - * contiguous group of cells. - */ -typedef uint32_t qp_ref_t; - -/* - * Constructors and accessors for qp_ref_t values, defined here to show - * how the qp_ref_t, qp_chunk_t, qp_cell_t types relate to each other - */ - -static inline qp_ref_t -make_ref(qp_chunk_t chunk, qp_cell_t cell) { +static inline dns_qpref_t +make_ref(dns_qpchunk_t chunk, dns_qpcell_t cell) { return (QP_CHUNK_SIZE * chunk + cell); } -static inline qp_chunk_t -ref_chunk(qp_ref_t ref) { +static inline dns_qpchunk_t +ref_chunk(dns_qpref_t ref) { return (ref / QP_CHUNK_SIZE); } -static inline qp_cell_t -ref_cell(qp_ref_t ref) { +static inline dns_qpcell_t +ref_cell(dns_qpref_t ref) { return (ref % QP_CHUNK_SIZE); } @@ -262,7 +214,7 @@ ref_cell(qp_ref_t ref) { * to a value that should trigger an obvious bug. See qp_init() * and get_root() below. */ -#define INVALID_REF ((qp_ref_t)~0UL) +#define INVALID_REF ((dns_qpref_t)~0UL) /*********************************************************************** * @@ -313,9 +265,9 @@ ref_cell(qp_ref_t ref) { */ typedef struct qp_usage { /*% the allocation point, increases monotonically */ - qp_cell_t used : QP_USAGE_BITS; + dns_qpcell_t used : QP_USAGE_BITS; /*% count of nodes no longer needed, also monotonic */ - qp_cell_t free : QP_USAGE_BITS; + dns_qpcell_t free : QP_USAGE_BITS; /*% qp->base->ptr[chunk] != NULL */ bool exists : 1; /*% is this chunk shared? [MT] */ @@ -355,7 +307,7 @@ typedef struct qp_usage { struct dns_qpbase { unsigned int magic; isc_refcount_t refcount; - qp_node_t *ptr[]; + dns_qpnode_t *ptr[]; }; /* @@ -368,8 +320,8 @@ typedef struct qp_rcuctx { struct rcu_head rcu_head; isc_mem_t *mctx; dns_qpmulti_t *multi; - qp_chunk_t count; - qp_chunk_t chunk[]; + dns_qpchunk_t count; + dns_qpchunk_t chunk[]; } qp_rcuctx_t; /* @@ -386,8 +338,8 @@ qpbase_unref(dns_qpreadable_t qpr) { * Now we know about `dns_qpreader_t` and `dns_qpbase_t`, * here's how we convert a twig reference into a pointer. */ -static inline qp_node_t * -ref_ptr(dns_qpreadable_t qpr, qp_ref_t ref) { +static inline dns_qpnode_t * +ref_ptr(dns_qpreadable_t qpr, dns_qpref_t ref) { dns_qpreader_t *qp = dns_qpreader(qpr); return (qp->base->ptr[ref_chunk(ref)] + ref_cell(ref)); } @@ -415,7 +367,7 @@ ref_ptr(dns_qpreadable_t qpr, qp_ref_t ref) { /* * Polymorphic initialization of the `dns_qpreader_t` prefix. * - * The location of the root node is actually a qp_ref_t, but is + * The location of the root node is actually a dns_qpref_t, but is * declared in DNS_QPREADER_FIELDS as uint32_t to avoid leaking too * many internal details into the public API. * @@ -514,17 +466,17 @@ struct dns_qp { /*% array of per-chunk allocation counters */ qp_usage_t *usage; /*% number of slots in `chunk` and `usage` arrays */ - qp_chunk_t chunk_max; + dns_qpchunk_t chunk_max; /*% which chunk is used for allocations */ - qp_chunk_t bump; + dns_qpchunk_t bump; /*% nodes in the `bump` chunk below `fender` are read only [MT] */ - qp_cell_t fender; + dns_qpcell_t fender; /*% number of leaf nodes */ - qp_cell_t leaf_count; + dns_qpcell_t leaf_count; /*% total of all usage[] counters */ - qp_cell_t used_count, free_count; + dns_qpcell_t used_count, free_count; /*% free cells that cannot be recovered right now */ - qp_cell_t hold_count; + dns_qpcell_t hold_count; /*% what kind of transaction was most recently started [MT] */ enum { QP_NONE, QP_WRITE, QP_UPDATE } transaction_mode : 2; /*% compact the entire trie [MT] */ @@ -547,7 +499,7 @@ struct dns_qp { * There is a `reader_ref` which corresponds to the `reader` pointer * (`ref_ptr(multi->reader_ref) == multi->reader`). The `reader_ref` is * necessary when freeing the space used by the reader, because there - * isn't a good way to recover a qp_ref_t from a qp_node_t pointer. + * isn't a good way to recover a dns_qpref_t from a dns_qpnode_t pointer. * * There is a per-trie list of snapshots that is used for reclaiming * memory when a snapshot is destroyed. @@ -558,11 +510,11 @@ struct dns_qp { struct dns_qpmulti { uint32_t magic; /*% RCU-protected pointer to current packed reader */ - qp_node_t *reader; + dns_qpnode_t *reader; /*% the mutex protects the rest of this structure */ isc_mutex_t mutex; /*% ref_ptr(writer, reader_ref) == reader */ - qp_ref_t reader_ref; + dns_qpref_t reader_ref; /*% the main working structure */ dns_qp_t writer; /*% saved allocator state to support rollback */ @@ -589,7 +541,7 @@ struct dns_qpmulti { * Get the 64-bit word of a node. */ static inline uint64_t -node64(qp_node_t *n) { +node64(dns_qpnode_t *n) { uint64_t lo = n->biglo; uint64_t hi = n->bighi; return (lo | (hi << 32)); @@ -599,16 +551,16 @@ node64(qp_node_t *n) { * Get the 32-bit word of a node. */ static inline uint32_t -node32(qp_node_t *n) { +node32(dns_qpnode_t *n) { return (n->small); } /* * Create a node from its parts */ -static inline qp_node_t +static inline dns_qpnode_t make_node(uint64_t big, uint32_t small) { - return ((qp_node_t){ + return ((dns_qpnode_t){ .biglo = (uint32_t)(big), .bighi = (uint32_t)(big >> 32), .small = small, @@ -620,7 +572,7 @@ make_node(uint64_t big, uint32_t small) { * a warning about mismatched pointer/integer sizes on 32 bit systems. */ static inline void * -node_pointer(qp_node_t *n) { +node_pointer(dns_qpnode_t *n) { return ((void *)(uintptr_t)(node64(n) & ~TAG_MASK)); } @@ -628,7 +580,7 @@ node_pointer(qp_node_t *n) { * Examine a node's tag bits */ static inline uint32_t -node_tag(qp_node_t *n) { +node_tag(dns_qpnode_t *n) { return (n->biglo & TAG_MASK); } @@ -636,7 +588,7 @@ node_tag(qp_node_t *n) { * simplified for the hot path */ static inline bool -is_branch(qp_node_t *n) { +is_branch(dns_qpnode_t *n) { return (n->biglo & BRANCH_TAG); } @@ -646,7 +598,7 @@ is_branch(qp_node_t *n) { * Get a leaf's pointer value. */ static inline void * -leaf_pval(qp_node_t *n) { +leaf_pval(dns_qpnode_t *n) { return (node_pointer(n)); } @@ -654,16 +606,16 @@ leaf_pval(qp_node_t *n) { * Get a leaf's integer value */ static inline uint32_t -leaf_ival(qp_node_t *n) { +leaf_ival(dns_qpnode_t *n) { return (node32(n)); } /* * Create a leaf node from its parts */ -static inline qp_node_t +static inline dns_qpnode_t make_leaf(const void *pval, uint32_t ival) { - qp_node_t leaf = make_node((uintptr_t)pval, ival); + dns_qpnode_t leaf = make_node((uintptr_t)pval, ival); REQUIRE(node_tag(&leaf) == LEAF_TAG); return (leaf); } @@ -680,15 +632,15 @@ make_leaf(const void *pval, uint32_t ival) { * Get a branch node's index word */ static inline uint64_t -branch_index(qp_node_t *n) { +branch_index(dns_qpnode_t *n) { return (node64(n)); } /* * Get a reference to a branch node's child twigs. */ -static inline qp_ref_t -branch_twigs_ref(qp_node_t *n) { +static inline dns_qpref_t +branch_twigs_ref(dns_qpnode_t *n) { return (node32(n)); } @@ -696,7 +648,7 @@ branch_twigs_ref(qp_node_t *n) { * Bit positions in the bitmap come directly from the key. DNS names are * converted to keys using the tables declared at the end of this file. */ -static inline qp_shift_t +static inline dns_qpshift_t qpkey_bit(const dns_qpkey_t key, size_t len, size_t offset) { if (offset < len) { return (key[offset]); @@ -709,15 +661,15 @@ qpkey_bit(const dns_qpkey_t key, size_t len, size_t offset) { * Extract a branch node's offset field, used to index the key. */ static inline size_t -branch_key_offset(qp_node_t *n) { +branch_key_offset(dns_qpnode_t *n) { return ((size_t)(branch_index(n) >> SHIFT_OFFSET)); } /* * Which bit identifies the twig of this node for this key? */ -static inline qp_shift_t -branch_keybit(qp_node_t *n, const dns_qpkey_t key, size_t len) { +static inline dns_qpshift_t +branch_keybit(dns_qpnode_t *n, const dns_qpkey_t key, size_t len) { return (qpkey_bit(key, len, branch_key_offset(n))); } @@ -725,8 +677,8 @@ branch_keybit(qp_node_t *n, const dns_qpkey_t key, size_t len) { * Get a pointer to a the first twig of a branch (this also functions * as a pointer to the entire twig vector). */ -static inline qp_node_t * -branch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { +static inline dns_qpnode_t * +branch_twigs(dns_qpreadable_t qpr, dns_qpnode_t *n) { return (ref_ptr(qpr, branch_twigs_ref(n))); } @@ -734,7 +686,7 @@ branch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { * Warm up the cache while calculating which twig we want. */ static inline void -prefetch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { +prefetch_twigs(dns_qpreadable_t qpr, dns_qpnode_t *n) { __builtin_prefetch(ref_ptr(qpr, branch_twigs_ref(n))); } @@ -743,7 +695,7 @@ prefetch_twigs(dns_qpreadable_t qpr, qp_node_t *n) { /* * Get a pointer to the root node, checking if the trie is empty. */ -static inline qp_node_t * +static inline dns_qpnode_t * get_root(dns_qpreadable_t qpr) { dns_qpreader_t *qp = dns_qpreader(qpr); if (qp->root_ref == INVALID_REF) { @@ -760,7 +712,7 @@ get_root(dns_qpreadable_t qpr) { * just enough to treat the root node as a vector of one twig. */ #define MOVABLE_ROOT(qp) \ - (&(qp_node_t){ \ + (&(dns_qpnode_t){ \ .biglo = BRANCH_TAG | (1 << SHIFT_NOBYTE), \ .small = qp->root_ref, \ }) @@ -778,11 +730,11 @@ get_root(dns_qpreadable_t qpr) { * we subtract 1 to set all lesser bits, and subtract the tag mask * because the type tag is not part of the bitmap. */ -static inline qp_weight_t -branch_count_bitmap_before(qp_node_t *n, qp_shift_t bit) { +static inline dns_qpweight_t +branch_count_bitmap_before(dns_qpnode_t *n, dns_qpshift_t bit) { uint64_t mask = (1ULL << bit) - 1 - TAG_MASK; uint64_t bitmap = branch_index(n) & mask; - return ((qp_weight_t)__builtin_popcountll(bitmap)); + return ((dns_qpweight_t)__builtin_popcountll(bitmap)); } /* @@ -791,24 +743,24 @@ branch_count_bitmap_before(qp_node_t *n, qp_shift_t bit) { * The offset is directly after the bitmap so the offset's lesser bits * covers the whole bitmap, and the bitmap's weight is the number of twigs. */ -static inline qp_weight_t -branch_twigs_size(qp_node_t *n) { +static inline dns_qpweight_t +branch_twigs_size(dns_qpnode_t *n) { return (branch_count_bitmap_before(n, SHIFT_OFFSET)); } /* * Position of a twig within the packed sparse vector. */ -static inline qp_weight_t -branch_twig_pos(qp_node_t *n, qp_shift_t bit) { +static inline dns_qpweight_t +branch_twig_pos(dns_qpnode_t *n, dns_qpshift_t bit) { return (branch_count_bitmap_before(n, bit)); } /* * Get a pointer to the twig for a given bit number. */ -static inline qp_node_t * -branch_twig_ptr(dns_qpreadable_t qpr, qp_node_t *n, qp_shift_t bit) { +static inline dns_qpnode_t * +branch_twig_ptr(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpshift_t bit) { return (ref_ptr(qpr, branch_twigs_ref(n) + branch_twig_pos(n, bit))); } @@ -816,20 +768,20 @@ branch_twig_ptr(dns_qpreadable_t qpr, qp_node_t *n, qp_shift_t bit) { * Is the twig identified by this bit present? */ static inline bool -branch_has_twig(qp_node_t *n, qp_shift_t bit) { +branch_has_twig(dns_qpnode_t *n, dns_qpshift_t bit) { return (branch_index(n) & (1ULL << bit)); } /* twig logistics *****************************************************/ static inline void -move_twigs(qp_node_t *to, qp_node_t *from, qp_weight_t size) { - memmove(to, from, size * sizeof(qp_node_t)); +move_twigs(dns_qpnode_t *to, dns_qpnode_t *from, dns_qpweight_t size) { + memmove(to, from, size * sizeof(dns_qpnode_t)); } static inline void -zero_twigs(qp_node_t *twigs, qp_weight_t size) { - memset(twigs, 0, size * sizeof(qp_node_t)); +zero_twigs(dns_qpnode_t *twigs, dns_qpweight_t size) { + memset(twigs, 0, size * sizeof(dns_qpnode_t)); } /*********************************************************************** @@ -873,14 +825,14 @@ zero_twigs(qp_node_t *twigs, qp_weight_t size) { * allocated using `alloc_twigs(&multi->writer, READER_SIZE)`. */ static inline void -make_reader(qp_node_t *reader, dns_qpmulti_t *multi) { +make_reader(dns_qpnode_t *reader, dns_qpmulti_t *multi) { dns_qp_t *qp = &multi->writer; reader[0] = make_node(READER_TAG | (uintptr_t)multi, QPREADER_MAGIC); reader[1] = make_node(READER_TAG | (uintptr_t)qp->base, qp->root_ref); } static inline bool -reader_valid(qp_node_t *reader) { +reader_valid(dns_qpnode_t *reader) { return (reader != NULL && // node_tag(&reader[0]) == READER_TAG && node_tag(&reader[1]) == READER_TAG && @@ -892,7 +844,7 @@ reader_valid(qp_node_t *reader) { * consistency checks. */ static inline dns_qpmulti_t * -unpack_reader(dns_qpreader_t *qp, qp_node_t *reader) { +unpack_reader(dns_qpreader_t *qp, dns_qpnode_t *reader) { INSIST(reader_valid(reader)); dns_qpmulti_t *multi = node_pointer(&reader[0]); dns_qpbase_t *base = node_pointer(&reader[1]); @@ -914,19 +866,19 @@ unpack_reader(dns_qpreader_t *qp, qp_node_t *reader) { */ static inline void -attach_leaf(dns_qpreadable_t qpr, qp_node_t *n) { +attach_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n) { dns_qpreader_t *qp = dns_qpreader(qpr); qp->methods->attach(qp->uctx, leaf_pval(n), leaf_ival(n)); } static inline void -detach_leaf(dns_qpreadable_t qpr, qp_node_t *n) { +detach_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n) { dns_qpreader_t *qp = dns_qpreader(qpr); qp->methods->detach(qp->uctx, leaf_pval(n), leaf_ival(n)); } static inline size_t -leaf_qpkey(dns_qpreadable_t qpr, qp_node_t *n, dns_qpkey_t key) { +leaf_qpkey(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpkey_t key) { dns_qpreader_t *qp = dns_qpreader(qpr); size_t len = qp->methods->makekey(key, qp->uctx, leaf_pval(n), leaf_ival(n)); diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index f11d34385a..d6a1d3055f 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -37,7 +37,7 @@ */ uint8_t -qp_test_bittoascii(qp_shift_t bit) { +qp_test_bittoascii(dns_qpshift_t bit) { uint8_t byte = dns_qp_byte_for_bit[bit]; if (bit == SHIFT_NOBYTE) { return ('.'); @@ -67,14 +67,14 @@ qp_test_keytoascii(dns_qpkey_t key, size_t len) { */ static size_t -getheight(dns_qp_t *qp, qp_node_t *n) { +getheight(dns_qp_t *qp, dns_qpnode_t *n) { if (node_tag(n) == LEAF_TAG) { return (0); } size_t max_height = 0; - qp_node_t *twigs = branch_twigs(qp, n); - qp_weight_t size = branch_twigs_size(n); - for (qp_weight_t pos = 0; pos < size; pos++) { + dns_qpnode_t *twigs = branch_twigs(qp, n); + dns_qpweight_t size = branch_twigs_size(n); + for (dns_qpweight_t pos = 0; pos < size; pos++) { size_t height = getheight(qp, &twigs[pos]); max_height = ISC_MAX(max_height, height); } @@ -83,20 +83,20 @@ getheight(dns_qp_t *qp, qp_node_t *n) { size_t qp_test_getheight(dns_qp_t *qp) { - qp_node_t *root = get_root(qp); + dns_qpnode_t *root = get_root(qp); return (root == NULL ? 0 : getheight(qp, root)); } static size_t -maxkeylen(dns_qp_t *qp, qp_node_t *n) { +maxkeylen(dns_qp_t *qp, dns_qpnode_t *n) { if (node_tag(n) == LEAF_TAG) { dns_qpkey_t key; return (leaf_qpkey(qp, n, key)); } size_t max_len = 0; - qp_node_t *twigs = branch_twigs(qp, n); - qp_weight_t size = branch_twigs_size(n); - for (qp_weight_t pos = 0; pos < size; pos++) { + dns_qpnode_t *twigs = branch_twigs(qp, n); + dns_qpweight_t size = branch_twigs_size(n); + for (dns_qpweight_t pos = 0; pos < size; pos++) { size_t len = maxkeylen(qp, &twigs[pos]); max_len = ISC_MAX(max_len, len); } @@ -105,7 +105,7 @@ maxkeylen(dns_qp_t *qp, qp_node_t *n) { size_t qp_test_maxkeylen(dns_qp_t *qp) { - qp_node_t *root = get_root(qp); + dns_qpnode_t *root = get_root(qp); return (root == NULL ? 0 : maxkeylen(qp, root)); } @@ -158,7 +158,7 @@ qp_test_dumpqp(dns_qp_t *qp) { void qp_test_dumpmulti(dns_qpmulti_t *multi) { dns_qpreader_t qpr; - qp_node_t *reader = rcu_dereference(multi->reader); + dns_qpnode_t *reader = rcu_dereference(multi->reader); dns_qpmulti_t *whence = unpack_reader(&qpr, reader); dumpqp(&multi->writer, "qpmulti->writer"); printf("qpmulti->reader %p root_ref %u %u:%u base %p\n", reader, @@ -176,10 +176,10 @@ qp_test_dumpmulti(dns_qpmulti_t *multi) { void qp_test_dumpchunks(dns_qp_t *qp) { - qp_cell_t used = 0; - qp_cell_t free = 0; + dns_qpcell_t used = 0; + dns_qpcell_t free = 0; dumpqp(qp, "qp"); - for (qp_chunk_t c = 0; c < qp->chunk_max; c++) { + for (dns_qpchunk_t c = 0; c < qp->chunk_max; c++) { printf("qp %p chunk %u base %p " "used %u free %u immutable %u discounted %u\n", qp, c, qp->base->ptr[c], qp->usage[c].used, @@ -196,11 +196,11 @@ void qp_test_dumptrie(dns_qpreadable_t qpr) { dns_qpreader_t *qp = dns_qpreader(qpr); struct { - qp_ref_t ref; - qp_shift_t max, pos; + dns_qpref_t ref; + dns_qpshift_t max, pos; } stack[512]; size_t sp = 0; - qp_cell_t leaf_count = 0; + dns_qpcell_t leaf_count = 0; /* * fake up a sentinel stack entry corresponding to the root @@ -211,7 +211,7 @@ qp_test_dumptrie(dns_qpreadable_t qpr) { stack[sp].max = 0; stack[sp].pos = 0; - qp_node_t *n = get_root(qp); + dns_qpnode_t *n = get_root(qp); if (n == NULL) { printf("%p EMPTY\n", n); fflush(stdout); @@ -222,15 +222,15 @@ qp_test_dumptrie(dns_qpreadable_t qpr) { for (;;) { if (is_branch(n)) { - qp_ref_t ref = branch_twigs_ref(n); - qp_weight_t max = branch_twigs_size(n); - qp_node_t *twigs = ref_ptr(qp, ref); + dns_qpref_t ref = branch_twigs_ref(n); + dns_qpweight_t max = branch_twigs_size(n); + dns_qpnode_t *twigs = ref_ptr(qp, ref); /* brief list of twigs */ dns_qpkey_t bits; size_t len = 0; - for (qp_shift_t bit = SHIFT_NOBYTE; bit < SHIFT_OFFSET; - bit++) + for (dns_qpshift_t bit = SHIFT_NOBYTE; + bit < SHIFT_OFFSET; bit++) { if (branch_has_twig(n, bit)) { bits[len++] = bit; @@ -271,11 +271,11 @@ qp_test_dumptrie(dns_qpreadable_t qpr) { } static void -dumpdot_name(qp_node_t *n) { +dumpdot_name(dns_qpnode_t *n) { if (n == NULL) { printf("empty"); } else if (is_branch(n)) { - qp_ref_t ref = branch_twigs_ref(n); + dns_qpref_t ref = branch_twigs_ref(n); printf("c%dn%d", ref_chunk(ref), ref_cell(ref)); } else { printf("v%p", leaf_pval(n)); @@ -283,7 +283,7 @@ dumpdot_name(qp_node_t *n) { } static void -dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { +dumpdot_twig(dns_qp_t *qp, dns_qpnode_t *n) { if (n == NULL) { printf("empty [shape=oval, label=\"\\N EMPTY\"];\n"); } else if (is_branch(n)) { @@ -291,7 +291,9 @@ dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { printf(" [shape=record, label=\"{ \\N\\noff %zu | ", branch_key_offset(n)); char sep = '{'; - for (qp_shift_t bit = SHIFT_NOBYTE; bit < SHIFT_OFFSET; bit++) { + for (dns_qpshift_t bit = SHIFT_NOBYTE; bit < SHIFT_OFFSET; + bit++) + { if (branch_has_twig(n, bit)) { printf("%c %c ", sep, branch_twig_pos(n, bit), @@ -301,17 +303,17 @@ dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { } printf("}}\"];\n"); - qp_node_t *twigs = branch_twigs(qp, n); - qp_weight_t size = branch_twigs_size(n); + dns_qpnode_t *twigs = branch_twigs(qp, n); + dns_qpweight_t size = branch_twigs_size(n); - for (qp_weight_t pos = 0; pos < size; pos++) { + for (dns_qpweight_t pos = 0; pos < size; pos++) { dumpdot_name(n); printf(":t%d:e -> ", pos); dumpdot_name(&twigs[pos]); printf(":w;\n"); } - for (qp_weight_t pos = 0; pos < size; pos++) { + for (dns_qpweight_t pos = 0; pos < size; pos++) { dumpdot_twig(qp, &twigs[pos]); } @@ -327,7 +329,7 @@ dumpdot_twig(dns_qp_t *qp, qp_node_t *n) { void qp_test_dumpdot(dns_qp_t *qp) { REQUIRE(QP_VALID(qp)); - qp_node_t *n = get_root(qp); + dns_qpnode_t *n = get_root(qp); printf("strict digraph {\nrankdir = \"LR\"; ranksep = 1.0;\n"); printf("ROOT [shape=point]; ROOT -> "); dumpdot_name(n); From 03016902dd95c99c34a17039d899be562a5dd40e Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 26 Sep 2023 20:19:37 -0700 Subject: [PATCH 8/9] rename dns_qp_findname_ancestor() to dns_qp_lookup() I am weary of typing so long a name. (plus, the name has become slightly misleading now that the DNS_QPFIND_NOEXACT option no longer exists.) --- lib/dns/forward.c | 3 +-- lib/dns/include/dns/qp.h | 41 ++++++++++++++++++++++++---------------- lib/dns/keytable.c | 6 ++---- lib/dns/nametree.c | 4 ++-- lib/dns/nta.c | 3 +-- lib/dns/qp.c | 7 +++---- lib/dns/zt.c | 3 +-- tests/bench/qplookups.c | 9 ++++----- tests/dns/qp_test.c | 11 +++++------ 9 files changed, 44 insertions(+), 43 deletions(-) diff --git a/lib/dns/forward.c b/lib/dns/forward.c index 0be252f3ac..f48a3dc076 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,8 +168,7 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, - NULL); + result = dns_qp_lookup(&qpr, name, NULL, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index f38eee37ac..cb92d4f78d 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -235,10 +235,10 @@ typedef struct dns_qpiter { /*% * A QP chain holds references to each populated node between the root and - * a given leaf. It is used internally by `dns_qp_findname_ancestor()` to - * return a partial match if the specific name requested is not found; - * optionally it can be passed back to the caller so that individual nodes - * can be accessed. + * a given leaf. It is used internally by `dns_qp_lookup()` to return a + * partial match if the specific name requested is not found; optionally it + * can be passed back to the caller so that individual nodes can be + * accessed. */ typedef struct dns_qpchain { unsigned int magic; @@ -527,24 +527,33 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, */ isc_result_t -dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_name_t *foundname, dns_name_t *predecessor, - dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); +dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, + dns_name_t *foundname, dns_name_t *predecessor, + dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); /*%< - * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the - * given DNS name. + * Look up a leaf in a qp-trie that is equal to, or an ancestor domain of, + * 'name'. * - * If 'foundname' is not NULL, it is updated to contain the name found. - * - * If 'predecessor' is not NULL, it is updated to contain the DNSSEC - * predecessor of the searched-for name. + * If 'foundname' is not NULL, it will be updated to contain the name + * that was found (if any). The return code, ISC_R_SUCCESS or + * DNS_R_PARTIALMATCH, indicates whether the name found is name that + * was requested, or an ancestor. If the result is ISC_R_NOTFOUND, + * 'foundname' will not be updated. * * If 'chain' is not NULL, it is updated to contain a QP chain with * references to the populated nodes in the tree between the root and - * the name found. + * the name that was found. If the return code is DNS_R_PARTIALMATCH + * then the chain terminates at the closest ancestor found; if it is + * ISC_R_SUCCESS then it terminates at the name that was requested. + * If the result is ISC_R_NOTFOUND, 'chain' will not be updated. * - * The leaf values are assigned to whichever of `*pval_r` and `*ival_r` - * are not null, unless the return value is ISC_R_NOTFOUND. + * If 'predecessor' is not NULL, it will be updated to contain the + * closest predecessor of the searched-for name that exists in the + * trie. + * + * The leaf data for the node that was found will be assigned to + * whichever of `*pval_r` and `*ival_r` are not NULL, unless the + * return value is ISC_R_NOTFOUND. * * Requires: * \li `qpr` is a pointer to a readable qp-trie diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index e8ce21fa19..7284140951 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,8 +518,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, - NULL); + result = dns_qp_lookup(&qpr, name, NULL, NULL, NULL, &pval, NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -548,8 +547,7 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, - NULL); + result = dns_qp_lookup(&qpr, name, NULL, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 60fa9a97f0..f2b3232726 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,8 +289,8 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, found, NULL, NULL, - (void **)&node, NULL); + result = dns_qp_lookup(&qpr, name, found, NULL, NULL, (void **)&node, + NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { switch (nametree->type) { case DNS_NAMETREE_BOOL: diff --git a/lib/dns/nta.c b/lib/dns/nta.c index c86b0ebf7b..060fa6c32d 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,8 +413,7 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, NULL, &pval, - NULL); + result = dns_qp_lookup(&qpr, name, NULL, NULL, NULL, &pval, NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index c554c85d23..4bc75414c9 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2012,10 +2012,9 @@ prevleaf(dns_qpiter_t *it) { } isc_result_t -dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_name_t *foundname, dns_name_t *predecessor, - dns_qpchain_t *chain, void **pval_r, - uint32_t *ival_r) { +dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, + dns_name_t *foundname, dns_name_t *predecessor, + dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r) { dns_qpreader_t *qp = dns_qpreader(qpr); dns_qpkey_t search, found; size_t searchlen, foundlen; diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 147e6fc830..7f1b416023 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -178,8 +178,7 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, dns_qpmulti_query(zt->multi, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &chain, &pval, - NULL); + result = dns_qp_lookup(&qpr, name, NULL, NULL, &chain, &pval, NULL); if (exactopts == DNS_ZTFIND_EXACT && result == DNS_R_PARTIALMATCH) { result = ISC_R_NOTFOUND; } else if (exactopts == DNS_ZTFIND_NOEXACT && result == ISC_R_SUCCESS) { diff --git a/tests/bench/qplookups.c b/tests/bench/qplookups.c index fd50ad37ca..be85825ec3 100644 --- a/tests/bench/qplookups.c +++ b/tests/bench/qplookups.c @@ -250,12 +250,11 @@ main(int argc, char **argv) { start = isc_time_monotonic(); for (i = 0; i < n; i++) { name = dns_fixedname_name(&items[i]); - dns_qp_findname_ancestor(qp, name, 0, NULL, NULL, NULL, NULL); + dns_qp_lookup(qp, name, 0, NULL, NULL, NULL, NULL); } stop = isc_time_monotonic(); - snprintf(buf, sizeof(buf), - "look up %zd names (dns_qp_findname_ancestor):", n); + snprintf(buf, sizeof(buf), "look up %zd names (dns_qp_lookup):", n); printf("%-57s%7.3fsec\n", buf, (stop - start) / (double)NS_PER_SEC); start = isc_time_monotonic(); @@ -276,12 +275,12 @@ main(int argc, char **argv) { ++search->ndata[1]; } - dns_qp_findname_ancestor(qp, search, 0, NULL, NULL, NULL, NULL); + dns_qp_lookup(qp, search, 0, NULL, NULL, NULL, NULL); } stop = isc_time_monotonic(); snprintf(buf, sizeof(buf), - "look up %zd wrong names (dns_qp_findname_ancestor):", n); + "look up %zd wrong names (dns_qp_lookup):", n); printf("%-57s%7.3fsec\n", buf, (stop - start) / (double)NS_PER_SEC); isc_mem_cput(mctx, items, n, sizeof(dns_fixedname_t)); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 6c83f8f365..c0c399085f 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -349,8 +349,8 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { void *pval = NULL; dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, foundname, NULL, - NULL, &pval, NULL); + result = dns_qp_lookup(qp, name, foundname, NULL, NULL, &pval, + NULL); #if 0 fprintf(stderr, "%s %s (expected %s) " @@ -496,8 +496,8 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { dns_qpchain_init(qp, &chain); dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, NULL, NULL, &chain, - NULL, NULL); + result = dns_qp_lookup(qp, name, NULL, NULL, &chain, NULL, + NULL); #if 0 fprintf(stderr, "%s %s (expected %s), " @@ -574,8 +574,7 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { char *predname = NULL; dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, NULL, pred, NULL, - NULL, NULL); + result = dns_qp_lookup(qp, name, NULL, pred, NULL, NULL, NULL); #if 0 fprintf(stderr, "%s: expected %s got %s\n", check[i].query, isc_result_totext(check[i].result), From 9736497d8a7c052df7a49919dcdb66b0a489ae50 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 26 Sep 2023 20:23:13 -0700 Subject: [PATCH 9/9] CHANGES for [GL !8338] --- CHANGES | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES b/CHANGES index b6a66d088c..75dea389ee 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +6260. [func] Added opptions to the QP trie that will be needed + when it is used as a zone or cache database: backward + iteration, and retrieval of DNSSEC predecessor + nodes and node chains. [GL !8338] + 6259. [placeholder] 6258. [func] Use explictly created external memory pools for