From: James Jones Date: Wed, 18 Aug 2021 02:27:00 +0000 (-0500) Subject: Switch to unsigned type for fr_lst_index_t (#4190) X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=97ff4e982fdb55caeb225104c383e639570aec5a;p=thirdparty%2Ffreeradius-server.git Switch to unsigned type for fr_lst_index_t (#4190) * Switch to unsigned type for fr_lst_index_t This involves adding one to the true index when storing it in an item, and subtracting one when reading it for use. * Switch to unsigned type for fr_lst_index_t This involves adding one to the true index when storing it in an item, and subtracting one when reading it for use. * Switch to unsigned type for fr_lst_index_t This involves adding one to the true index when storing it in an item, and subtracting one when reading it for use. --- diff --git a/src/lib/util/lst.c b/src/lib/util/lst.c index 1895af73b4c..11a6861cbdb 100644 --- a/src/lib/util/lst.c +++ b/src/lib/util/lst.c @@ -75,14 +75,30 @@ static inline CC_HINT(always_inline, nonnull) void *index_addr(fr_lst_t *lst, vo return ((uint8_t *)data) + (lst)->offset; } +/* + * Concerning item_index() and item_index_set(): + * To let zero be the value *as stored in an item* that indicates not being in an LST, + * we add one to the real index when storing it and subtract one when retrieving it. + * + * This lets the LST functions use item indices in [0, lst->capacity), important for + * 1. the circular array, which allows an important optimization for fr_lst_pop() + * 2. quick reduction of indices + * + * fr_item_insert() needs to see the value actually stored, hence raw_item_index(). + */ +static inline CC_HINT(always_inline, nonnull) fr_lst_index_t raw_item_index(fr_lst_t *lst, void *data) +{ + return *(fr_lst_index_t *)index_addr(lst, data); +} + static inline CC_HINT(always_inline, nonnull) fr_lst_index_t item_index(fr_lst_t *lst, void *data) { - return (*(fr_lst_index_t *)index_addr(lst, data)); + return raw_item_index(lst, data) - 1; } static inline CC_HINT(always_inline, nonnull) void item_index_set(fr_lst_t *lst, void *data, fr_lst_index_t idx) { - (*(fr_lst_index_t *)index_addr(lst, data)) = idx; + (*(fr_lst_index_t *)index_addr(lst, data)) = idx + 1; } static inline CC_HINT(always_inline, nonnull) fr_lst_index_t index_reduce(fr_lst_t *lst, fr_lst_index_t idx) @@ -690,7 +706,7 @@ int fr_lst_extract(fr_lst_t *lst, void *data) return -1; } - if (unlikely(item_index(lst, data) < 0)) { + if (unlikely(raw_item_index(lst, data) == 0)) { fr_strerror_const("Tried to extract element not in LST"); return -1; } @@ -701,8 +717,6 @@ int fr_lst_extract(fr_lst_t *lst, void *data) int fr_lst_insert(fr_lst_t *lst, void *data) { - fr_lst_index_t data_index; - /* * Expand if need be. Not in the paper, but we want the capability. */ @@ -711,9 +725,7 @@ int fr_lst_insert(fr_lst_t *lst, void *data) /* * Don't insert something that looks like it's already in an LST. */ - data_index = item_index(lst, data); - if (unlikely((data_index > 0) || - ((data_index == 0) && (lst->num_elements > 0) && (lst->idx == 0) && (item(lst, 0) == data)))) { + if (unlikely(raw_item_index(lst, data) > 0)) { fr_strerror_const("Node is already in the LST"); return -1; } diff --git a/src/lib/util/lst.h b/src/lib/util/lst.h index 60720fcdda2..8925c55b19d 100644 --- a/src/lib/util/lst.h +++ b/src/lib/util/lst.h @@ -41,7 +41,7 @@ typedef struct fr_lst_s fr_lst_t; * type of a structure with a member of type fr_lst_index_t. That member's name must be * passed as the _field argument. */ -typedef int fr_lst_index_t; +typedef unsigned int fr_lst_index_t; typedef fr_lst_index_t fr_lst_iter_t; @@ -77,6 +77,12 @@ typedef int8_t (*fr_lst_cmp_t)(void const *a, void const *b); fr_lst_t *_fr_lst_alloc(TALLOC_CTX *ctx, fr_lst_cmp_t cmp, char const *type, size_t offset) CC_HINT(nonnull(2)); /** Check if an entry is inserted into an LST. + * + * @param[in] lst_id An fr_lst_index_t value *as stored in an item* + * + * Thus one should only pass this function an index as retrieved directly from + * the item, *not* the value returned by item_index() (q.v.). + * * This checks a necessary condition for a fr_lst_index_t value to be * that of an inserted entry. A more complete check would need the entry * itself and a pointer to the fr_lst_t it may be inserted in. @@ -84,7 +90,7 @@ fr_lst_t *_fr_lst_alloc(TALLOC_CTX *ctx, fr_lst_cmp_t cmp, char const *type, siz */ static inline bool fr_lst_entry_inserted(fr_lst_index_t lst_id) { - return (lst_id >= 0); + return (lst_id > 0); } void *fr_lst_peek(fr_lst_t *lst) CC_HINT(nonnull); diff --git a/src/lib/util/lst_tests.c b/src/lib/util/lst_tests.c index 733328a56cb..8e454a9cdc3 100644 --- a/src/lib/util/lst_tests.c +++ b/src/lib/util/lst_tests.c @@ -116,7 +116,7 @@ static void lst_test(int skip) TEST_CASE("deletions"); for (int entry = 0; entry < LST_TEST_SIZE; entry += skip) { - TEST_CHECK(array[entry].index != -1); + TEST_CHECK(array[entry].index != 0); TEST_MSG("element %i removed out of order", entry); TEST_CHECK((ret = fr_lst_extract(lst, &array[entry])) >= 0); @@ -125,7 +125,7 @@ static void lst_test(int skip) TEST_CHECK(!fr_lst_contains(lst, &array[entry])); TEST_MSG("element %i removed but still in LST", entry); - TEST_CHECK(array[entry].index == -1); + TEST_CHECK(array[entry].index == 0); TEST_MSG("element %i removed out of order", entry); } @@ -342,7 +342,7 @@ static void lst_cycle(void) removed = 0; for (i = 0; i < LST_CYCLE_SIZE; i++) { - if (array[i].index == -1) { + if (array[i].index == 0) { TEST_CHECK((ret = fr_lst_insert(lst, &array[i])) >= 0); TEST_MSG("insert failed, returned %i - %s", ret, fr_strerror()); inserted++; @@ -407,7 +407,7 @@ static void lst_iter(void) for (int i = 0; i < NVALUES; i++, data = fr_lst_iter_next(lst, &iter)) { TEST_CHECK(data != NULL); TEST_CHECK(!data->visited); - TEST_CHECK(data->index >= 0); + TEST_CHECK(data->index > 0); data->visited = true; }