From: Arran Cudbard-Bell Date: Tue, 4 Apr 2023 00:34:36 +0000 (-0600) Subject: slab: Style guide says alloc functions should return the newly allocated structure... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=adc2b27f351490d933977dda86c48f8ca5ec6bf2;p=thirdparty%2Ffreeradius-server.git slab: Style guide says alloc functions should return the newly allocated structure unless they have complex failure conditions which slabs don't https://wiki.freeradius.org/contributing/coding-standards#return-types_pointer --- diff --git a/src/lib/util/slab.h b/src/lib/util/slab.h index 3c2e07c62e..98266a783a 100644 --- a/src/lib/util/slab.h +++ b/src/lib/util/slab.h @@ -160,7 +160,6 @@ DIAG_OFF(unused-function) \ /** Allocate a slab list to manage slabs of allocated memory \ * \ * @param[in] ctx in which to allocate the slab list. \ - * @param[out] out slab_list that has been allocated. \ * @param[in] el Event list in which to run clean up function. \ * @param[in] config Slab config parameters. \ * @param[in] alloc Optional callback to use when allocating new elements. \ @@ -168,31 +167,40 @@ DIAG_OFF(unused-function) \ * @param[in] uctx to pass to callbacks. \ * @param[in] release_reset Should elements be reset and children freed when the element is released.\ * @param[in] reserve_mru If true, the most recently used element will be returned when an element is reserved. \ + * @return \ + * - A new slab. \ + * - NULL on error. \ */ \ - static inline CC_HINT(nonnull(2)) int _name ## _slab_list_alloc(TALLOC_CTX *ctx, \ - _name ## _slab_list_t **out, \ - fr_event_list_t *el, \ - fr_slab_config_t const *config, \ - _type ## _slab_alloc_t alloc, \ - _type ## _slab_reserve_t reserve, \ - void *uctx, \ - bool release_reset, \ - bool reserve_mru) \ + static inline _name ## _slab_list_t *_name ## _slab_list_alloc(TALLOC_CTX *ctx, \ + fr_event_list_t *el, \ + fr_slab_config_t const *config, \ + _type ## _slab_alloc_t alloc, \ + _type ## _slab_reserve_t reserve, \ + void *uctx, \ + bool release_reset, \ + bool reserve_mru) \ { \ - MEM(*out = talloc_zero(ctx, _name ## _slab_list_t)); \ - (*out)->el = el; \ - (*out)->config = *config; \ - if ((*out)->config.elements_per_slab == 0) \ - (*out)->config.elements_per_slab = (config->min_elements ? config->min_elements : 1); \ - (*out)->alloc = alloc; \ - (*out)->reserve = reserve; \ - (*out)->uctx = uctx; \ - (*out)->release_reset = release_reset; \ - (*out)->reserve_mru = reserve_mru; \ - _name ## _slab_init(&(*out)->reserved); \ - _name ## _slab_init(&(*out)->avail); \ - if (el) (void) fr_event_timer_in(*out, el, &(*out)->ev, config->interval, _ ## _name ## _slab_cleanup, *out); \ - return 0; \ + _name ## _slab_list_t *slab; \ + MEM(slab = talloc_zero(ctx, _name ## _slab_list_t)); \ + slab->el = el; \ + slab->config = *config; \ + if (slab->config.elements_per_slab == 0) { \ + slab->config.elements_per_slab = (config->min_elements ? config->min_elements : 1); \ + } \ + slab->alloc = alloc; \ + slab->reserve = reserve; \ + slab->uctx = uctx; \ + slab->release_reset = release_reset; \ + slab->reserve_mru = reserve_mru; \ + _name ## _slab_init(&slab->reserved); \ + _name ## _slab_init(&slab->avail); \ + if (el) { \ + if (unlikely(fr_event_timer_in(slab, el, &slab->ev, config->interval, _ ## _name ## _slab_cleanup, slab) < 0)) { \ + talloc_free(slab); \ + return NULL; \ + }; \ + } \ + return slab; \ } \ \ /** Callback for talloc freeing a slab element \ diff --git a/src/lib/util/slab_tests.c b/src/lib/util/slab_tests.c index e6fc877e67..69be54714b 100644 --- a/src/lib/util/slab_tests.c +++ b/src/lib/util/slab_tests.c @@ -57,13 +57,11 @@ static void test_alloc(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[5]; test_uctx_t test_uctx, test_uctx2; - int ret = -1; /* * Each slab will contain 2 elements, maximum of 4 elements allocated from slabs. */ - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -129,15 +127,13 @@ static void test_alloc_fail(void) { test_slab_list_t *test_slab_list; test_element_t *test_elements[5]; - int ret = -1; fr_slab_config_t slab_config = def_slab_config; /* * Each slab will contain 2 elements, maximum of 4 elements allocated from slabs. */ slab_config.at_max_fail = true; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -170,10 +166,8 @@ static void test_reuse_reset(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[5]; test_uctx_t test_uctx; - int ret = -1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -232,10 +226,8 @@ static void test_reuse_noreset(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[3]; test_uctx_t test_uctx; - int ret = -1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, false, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, false, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -286,14 +278,12 @@ static void test_reserve_mru(void) { test_slab_list_t *test_slab_list; test_element_t *test_elements[2]; - int ret = -1; /* * First use a slab list with reserve_mru = false to verify that the two reservations * result in different elements being returned */ - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -311,8 +301,7 @@ static void test_reserve_mru(void) /* * Now use a slab list with reserve_mru = true */ - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, true, true); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, true); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -336,10 +325,8 @@ static void test_free(void) test_slab_list_t *test_slab_list; test_element_t *test_element; test_uctx_t test_uctx; - int ret = -1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &def_slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -372,12 +359,10 @@ static void test_init(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[2]; test_conf_t test_conf = { .initial = 10 }; - int ret = -1; fr_slab_config_t slab_config = def_slab_config; slab_config.elements_per_slab = 1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &slab_config, test_element_alloc, NULL, &test_conf, false, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, test_element_alloc, NULL, &test_conf, false, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -412,12 +397,10 @@ static void test_reserve(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[2]; test_conf_t test_conf = { .initial = 10 }; - int ret = -1; fr_slab_config_t slab_config = def_slab_config; slab_config.elements_per_slab = 1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &slab_config, NULL, test_element_alloc, &test_conf, false, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, NULL, test_element_alloc, &test_conf, false, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -459,12 +442,10 @@ static void test_init_reserve(void) test_slab_list_t *test_slab_list; test_element_t *test_elements[2]; test_conf_t test_conf = { .initial = 10 }; - int ret = -1; fr_slab_config_t slab_config = def_slab_config; slab_config.elements_per_slab = 1; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &slab_config, test_element_alloc, test_element_reserve, &test_conf, false, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, test_element_alloc, test_element_reserve, &test_conf, false, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -500,15 +481,14 @@ static void test_clearup_1(void) fr_event_list_t *el; test_slab_list_t *test_slab_list; test_element_t *test_elements[6]; - int i, events, ret = -1; + int i, events; fr_slab_config_t slab_config = def_slab_config; el = fr_event_list_alloc(ctx, NULL, NULL); fr_event_list_set_time_func(el, test_time); slab_config.max_elements = 6; - ret = test_slab_list_alloc(NULL, &test_slab_list, el, &slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, el, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -555,7 +535,7 @@ static void test_clearup_2(void) fr_event_list_t *el; test_slab_list_t *test_slab_list; test_element_t *test_elements[20]; - int i, events, ret = -1; + int i, events; fr_slab_config_t slab_config = def_slab_config; el = fr_event_list_alloc(ctx, NULL, NULL); @@ -563,8 +543,7 @@ static void test_clearup_2(void) slab_config.min_elements = 16; slab_config.max_elements = 20; - ret = test_slab_list_alloc(NULL, &test_slab_list, el, &slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, el, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -621,7 +600,7 @@ static void test_clearup_3(void) fr_event_list_t *el; test_slab_list_t *test_slab_list; test_element_t *test_elements[20]; - int i, events, ret = -1; + int i, events; fr_slab_config_t slab_config = def_slab_config; el = fr_event_list_alloc(ctx, NULL, NULL); @@ -629,8 +608,7 @@ static void test_clearup_3(void) slab_config.min_elements = 0; slab_config.max_elements = 20; - ret = test_slab_list_alloc(NULL, &test_slab_list, el, &slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, el, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -716,7 +694,7 @@ static void test_realloc(void) fr_event_list_t *el; test_slab_list_t *test_slab_list; test_element_t *test_elements[20]; - int i, events, ret = -1; + int i, events; fr_slab_config_t slab_config = def_slab_config; el = fr_event_list_alloc(ctx, NULL, NULL); @@ -724,8 +702,7 @@ static void test_realloc(void) slab_config.min_elements = 0; slab_config.max_elements = 20; - ret = test_slab_list_alloc(NULL, &test_slab_list, el, &slab_config, NULL, NULL, NULL, true, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, el, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -777,14 +754,12 @@ static void test_child_alloc(void) { test_slab_list_t *test_slab_list; test_element_t *test_elements[2]; - int ret = -1; fr_slab_config_t slab_config = def_slab_config; slab_config.max_elements = 2; slab_config.num_children = 1; slab_config.child_pool_size = 128; - ret = test_slab_list_alloc(NULL, &test_slab_list, NULL, &slab_config, NULL, NULL, NULL, false, false); - TEST_CHECK(ret == 0); + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, NULL, NULL, NULL, false, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; diff --git a/src/modules/rlm_imap/rlm_imap.c b/src/modules/rlm_imap/rlm_imap.c index 4534e527f2..93b4c4a420 100644 --- a/src/modules/rlm_imap/rlm_imap.c +++ b/src/modules/rlm_imap/rlm_imap.c @@ -247,8 +247,9 @@ static int mod_thread_instantiate(module_thread_inst_ctx_t const *mctx) rlm_imap_thread_t *t = talloc_get_type_abort(mctx->thread, rlm_imap_thread_t); fr_curl_handle_t *mhandle; - if (imap_slab_list_alloc(t, &t->slab, mctx->el, &inst->conn_config.reuse, - imap_conn_alloc, NULL, inst, false, false) < 0) { + if (!(t->slab = imap_slab_list_alloc(t, mctx->el, &inst->conn_config.reuse, + imap_conn_alloc, NULL, inst, + false, false))) { ERROR("Connection handle pool instantiation failed"); return -1; } diff --git a/src/modules/rlm_rest/rlm_rest.c b/src/modules/rlm_rest/rlm_rest.c index b3bea1a213..360fec2735 100644 --- a/src/modules/rlm_rest/rlm_rest.c +++ b/src/modules/rlm_rest/rlm_rest.c @@ -1165,8 +1165,8 @@ static int mod_thread_instantiate(module_thread_inst_ctx_t const *mctx) t->inst = inst; - if (rest_slab_list_alloc(t, &t->slab, mctx->el, &inst->conn_config.reuse, - rest_conn_alloc, NULL, inst, false, false) < 0) { + if (!(t->slab = rest_slab_list_alloc(t, mctx->el, &inst->conn_config.reuse, + rest_conn_alloc, NULL, inst, false, false))) { ERROR("Connection handle pool instantiation failed"); return -1; } diff --git a/src/modules/rlm_smtp/rlm_smtp.c b/src/modules/rlm_smtp/rlm_smtp.c index f7369ce5e6..3786d67b42 100644 --- a/src/modules/rlm_smtp/rlm_smtp.c +++ b/src/modules/rlm_smtp/rlm_smtp.c @@ -1205,13 +1205,15 @@ static int mod_thread_instantiate(module_thread_inst_ctx_t const *mctx) rlm_smtp_thread_t *t = talloc_get_type_abort(mctx->thread, rlm_smtp_thread_t); fr_curl_handle_t *mhandle; - if (smtp_slab_list_alloc(t, &t->slab_onetime, mctx->el, &inst->conn_config.reuse, - smtp_onetime_conn_alloc, smtp_onetime_conn_init, inst, false, false) < 0) { + if (!(t->slab_onetime = smtp_slab_list_alloc(t, mctx->el, &inst->conn_config.reuse, + smtp_onetime_conn_alloc, smtp_onetime_conn_init, + inst, false, false))) { ERROR("Connection handle pool instantiation failed"); return -1; } - if (smtp_slab_list_alloc(t, &t->slab_persist, mctx->el, &inst->conn_config.reuse, - smtp_persist_conn_alloc, smtp_persist_conn_init, inst, false, true) < 0) { + if (!(t->slab_persist = smtp_slab_list_alloc(t, mctx->el, &inst->conn_config.reuse, + smtp_persist_conn_alloc, smtp_persist_conn_init, + inst, false, true))) { ERROR("Connection handle pool instantiation failed"); return -1; }