From 6512137c5251ad2f3aec16eefbb357fb3d239b95 Mon Sep 17 00:00:00 2001 From: Tony Finch Date: Thu, 6 Apr 2023 11:24:47 +0100 Subject: [PATCH] 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. --- lib/dns/include/dns/qp.h | 32 +++++++++++++++++++------------- lib/dns/qp.c | 27 ++++++++++++++------------- lib/dns/zt.c | 2 +- tests/bench/qpmulti.c | 3 ++- tests/dns/qp_test.c | 8 ++++++-- tests/dns/qpmulti_test.c | 8 ++++++-- 6 files changed, 48 insertions(+), 32 deletions(-) diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index a425a4c257..e2e4544d7a 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -445,12 +445,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: @@ -464,13 +463,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 @@ -487,13 +485,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 @@ -517,10 +514,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)` @@ -531,10 +532,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 @@ -564,6 +569,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, @@ -578,8 +586,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 diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 60acf13ed5..e0fe6799b0 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -1627,7 +1627,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)); @@ -1657,6 +1657,8 @@ dns_qp_deletekey(dns_qp_t *qp, const dns_qpkey_t search_key, return (ISC_R_NOTFOUND); } + OUTARG(pval_r, leaf_pval(n)); + OUTARG(ival_r, leaf_ival(n)); detach_leaf(qp, n); qp->leaf_count--; @@ -1699,10 +1701,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)); } /*********************************************************************** @@ -1730,8 +1733,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; @@ -1745,8 +1746,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); - *ival_r = leaf_ival(n); + OUTARG(pval_r, leaf_pval(n)); + OUTARG(ival_r, leaf_ival(n)); break; } qpi->sp++; @@ -1811,8 +1812,8 @@ dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key, return (ISC_R_NOTFOUND); } - *pval_r = leaf_pval(n); - *ival_r = leaf_ival(n); + OUTARG(pval_r, leaf_pval(n)); + OUTARG(ival_r, leaf_ival(n)); return (ISC_R_SUCCESS); } @@ -1902,8 +1903,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); - *ival_r = leaf_ival(n); + OUTARG(pval_r, leaf_pval(n)); + OUTARG(ival_r, leaf_ival(n)); if (offset == QPKEY_EQUAL) { return (result); } else { @@ -1913,8 +1914,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); - *ival_r = leaf_ival(n); + OUTARG(pval_r, leaf_pval(n)); + OUTARG(ival_r, leaf_ival(n)); return (DNS_R_PARTIALMATCH); } } diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 5095729195..0c64f74636 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -157,7 +157,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); diff --git a/tests/bench/qpmulti.c b/tests/bench/qpmulti.c index 7034ac983f..1181d3bf8b 100644 --- a/tests/bench/qpmulti.c +++ b/tests/bench/qpmulti.c @@ -310,7 +310,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++; diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index ee9b30da00..caaa21b5be 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -188,7 +188,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; } @@ -340,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++]); diff --git a/tests/dns/qpmulti_test.c b/tests/dns/qpmulti_test.c index 16e5350dec..4f0c4bd6a8 100644 --- a/tests/dns/qpmulti_test.c +++ b/tests/dns/qpmulti_test.c @@ -283,9 +283,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, - item[i].len); + void *pvald = NULL; + 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,