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

@@ -191,10 +191,7 @@ typedef struct dns_qpiter {
 	unsigned int	magic;
 	dns_qpreader_t *qp;
 	uint16_t	sp;
-	struct __attribute__((__packed__)) {
+	void	       *stack[DNS_QP_MAXKEY];
-		uint32_t ref;
-		uint8_t	 more;
-	} 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
  *	}
  *

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);
 	}

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);

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 = (dns_qpiter_t){
-	qpi->qp = qp;
+		.qp = qp,
-	qpi->sp = 0;
+		.magic = QPITER_MAGIC,
-	qpi->stack[qpi->sp].ref = qp->root_ref;
+	};
-	qpi->stack[qpi->sp].more = 0;
 }
 
 /*
+ * 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
+static isc_result_t
-dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r) {
+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) {
+	REQUIRE(QP_VALID(qp));
-		INSIST(qpi->sp == 0);
+
-		qpi->magic = 0;
+	node = get_root(qp);
+	if (node == NULL) {
 		return (ISC_R_NOMORE);
 	}
 
-	/* push branch nodes onto the stack until we reach a leaf */
+	do {
-	for (;;) {
+		if (qpi->stack[qpi->sp] == NULL) {
-		qp_node_t *n = ref_ptr(qp, qpi->stack[qpi->sp].ref);
+			/* newly initialized iterator: use the root node */
-		if (node_tag(n) == LEAF_TAG) {
+			INSIST(qpi->sp == 0);
-			SET_IF_NOT_NULL(pval_r, leaf_pval(n));
+			qpi->stack[0] = node;
-			SET_IF_NOT_NULL(ival_r, leaf_ival(n));
+		} else if (!initial_branch) {
-			break;
+			/*
+			 * 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) {
+		 * if we're at a branch now, push its first (or last) twig
-		qpi->sp--;
+		 * onto the stack and loop again.
-	}
+		 */
+		if (is_branch(node)) {
+			qpi->sp++;
+			INSIST(qpi->sp < DNS_QP_MAXKEY);
 
-	/* move across to the next twig */
+			qp_node_t *twigs = ref_ptr(qp, branch_twigs_ref(node));
-	if (qpi->stack[qpi->sp].more > 0) {
+			if (!forward) {
-		qpi->stack[qpi->sp].more--;
+				twigs += branch_twigs_size(node) - 1;
-		qpi->stack[qpi->sp].ref++;
+			}
-	} else {
+			node = qpi->stack[qpi->sp] = twigs;
-		INSIST(qpi->sp == 0);
+			initial_branch = false;
-		qpi->stack[qpi->sp].ref = INVALID_REF;
+		}
-	}
+	} 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

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;

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();
 

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;
+		ival = isc_random_uniform(ITER_ITEMS - 1) + 1;
-		void *pval = &item[ival];
+		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);
 }
 

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);