From 3bf23fadb0cd9c39319d6db5d12b2a1ab1c27f44 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Tue, 19 Sep 2023 00:41:57 -0700 Subject: [PATCH] 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);