From bbb4cdb92df6113413610814b83f2e65adfadad9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 19 Oct 2021 11:46:37 +0200 Subject: [PATCH] Make isc_heap_create() and isc_heap_insert() return void Previously, the function(s) in the commit subject could fail for various reasons - mostly allocation failures, or other functions returning different return code than ISC_R_SUCCESS. Now, the aforementioned function(s) cannot ever fail and they would always return ISC_R_SUCCESS. Change the function(s) to return void and remove the extra checks in the code that uses them. --- lib/dns/rbtdb.c | 105 ++++++------------------------------- lib/dns/zoneverify.c | 55 +++++-------------- lib/isc/heap.c | 16 ++---- lib/isc/include/isc/heap.h | 8 +-- lib/isc/tests/heap_test.c | 5 +- lib/isc/timer.c | 17 ++---- 6 files changed, 38 insertions(+), 168 deletions(-) diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 7abbace2cd..1f3be609d6 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -558,7 +558,7 @@ expire_header(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, bool tree_locked, static void overmem_purge(dns_rbtdb_t *rbtdb, unsigned int locknum_start, isc_stdtime_t now, bool tree_locked); -static isc_result_t +static void resign_insert(dns_rbtdb_t *rbtdb, int idx, rdatasetheader_t *newheader); static void resign_delete(dns_rbtdb_t *rbtdb, rbtdb_version_t *version, @@ -2587,17 +2587,7 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp, bool commit) { lock = &rbtdb->node_locks[header->node->locknum].lock; NODE_LOCK(lock, isc_rwlocktype_write); if (rollback && !IGNORE(header)) { - isc_result_t result; - result = resign_insert(rbtdb, header->node->locknum, - header); - if (result != ISC_R_SUCCESS) { - isc_log_write(dns_lctx, - DNS_LOGCATEGORY_DATABASE, - DNS_LOGMODULE_ZONE, ISC_LOG_ERROR, - "Unable to reinsert header to " - "re-signing heap: %s", - isc_result_totext(result)); - } + resign_insert(rbtdb, header->node->locknum, header); } decrement_reference(rbtdb, header->node, least_serial, isc_rwlocktype_write, isc_rwlocktype_none, @@ -6025,16 +6015,13 @@ cname_and_other_data(dns_rbtnode_t *node, rbtdb_serial_t serial) { return (false); } -static isc_result_t +static void resign_insert(dns_rbtdb_t *rbtdb, int idx, rdatasetheader_t *newheader) { - isc_result_t result; - INSIST(!IS_CACHE(rbtdb)); INSIST(newheader->heap_index == 0); INSIST(!ISC_LINK_LINKED(newheader, link)); - result = isc_heap_insert(rbtdb->heaps[idx], newheader); - return (result); + isc_heap_insert(rbtdb->heaps[idx], newheader); } /* @@ -6457,20 +6444,9 @@ find_header: newheader, link); } INSIST(rbtdb->heaps != NULL); - result = isc_heap_insert(rbtdb->heaps[idx], - newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + isc_heap_insert(rbtdb->heaps[idx], newheader); } else if (RESIGN(newheader)) { - result = resign_insert(rbtdb, idx, newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + resign_insert(rbtdb, idx, newheader); /* * Don't call resign_delete as we don't need * to reverse the delete. The free_rdataset @@ -6500,13 +6476,7 @@ find_header: idx = newheader->node->locknum; if (IS_CACHE(rbtdb)) { INSIST(rbtdb->heaps != NULL); - result = isc_heap_insert(rbtdb->heaps[idx], - newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + isc_heap_insert(rbtdb->heaps[idx], newheader); if (ZEROTTL(newheader)) { ISC_LIST_APPEND(rbtdb->rdatasets[idx], newheader, link); @@ -6515,12 +6485,7 @@ find_header: newheader, link); } } else if (RESIGN(newheader)) { - result = resign_insert(rbtdb, idx, newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + resign_insert(rbtdb, idx, newheader); resign_delete(rbtdb, rbtversion, header); } if (topheader_prev != NULL) { @@ -6565,12 +6530,7 @@ find_header: idx = newheader->node->locknum; if (IS_CACHE(rbtdb)) { - result = isc_heap_insert(rbtdb->heaps[idx], newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + isc_heap_insert(rbtdb->heaps[idx], newheader); if (ZEROTTL(newheader)) { ISC_LIST_APPEND(rbtdb->rdatasets[idx], newheader, link); @@ -6579,12 +6539,7 @@ find_header: newheader, link); } } else if (RESIGN(newheader)) { - result = resign_insert(rbtdb, idx, newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - return (result); - } + resign_insert(rbtdb, idx, newheader); resign_delete(rbtdb, rbtversion, header); } @@ -7111,13 +7066,8 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, RDATASET_ATTR_RESIGN); newheader->resign = header->resign; newheader->resign_lsb = header->resign_lsb; - result = resign_insert(rbtdb, rbtnode->locknum, - newheader); - if (result != ISC_R_SUCCESS) { - free_rdataset(rbtdb, rbtdb->common.mctx, - newheader); - goto unlock; - } + resign_insert(rbtdb, rbtnode->locknum, + newheader); } /* * We have to set the serial since the rdataslab @@ -7785,7 +7735,6 @@ getsize(dns_db_t *db, dns_dbversion_t *version, uint64_t *records, static isc_result_t setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { dns_rbtdb_t *rbtdb = (dns_rbtdb_t *)db; - isc_result_t result = ISC_R_SUCCESS; rdatasetheader_t *header, oldheader; REQUIRE(VALID_RBTDB(rbtdb)); @@ -7824,11 +7773,11 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { } } else if (resign != 0) { RDATASET_ATTR_SET(header, RDATASET_ATTR_RESIGN); - result = resign_insert(rbtdb, header->node->locknum, header); + resign_insert(rbtdb, header->node->locknum, header); } NODE_UNLOCK(&rbtdb->node_locks[header->node->locknum].lock, isc_rwlocktype_write); - return (result); + return (ISC_R_SUCCESS); } static isc_result_t @@ -8236,11 +8185,7 @@ dns_rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, } sooner = IS_CACHE(rbtdb) ? ttl_sooner : resign_sooner; for (i = 0; i < (int)rbtdb->node_lock_count; i++) { - result = isc_heap_create(hmctx, sooner, set_index, 0, - &rbtdb->heaps[i]); - if (result != ISC_R_SUCCESS) { - goto cleanup_heaps; - } + isc_heap_create(hmctx, sooner, set_index, 0, &rbtdb->heaps[i]); } /* @@ -8395,26 +8340,6 @@ dns_rbtdb_create(isc_mem_t *mctx, const dns_name_t *origin, dns_dbtype_t type, return (ISC_R_SUCCESS); -cleanup_heaps: - if (rbtdb->heaps != NULL) { - for (i = 0; i < (int)rbtdb->node_lock_count; i++) { - if (rbtdb->heaps[i] != NULL) { - isc_heap_destroy(&rbtdb->heaps[i]); - } - } - isc_mem_put(hmctx, rbtdb->heaps, - rbtdb->node_lock_count * sizeof(isc_heap_t *)); - } - - if (rbtdb->rdatasets != NULL) { - isc_mem_put(mctx, rbtdb->rdatasets, - rbtdb->node_lock_count * - sizeof(rdatasetheaderlist_t)); - } - if (rbtdb->rrsetstats != NULL) { - dns_stats_detach(&rbtdb->rrsetstats); - } - cleanup_node_locks: isc_mem_put(mctx, rbtdb->node_locks, rbtdb->node_lock_count * sizeof(rbtdb_nodelock_t)); diff --git a/lib/dns/zoneverify.c b/lib/dns/zoneverify.c index 4faa458b89..b1d82dac2b 100644 --- a/lib/dns/zoneverify.c +++ b/lib/dns/zoneverify.c @@ -395,13 +395,12 @@ chain_equal(const struct nsec3_chain_fixed *e1, return (memcmp(e1 + 1, e2 + 1, data_length) == 0); } -static isc_result_t +static void record_nsec3(const vctx_t *vctx, const unsigned char *rawhash, const dns_rdata_nsec3_t *nsec3, isc_heap_t *chains) { - struct nsec3_chain_fixed *element; + struct nsec3_chain_fixed *element = NULL; + unsigned char *cp = NULL; size_t len; - unsigned char *cp; - isc_result_t result; len = sizeof(*element) + nsec3->next_length * 2 + nsec3->salt_length; @@ -417,13 +416,7 @@ record_nsec3(const vctx_t *vctx, const unsigned char *rawhash, memmove(cp, rawhash, nsec3->next_length); cp += nsec3->next_length; memmove(cp, nsec3->next, nsec3->next_length); - result = isc_heap_insert(chains, element); - if (result != ISC_R_SUCCESS) { - zoneverify_log_error(vctx, "isc_heap_insert failed: %s", - isc_result_totext(result)); - isc_mem_put(vctx->mctx, element, len); - } - return (result); + isc_heap_insert(chains, element); } /* @@ -498,12 +491,7 @@ match_nsec3(const vctx_t *vctx, const dns_name_t *name, /* * Record chain. */ - result = record_nsec3(vctx, rawhash, &nsec3, vctx->expected_chains); - if (result != ISC_R_SUCCESS) { - zoneverify_log_error(vctx, "record_nsec3(): %s", - isc_result_totext(result)); - return (result); - } + record_nsec3(vctx, rawhash, &nsec3, vctx->expected_chains); /* * Make sure there is only one NSEC3 record with this set of @@ -607,6 +595,7 @@ record_found(const vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node, if (nsec3.next_length != isc_buffer_usedlength(&b)) { continue; } + /* * We only care about NSEC3 records that match a NSEC3PARAM * record. @@ -618,12 +607,7 @@ record_found(const vctx_t *vctx, const dns_name_t *name, dns_dbnode_t *node, /* * Record chain. */ - result = record_nsec3(vctx, owner, &nsec3, vctx->found_chains); - if (result != ISC_R_SUCCESS) { - zoneverify_log_error(vctx, "record_nsec3(): %s", - isc_result_totext(result)); - goto cleanup; - } + record_nsec3(vctx, owner, &nsec3, vctx->found_chains); } result = ISC_R_SUCCESS; @@ -1284,11 +1268,9 @@ verifyemptynodes(const vctx_t *vctx, const dns_name_t *name, return (ISC_R_SUCCESS); } -static isc_result_t +static void vctx_init(vctx_t *vctx, isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, dns_name_t *origin, dns_keytable_t *secroots) { - isc_result_t result; - memset(vctx, 0, sizeof(*vctx)); vctx->mctx = mctx; @@ -1310,21 +1292,11 @@ vctx_init(vctx_t *vctx, isc_mem_t *mctx, dns_zone_t *zone, dns_db_t *db, dns_rdataset_init(&vctx->nsec3paramsigs); vctx->expected_chains = NULL; - result = isc_heap_create(mctx, chain_compare, NULL, 1024, - &vctx->expected_chains); - if (result != ISC_R_SUCCESS) { - return (result); - } + isc_heap_create(mctx, chain_compare, NULL, 1024, + &vctx->expected_chains); vctx->found_chains = NULL; - result = isc_heap_create(mctx, chain_compare, NULL, 1024, - &vctx->found_chains); - if (result != ISC_R_SUCCESS) { - isc_heap_destroy(&vctx->expected_chains); - return (result); - } - - return (result); + isc_heap_create(mctx, chain_compare, NULL, 1024, &vctx->found_chains); } static void @@ -1993,10 +1965,7 @@ dns_zoneverify_dnssec(dns_zone_t *zone, dns_db_t *db, dns_dbversion_t *ver, isc_result_t result, vresult = ISC_R_UNSET; vctx_t vctx; - result = vctx_init(&vctx, mctx, zone, db, ver, origin, secroots); - if (result != ISC_R_SUCCESS) { - return (result); - } + vctx_init(&vctx, mctx, zone, db, ver, origin, secroots); result = check_apex_rrsets(&vctx); if (result != ISC_R_SUCCESS) { diff --git a/lib/isc/heap.c b/lib/isc/heap.c index 25b813c0c4..0f7de62c86 100644 --- a/lib/isc/heap.c +++ b/lib/isc/heap.c @@ -78,7 +78,7 @@ heap_check(isc_heap_t *heap) { #define heap_check(x) (void)0 #endif /* ifdef ISC_HEAP_CHECK */ -isc_result_t +void isc_heap_create(isc_mem_t *mctx, isc_heapcompare_t compare, isc_heapindex_t idx, unsigned int size_increment, isc_heap_t **heapp) { isc_heap_t *heap; @@ -102,8 +102,6 @@ isc_heap_create(isc_mem_t *mctx, isc_heapcompare_t compare, isc_heapindex_t idx, heap->index = idx; *heapp = heap; - - return (ISC_R_SUCCESS); } void @@ -123,7 +121,7 @@ isc_heap_destroy(isc_heap_t **heapp) { isc_mem_putanddetach(&heap->mctx, heap, sizeof(*heap)); } -static bool +static void resize(isc_heap_t *heap) { void **new_array; unsigned int new_size; @@ -139,8 +137,6 @@ resize(isc_heap_t *heap) { } heap->size = new_size; heap->array = new_array; - - return (true); } static void @@ -194,7 +190,7 @@ sink_down(isc_heap_t *heap, unsigned int i, void *elt) { heap_check(heap); } -isc_result_t +void isc_heap_insert(isc_heap_t *heap, void *elt) { unsigned int new_last; @@ -203,14 +199,12 @@ isc_heap_insert(isc_heap_t *heap, void *elt) { heap_check(heap); new_last = heap->last + 1; RUNTIME_CHECK(new_last > 0); /* overflow check */ - if (new_last >= heap->size && !resize(heap)) { - return (ISC_R_NOMEMORY); + if (new_last >= heap->size) { + resize(heap); } heap->last = new_last; float_up(heap, new_last, elt); - - return (ISC_R_SUCCESS); } void diff --git a/lib/isc/include/isc/heap.h b/lib/isc/include/isc/heap.h index ec28ed5e31..98ae74c3f7 100644 --- a/lib/isc/include/isc/heap.h +++ b/lib/isc/include/isc/heap.h @@ -46,7 +46,7 @@ typedef void (*isc_heapaction_t)(void *, void *); typedef struct isc_heap isc_heap_t; -isc_result_t +void isc_heap_create(isc_mem_t *mctx, isc_heapcompare_t compare, isc_heapindex_t index, unsigned int size_increment, isc_heap_t **heapp); @@ -71,10 +71,6 @@ isc_heap_create(isc_mem_t *mctx, isc_heapcompare_t compare, * used, which is currently 1024, allowing space for an additional 1024 * heap elements to be inserted before adding more space. *\li "heapp" is not NULL, and "*heap" is NULL. - * - * Returns: - *\li ISC_R_SUCCESS - success - *\li ISC_R_NOMEMORY - insufficient memory */ void @@ -86,7 +82,7 @@ isc_heap_destroy(isc_heap_t **heapp); *\li "heapp" is not NULL and "*heap" points to a valid isc_heap_t. */ -isc_result_t +void isc_heap_insert(isc_heap_t *heap, void *elt); /*!< * \brief Inserts a new element into a heap. diff --git a/lib/isc/tests/heap_test.c b/lib/isc/tests/heap_test.c index 7dac7d6ca1..5c41b20d32 100644 --- a/lib/isc/tests/heap_test.c +++ b/lib/isc/tests/heap_test.c @@ -76,17 +76,14 @@ idx(void *p, unsigned int i) { static void isc_heap_delete_test(void **state) { isc_heap_t *heap = NULL; - isc_result_t result; struct e e1 = { 100, 0 }; UNUSED(state); - result = isc_heap_create(test_mctx, compare, idx, 0, &heap); - assert_int_equal(result, ISC_R_SUCCESS); + isc_heap_create(test_mctx, compare, idx, 0, &heap); assert_non_null(heap); isc_heap_insert(heap, &e1); - assert_int_equal(result, ISC_R_SUCCESS); assert_int_equal(e1.index, 1); isc_heap_delete(heap, e1.index); diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 9a8e97b73d..30de75632f 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -97,7 +97,6 @@ isc_timermgr_poke(isc_timermgr_t *manager); static inline isc_result_t schedule(isc_timer_t *timer, isc_time_t *now, bool signal_ok) { - isc_result_t result; isc_timermgr_t *manager; isc_time_t due; @@ -113,7 +112,7 @@ schedule(isc_timer_t *timer, isc_time_t *now, bool signal_ok) { * Compute the new due time. */ if (timer->type != isc_timertype_once) { - result = isc_time_add(now, &timer->interval, &due); + isc_result_t result = isc_time_add(now, &timer->interval, &due); if (result != ISC_R_SUCCESS) { return (result); } @@ -158,11 +157,7 @@ schedule(isc_timer_t *timer, isc_time_t *now, bool signal_ok) { } } else { timer->due = due; - result = isc_heap_insert(manager->heap, timer); - if (result != ISC_R_SUCCESS) { - INSIST(result == ISC_R_NOMEMORY); - return (ISC_R_NOMEMORY); - } + isc_heap_insert(manager->heap, timer); manager->nscheduled++; } @@ -657,7 +652,6 @@ set_index(void *what, unsigned int index) { isc_result_t isc__timermgr_create(isc_mem_t *mctx, isc_timermgr_t **managerp) { isc_timermgr_t *manager; - isc_result_t result; /* * Create a timer manager. @@ -674,12 +668,7 @@ isc__timermgr_create(isc_mem_t *mctx, isc_timermgr_t **managerp) { manager->nscheduled = 0; isc_time_settoepoch(&manager->due); manager->heap = NULL; - result = isc_heap_create(mctx, sooner, set_index, 0, &manager->heap); - if (result != ISC_R_SUCCESS) { - INSIST(result == ISC_R_NOMEMORY); - isc_mem_put(mctx, manager, sizeof(*manager)); - return (ISC_R_NOMEMORY); - } + isc_heap_create(mctx, sooner, set_index, 0, &manager->heap); isc_mutex_init(&manager->lock); isc_mem_attach(mctx, &manager->mctx); isc_condition_init(&manager->wakeup);