Skip to content

Move os_memdup into src/utils/allocs - #429

Merged
aloisklink merged 2 commits into
mainfrom
refactor/move-os_memdup
Feb 20, 2023
Merged

Move os_memdup into src/utils/allocs#429
aloisklink merged 2 commits into
mainfrom
refactor/move-os_memdup

Conversation

@aloisklink

Copy link
Copy Markdown
Contributor

Move os_memdup from src/radius/common.h into src/utils/allocs.h.

This function should be with all the other memory allocation functions, so that we can give it a C23 [[nodiscard]] attribute (or GCC/Clang equivalent attribute, since we don't yet support C23).

This PR also documents that the implementation of os_memdup was adapted from commit
dbdda35 in hostapd.


Kind of confusingly, there's actually two versions of the function in PR #380.

os_memdup in src/radius/common.h:

edgesec/src/radius/common.h

Lines 179 to 189 in 666a609

/**
* @brief Allocate duplicate of passed memory chunk
*
* This function allocates a memory block like os_malloc() would, and
* copies the given source buffer into it.
*
* @param src Source buffer to duplicate
* @param len Length of source buffer
* @return void* %NULL if allocation failed, copy of src buffer otherwise
*/
static inline void *os_memdup(const void *src, size_t len) {

sys_memdup in src/utils/allocs.h (this one was added in commit 24a8d46):

/**
* os_memdup - Allocate duplicate of passed memory chunk
*
* This function allocates a memory block like os_malloc() would, and
* copies the given source buffer into it.
*
* @param src Source buffer to duplicate
* @param len Length of source buffer
* @return void* %NULL if allocation failed, copy of src buffer otherwise
*/
void *sys_memdup(const void *src, size_t len);

I've decided to stick my version of this function into src/utils/allocs.h, since @mereacre commited it later in 24a8d46.

Plus, having it in allocs makes more sense, since it also makes it easier for us to add a [[nodiscard]] attribute to the function, so that the compiler throws an error if we don't do anything with the return value of the function.

Document that the implementation of `os_memdup` was adapted from commit
dbdda355d0add3f7d96e3279321d3a63abfc4b32 in hostapd, see
https://w1.fi/cgit/hostap/commit/?id=dbdda355d0add3f7d96e3279321d3a63abfc4b32
@aloisklink aloisklink added the refactor Refactoring code label Feb 17, 2023
@aloisklink aloisklink added this to the RADIUS ERP Server milestone Feb 17, 2023
@aloisklink
aloisklink enabled auto-merge February 17, 2023 19:25
@aloisklink aloisklink mentioned this pull request Feb 17, 2023
@codecov

codecov Bot commented Feb 17, 2023

Copy link
Copy Markdown

Codecov Report

Merging #429 (918301c) into main (ba237fd) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main     #429   +/-   ##
=======================================
  Coverage   53.09%   53.09%           
=======================================
  Files         144      144           
  Lines       19880    19880           
=======================================
  Hits        10555    10555           
  Misses       9325     9325           
Impacted Files Coverage Δ
src/radius/common.h 42.59% <ø> (+3.60%) ⬆️
src/utils/allocs.h 45.45% <0.00%> (-37.88%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Move `os_memdup` from `src/radius/common.h` into `src/utils/allocs.h`.

This function should be with all the other memory allocation functions,
so that we can give it a C23 [[nodiscard]] attribute, see
https://en.cppreference.com/w/c/language/attributes/nodiscard
(or GCC/Clang equivalent attribute, since we don't yet support C23).
@aloisklink
aloisklink force-pushed the refactor/move-os_memdup branch from cda0828 to 918301c Compare February 20, 2023 12:12
@mereacre
mereacre self-requested a review February 20, 2023 16:27
@aloisklink
aloisklink added this pull request to the merge queue Feb 20, 2023
Merged via the queue into main with commit 0839771 Feb 20, 2023
@aloisklink
aloisklink deleted the refactor/move-os_memdup branch February 20, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Refactoring code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants