Refactore memory API to make them portable#3354
Conversation
Sort the entries alphabetically. Signed-off-by: Daniel Wagner <wagi@kernel.org>
libnvme uses posix_memalign to ensure that allocated memory buffers are properly aligned. This API does not exist on Windows, so move this code into a platform abstraction. Since we have to abstract the same APIs for nvme-cli itself, add these helpers to the libnvme API. Signed-off-by: Daniel Wagner <wagi@kernel.org>
5f98aeb to
9715f3e
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the project’s memory allocation helpers by moving them into libnvme as portable exported APIs (with a Windows implementation), and updates nvme-cli/plugins to use the new libnvme_alloc* / libnvme_free* APIs to reduce duplication between libnvme and nvme-cli.
Changes:
- Introduces new exported libnvme allocation APIs (
libnvme_alloc,libnvme_realloc,libnvme_free,libnvme_alloc_huge,libnvme_free_huge) and wires them into the public headers and symbol exports. - Adds a Windows-specific allocator implementation and updates many callers in nvme-cli/plugins to use libnvme allocation + cleanup helpers.
- Adds a
reallocarray()fallback for platforms that lack it and a Meson feature check forreallocarray.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| util/table.c | Adds a local reallocarray() fallback when not provided by libc. |
| util/meson.build | Stops building the old util/mem.c helper (now replaced by libnvme APIs). |
| util/mem.h | Removes the old nvme-cli memory API header. |
| util/cleanup.h | Switches cleanup helpers to use libnvme_free / libnvme_free_huge. |
| plugins/zns/zns.c | Migrates allocations/frees to libnvme_alloc* and cleanup helpers. |
| plugins/wdc/wdc-nvme.c | Migrates allocations/frees to libnvme_alloc* and cleanup helpers. |
| plugins/toshiba/toshiba-nvme.c | Migrates allocations/frees to libnvme_alloc + cleanup helpers and simplifies error paths. |
| plugins/solidigm/solidigm-internal-logs.c | Migrates allocations/frees to libnvme_alloc* and cleanup helpers. |
| plugins/shannon/shannon-nvme.c | Migrates allocations/frees to libnvme_alloc + cleanup helpers. |
| plugins/scaleflux/sfx-nvme.c | Migrates huge allocations to libnvme_alloc_huge / libnvme_free_huge. |
| plugins/ocp/ocp-nvme.c | Migrates allocations/frees to libnvme_alloc* and cleanup helpers. |
| plugins/netapp/netapp-nvme.c | Migrates allocations/frees to libnvme_alloc / libnvme_free. |
| plugins/micron/micron-nvme.c | Migrates allocations/frees to libnvme_alloc* and fixes pointer arithmetic via fw_ptr. |
| plugins/memblaze/memblaze-nvme.c | Migrates allocations/frees to libnvme_alloc* and fixes pointer arithmetic via fw_ptr. |
| plugins/lm/lm-nvme.c | Migrates huge allocations to libnvme_alloc_huge and updates error strings accordingly. |
| plugins/ibm/ibm-nvme.c | Migrates allocations to libnvme_alloc. |
| plugins/feat/feat-nvme.c | Migrates allocations to libnvme_alloc + cleanup helper. |
| nvme.h | Drops include of removed util/mem.h. |
| nvme.c | Migrates many allocations to libnvme_alloc* and cleanup helpers. |
| nvme-rpmb.c | Migrates allocations/frees to libnvme_alloc / libnvme_free and cleanup helpers. |
| meson.build | Adds Meson feature test/config define for reallocarray(). |
| libnvme/src/nvme/util.c | Removes internal __libnvme_alloc / __libnvme_realloc helpers (replaced by new mem module). |
| libnvme/src/nvme/tree.c | Switches an internal allocation to libnvme_alloc + cleanup helper. |
| libnvme/src/nvme/private.h | Removes declarations of now-removed internal allocation helpers. |
| libnvme/src/nvme/nvme-cmds.c | Migrates internal allocations to libnvme_alloc* + cleanup helper. |
| libnvme/src/nvme/mem.h | Adds new public libnvme memory API header. |
| libnvme/src/nvme/mem-win.c | Adds Windows implementation of the new libnvme memory APIs. |
| libnvme/src/nvme/mem-linux.c | Adds Linux implementation of the new libnvme memory APIs. |
| libnvme/src/nvme/fabrics.c | Migrates internal allocations to libnvme_alloc / libnvme_free + cleanup helper. |
| libnvme/src/nvme/cleanup.h | Adds __cleanup_libnvme_free helper in libnvme. |
| libnvme/src/meson.build | Adds platform-specific mem source files and installs the new header. |
| libnvme/src/libnvme.ld | Exports the new allocation symbols and reorders/adjusts some existing exports. |
| libnvme/src/libnvme.h.in | Exposes <nvme/mem.h> in the generated public libnvme umbrella header. |
Comments suppressed due to low confidence (4)
libnvme/src/nvme/mem-linux.c:13
mem-linux.ccallsmemset()/memcpy()but no longer includes<string.h>, which will break builds on compilers that treat implicit declarations as errors. Add<string.h>back to this file's includes.
libnvme/src/nvme/mem-linux.c:28posix_memalign()expects avoid **for its first parameter, but this passes(void *)&p(avoid *). This is an incompatible argument type and can fail compilation with-Werror/ lead to UB. Pass&p(or(void **)&p) instead.
libnvme/src/nvme/mem-linux.c:38libnvme_realloc()unconditionally callsmalloc_usable_size(p)even whenp == NULL, which is undefined and contradicts the header contract that NULL should behave likelibnvme_alloc(). Add an earlyif (!p) return libnvme_alloc(len);before callingmalloc_usable_size().
nvme.c:2902- Allocation failure check is inverted:
ctrlis non-NULL on success, soif (ctrl) return false;makes this function always return false (and never issues Identify). This should beif (!ctrl) return false;.
__cleanup_libnvme_free struct nvme_id_ctrl *ctrl = libnvme_alloc(sizeof(*ctrl));
if (ctrl)
return false;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add Windows implementations of libnvme_alloc, libnvme_realloc, and related memory allocation helpers. This provides a consistent cross-platform memory allocation interface for libnvme on Windows systems. Signed-off-by: Broc Going <bgoing@micron.com> Signed-off-by: Brandon Capener <bcapener@micron.com> [wagi: cherry-picked code, fixed libnvme_realloc] Signed-off-by: Daniel Wagner <wagi@kernel.org>
The library added platform abstraction code for memory allocation. Switch over to this API and remove the duplicated code in nvme-cli. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Move these helpers to the library, so it's abstraction code for the mem API are in one place. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Add Windows support for the huge memory allocation APIs by implementing libnvme_alloc_huge() and libnvme_free_huge() using Windows-specific memory allocation mechanisms. This brings the huge page memory API support closer to feature parity across supported platforms. Signed-off-by: Broc Going <bgoing@micron.com> Signed-off-by: Brandon Capener <bcapener@micron.com> [wagi: cherry picked code] Signed-off-by: Daniel Wagner <wagi@kernel.org>
Introduce libnvme_free as the common deallocation API for memory allocated via libnvme_alloc() and libnvme_realloc(). This completes the basic memory management interface by providing a consistent wrapper for freeing libnvme-managed allocations. Signed-off-by: Daniel Wagner <wagi@kernel.org>
In case reallocarray is not available provide an open-coded version. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Use the library alloc function which ensures that alllocation are propaerly aligned. This avoids a direct dependency on posix_memalign. Signed-off-by: Daniel Wagner <wagi@kernel.org>
|
@bgoing-micron-oss would you mind to review this here? I splitted the memory API changes into this single PR. I think I got most of it covered. I've prepare another PR based on your #3310. So far I covered dprintf and getline (#3355). And then I want to tackle the signal API. |
|
okay let's go with those changes and see what I missed :) |
Refactore the memory APIs so that it's possible to provide versions for Windows. This reduces the code duplication between between libnvme and nvme-cli but means there are a handful new exported functions. But given this is tricky code it might makes sense to provide this for all users anyway.