Improve qp-trie leaf return values

Make the `pval_r` and `ival_r` out arguments optional.

Add `pval_r` and `ival_r` out arguments to `dns_qp_deletekey()`
and `dns_qp_deletename()`, to return the deleted leaf.

fuzz/dns_qp.c

@@ -179,7 +179,8 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
 				UNREACHABLE();
 			}
 		} else {
-			result = dns_qp_deletekey(qp, item[i].key, item[i].len);
+			result = dns_qp_deletekey(qp, item[i].key, item[i].len,
+						  NULL, NULL);
 			TRACE("count %zu del %s %zu >%s<", count,
 			      isc_result_toid(result), i, item[i].ascii);
 			if (result == ISC_R_SUCCESS) {

lib/dns/include/dns/qp.h

@@ -437,12 +437,11 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key,
 /*%<
  * Find a leaf in a qp-trie that matches the given search key
  *
- * The leaf values are assigned to `*pval_r` and `*ival_r`
+ * 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  `pval_r != NULL`
- * \li  `ival_r != NULL`
  * \li	`search_keylen < sizeof(dns_qpkey_t)`
  *
  * Returns:
@@ -456,13 +455,12 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r,
 /*%<
  * Find a leaf in a qp-trie that matches the given DNS name
  *
- * The leaf values are assigned to `*pval_r` and `*ival_r`
+ * 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  `pval_r != NULL`
- * \li  `ival_r != NULL`
  *
  * Returns:
  * \li  ISC_R_NOTFOUND if the trie has no leaf with a matching key
@@ -479,13 +477,12 @@ dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name,
  * If the DNS_QPFIND_NOEXACT option is set, find a strict parent
  * domain not equal to the search name.
  *
- * The leaf values are assigned to `*pval_r` and `*ival_r`
+ * 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  `pval_r != NULL`
- * \li  `ival_r != NULL`
  *
  * Returns:
  * \li  ISC_R_SUCCESS if an exact match was found
@@ -509,10 +506,14 @@ dns_qp_insert(dns_qp_t *qp, void *pval, uint32_t ival);
  */
 
 isc_result_t
-dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t key, size_t keylen);
+dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t key, size_t keylen,
+		 void **pval_r, uint32_t *ival_r);
 /*%<
  * Delete a leaf from a qp-trie that matches the given key
  *
+ * 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  `qp` is a pointer to a valid qp-trie
  * \li	`keylen < sizeof(dns_qpkey_t)`
@@ -523,10 +524,14 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t key, size_t keylen);
  */
 
 isc_result_t
-dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name);
+dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name, void **pval_r,
+		  uint32_t *ival_r);
 /*%<
  * Delete a leaf from a qp-trie that matches the given DNS name
  *
+ * 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  `qp` is a pointer to a valid qp-trie
  * \li  `name` is a pointer to a valid qp-trie
@@ -556,6 +561,9 @@ dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r);
 /*%<
  * Get the next leaf object of a trie in lexicographic order of its keys.
  *
+ * The leaf values are assigned to whichever of `*pval_r` and `*ival_r`
+ * are not null, unless the return value is ISC_R_NOMORE.
+ *
  * NOTE: see the safety note under `dns_qpiter_init()`.
  *
  * For example,
@@ -570,8 +578,6 @@ dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r);
  *
  * Requires:
  * \li  `qpi` is a pointer to a valid qp iterator
- * \li  `pval_r != NULL`
- * \li  `ival_r != NULL`
  *
  * Returns:
  * \li  ISC_R_SUCCESS if a leaf was found and pval_r and ival_r were set

lib/dns/qp.c

@@ -1613,7 +1613,7 @@ growbranch:
 
 isc_result_t
 dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key,
-		 size_t search_keylen) {
+		 size_t search_keylen, void **pval_r, uint32_t *ival_r) {
 	REQUIRE(QP_VALID(qp));
 	REQUIRE(search_keylen < sizeof(dns_qpkey_t));
 
@@ -1643,6 +1643,8 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key,
 		return (ISC_R_NOTFOUND);
 	}
 
+	SET_IF_NOT_NULL(pval_r, leaf_pval(n));
+	SET_IF_NOT_NULL(ival_r, leaf_ival(n));
 	detach_leaf(qp, n);
 	qp->leaf_count--;
 
@@ -1685,10 +1687,11 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key,
 }
 
 isc_result_t
-dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name) {
+dns_qp_deletename(dns_qp_t *qp, const dns_name_t *name, void **pval_r,
+		  uint32_t *ival_r) {
 	dns_qpkey_t key;
 	size_t keylen = dns_qpkey_fromname(key, name);
-	return (dns_qp_deletekey(qp, key, keylen));
+	return (dns_qp_deletekey(qp, key, keylen, pval_r, ival_r));
 }
 
 /***********************************************************************
@@ -1716,8 +1719,6 @@ isc_result_t
 dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r) {
 	REQUIRE(QPITER_VALID(qpi));
 	REQUIRE(QP_VALID(qpi->qp));
-	REQUIRE(pval_r != NULL);
-	REQUIRE(ival_r != NULL);
 
 	dns_qpreader_t *qp = qpi->qp;
 
@@ -1731,8 +1732,8 @@ dns_qpiter_next(dns_qpiter_t *qpi, void **pval_r, uint32_t *ival_r) {
 	for (;;) {
 		qp_node_t *n = ref_ptr(qp, qpi->stack[qpi->sp].ref);
 		if (node_tag(n) == LEAF_TAG) {
-			*pval_r = leaf_pval(n);
+			SET_IF_NOT_NULL(pval_r, leaf_pval(n));
-			*ival_r = leaf_ival(n);
+			SET_IF_NOT_NULL(ival_r, leaf_ival(n));
 			break;
 		}
 		qpi->sp++;
@@ -1772,8 +1773,6 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key,
 	qp_node_t *n = NULL;
 
 	REQUIRE(QP_VALID(qp));
-	REQUIRE(pval_r != NULL);
-	REQUIRE(ival_r != NULL);
 	REQUIRE(search_keylen < sizeof(dns_qpkey_t));
 
 	n = get_root(qp);
@@ -1797,8 +1796,8 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key,
 		return (ISC_R_NOTFOUND);
 	}
 
-	*pval_r = leaf_pval(n);
+	SET_IF_NOT_NULL(pval_r, leaf_pval(n));
-	*ival_r = leaf_ival(n);
+	SET_IF_NOT_NULL(ival_r, leaf_ival(n));
 	return (ISC_R_SUCCESS);
 }
 
@@ -1827,8 +1826,6 @@ dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name,
 	} label[DNS_NAME_MAXLABELS];
 
 	REQUIRE(QP_VALID(qp));
-	REQUIRE(pval_r != NULL);
-	REQUIRE(ival_r != NULL);
 
 	searchlen = dns_qpkey_fromname(search, name);
 	if ((options & DNS_QPFIND_NOEXACT) != 0) {
@@ -1888,8 +1885,8 @@ dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name,
 	offset = qpkey_compare(search, searchlen, found, foundlen);
 
 	if (offset == QPKEY_EQUAL || offset == foundlen) {
-		*pval_r = leaf_pval(n);
+		SET_IF_NOT_NULL(pval_r, leaf_pval(n));
-		*ival_r = leaf_ival(n);
+		SET_IF_NOT_NULL(ival_r, leaf_ival(n));
 		if (offset == QPKEY_EQUAL) {
 			return (result);
 		} else {
@@ -1899,8 +1896,8 @@ dns_qp_findname_parent(dns_qpreadable_t qpr, const dns_name_t *name,
 	while (labels-- > 0) {
 		if (offset > label[labels].off) {
 			n = ref_ptr(qp, label[labels].ref);
-			*pval_r = leaf_pval(n);
+			SET_IF_NOT_NULL(pval_r, leaf_pval(n));
-			*ival_r = leaf_ival(n);
+			SET_IF_NOT_NULL(ival_r, leaf_ival(n));
 			return (DNS_R_PARTIALMATCH);
 		}
 	}

lib/dns/zt.c

@@ -156,7 +156,7 @@ dns_zt_unmount(dns_zt_t *zt, dns_zone_t *zone) {
 	REQUIRE(VALID_ZT(zt));
 
 	dns_qpmulti_write(zt->multi, &qp);
-	result = dns_qp_deletename(qp, dns_zone_getorigin(zone));
+	result = dns_qp_deletename(qp, dns_zone_getorigin(zone), NULL, NULL);
 	dns_qp_compact(qp, DNS_QPGC_MAYBE);
 	dns_qpmulti_commit(zt->multi, &qp);
 
@@ -169,7 +169,6 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options,
 	isc_result_t result;
 	dns_qpread_t qpr;
 	void *pval = NULL;
-	uint32_t ival;
 	dns_ztfind_t exactmask = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_EXACT;
 	dns_ztfind_t exactopts = options & exactmask;
 
@@ -179,12 +178,12 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options,
 	dns_qpmulti_query(zt->multi, &qpr);
 
 	if (exactopts == DNS_ZTFIND_EXACT) {
-		result = dns_qp_getname(&qpr, name, &pval, &ival);
+		result = dns_qp_getname(&qpr, name, &pval, NULL);
 	} else if (exactopts == DNS_ZTFIND_NOEXACT) {
 		result = dns_qp_findname_parent(&qpr, name, DNS_QPFIND_NOEXACT,
-						&pval, &ival);
+						&pval, NULL);
 	} else {
-		result = dns_qp_findname_parent(&qpr, name, 0, &pval, &ival);
+		result = dns_qp_findname_parent(&qpr, name, 0, &pval, NULL);
 	}
 	dns_qpread_destroy(zt->multi, &qpr);
 
@@ -508,7 +507,6 @@ dns_zt_apply(dns_zt_t *zt, bool stop, isc_result_t *sub,
 	dns_qpiter_t qpi;
 	dns_qpread_t qpr;
 	void *zone = NULL;
-	uint32_t ival;
 
 	REQUIRE(VALID_ZT(zt));
 	REQUIRE(action != NULL);
@@ -516,7 +514,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, &ival) == ISC_R_SUCCESS) {
+	while (dns_qpiter_next(&qpi, &zone, NULL) == ISC_R_SUCCESS) {
 		result = action(zone, uap);
 		if (tresult == ISC_R_SUCCESS) {
 			tresult = result;

tests/bench/load-names.c

@@ -373,10 +373,7 @@ sqz_qp(void *qp) {
 
 static isc_result_t
 get_qp(void *qp, size_t count, void **pval) {
-	uint32_t ival = 0;
+	return (dns_qp_getname(qp, &item[count].fixed.name, pval, NULL));
-	isc_result_t result = dns_qp_getname(qp, &item[count].fixed.name, pval,
-					     &ival);
-	return (result);
 }
 
 static void *

tests/bench/qpmulti.c

@@ -311,7 +311,8 @@ mutate_transactions(uv_idle_t *idle) {
 			uint32_t i = isc_random_uniform(args->max_item);
 			if (item[i].present) {
 				isc_result_t result = dns_qp_deletekey(
-					qp, item[i].key, item[i].len);
+					qp, item[i].key, item[i].len, NULL,
+					NULL);
 				INSIST(result == ISC_R_SUCCESS);
 				item[i].present = false;
 				args->present++;

tests/dns/qp_test.c

@@ -189,7 +189,11 @@ ISC_RUN_TEST_IMPL(qpiter) {
 		dns_qpkey_t key;
 		size_t len = qpiter_makekey(key, item, pval, ival);
 		if (dns_qp_insert(qp, pval, ival) == ISC_R_EXISTS) {
-			dns_qp_deletekey(qp, key, len);
+			void *pvald = NULL;
+			uint32_t ivald = 0;
+			dns_qp_deletekey(qp, key, len, &pvald, &ivald);
+			assert_ptr_equal(pval, pvald);
+			assert_int_equal(ival, ivald);
 			item[ival] = 0;
 		}
 
@@ -257,7 +261,6 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) {
 		dns_fixedname_t fixed;
 		dns_name_t *name = dns_fixedname_name(&fixed);
 		void *pval = NULL;
-		uint32_t ival;
 
 #if 0
 		fprintf(stderr, "%s %u %s %s\n", check[i].query,
@@ -266,7 +269,7 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) {
 #endif
 		dns_test_namefromstring(check[i].query, &fixed);
 		result = dns_qp_findname_parent(qp, name, check[i].options,
-						&pval, &ival);
+						&pval, NULL);
 		assert_int_equal(result, check[i].result);
 		if (check[i].found == NULL) {
 			assert_null(pval);
@@ -341,7 +344,7 @@ ISC_RUN_TEST_IMPL(partialmatch) {
 
 	/* what if entries in the trie are relative to the zone apex? */
 	dns_qpkey_t rootkey = { SHIFT_NOBYTE };
-	result = dns_qp_deletekey(qp, rootkey, 1);
+	result = dns_qp_deletekey(qp, rootkey, 1, NULL, NULL);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	INSIST(insert[i][0] == '\0');
 	insert_str(qp, insert[i++]);

tests/dns/qpmulti_test.c

@@ -159,7 +159,6 @@ random_byte(void) {
 static void
 setup_items(void) {
 	void *pval = NULL;
-	uint32_t ival = ~0U;
 	dns_qp_t *qp = NULL;
 	dns_qp_create(mctx, &test_methods, NULL, &qp);
 	for (size_t i = 0; i < ARRAY_SIZE(item); i++) {
@@ -172,7 +171,7 @@ setup_items(void) {
 			memmove(item[i].ascii, item[i].key, len);
 			qp_test_keytoascii(item[i].ascii, len);
 		} while (dns_qp_getkey(qp, item[i].key, item[i].len, &pval,
-				       &ival) == ISC_R_SUCCESS);
+				       NULL) == ISC_R_SUCCESS);
 		assert_int_equal(dns_qp_insert(qp, &item[i], i), ISC_R_SUCCESS);
 	}
 	dns_qp_destroy(&qp);
@@ -283,9 +282,13 @@ one_transaction(dns_qpmulti_t *qpm) {
 		if (item[i].in_rw) {
 			/* TRACE("delete %zu %.*s", i,
 				 item[i].len, item[i].ascii); */
-			result = dns_qp_deletekey(qpw, item[i].key,
+			void *pvald = NULL;
-						  item[i].len);
+			uint32_t ivald = 0;
+			result = dns_qp_deletekey(qpw, item[i].key, item[i].len,
+						  &pvald, &ivald);
 			ASSERT(result == ISC_R_SUCCESS);
+			ASSERT(pvald == &item[i]);
+			ASSERT(ivald == i);
 			item[i].in_rw = false;
 		} else {
 			/* TRACE("insert %zu %.*s", i,