-
Notifications
You must be signed in to change notification settings - Fork 697
libnvme and nvme-cli 3.x API update #2955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d497970 to
4b4b38d
Compare
Signed-off-by: Daniel Wagner <[email protected]>
4b4b38d to
aa7e0f1
Compare
The project defaults set the force-fallback-for libnvme. Disable this for the distro build, where the library is build/installed separately. Signed-off-by: Daniel Wagner <[email protected]>
Merge all existing symbols into a new 2.0 section. Signed-off-by: Daniel Wagner <[email protected]>
Get rid of deprecated functions. Signed-off-by: Daniel Wagner <[email protected]>
libnvme-mi was added after the initial 1.x release and it was decided to ship it independ of libnvme. With the upcoming 2.x work, merge the two libraries so it's possible to use common code, that is unify the APIs. Signed-off-by: Daniel Wagner <[email protected]>
aa7e0f1 to
1bc0e35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates libnvme to support the nvme-cli 3.x API, involving a major refactoring of the library's architecture. The changes replace the nvme_root_t abstraction with nvme_global_ctx, introduce a new transport handle abstraction to unify direct and MI access, and update error handling to return negative error codes instead of setting errno.
Key Changes:
- Replaced
nvme_root_twithstruct nvme_global_ctxthroughout the codebase - Introduced
nvme_transport_handleabstraction to unify direct device and MI controller access - Changed error handling from errno-based to negative error code returns
- Updated constant names (e.g.,
NVME_LOG_LID_DISCOVER→NVME_LOG_LID_DISCOVERY)
Reviewed Changes
Copilot reviewed 54 out of 167 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libnvme/test/ioctl/discovery.c | Updates test to use new API with transport handles and error codes |
| libnvme/test/ioctl/ana.c | Updates ANA log tests to use transport handles instead of file descriptors |
| libnvme/test/cpp.cc | Updates C++ test to use new global context API |
| libnvme/test/config/*.c | Updates configuration tests to use nvme_global_ctx instead of nvme_root_t |
| libnvme/src/nvme/util.h | Updates function signatures and removes deprecated function |
| libnvme/src/nvme/util.c | Updates error handling to return negative error codes |
| libnvme/src/nvme/types.h | Adds new types and updates constant names |
| libnvme/src/nvme/tree.h | Major API refactoring replacing nvme_root_t with nvme_global_ctx |
| libnvme/src/nvme/tree.c | Implements tree API changes with new context and transport handle abstractions |
| libnvme/src/nvme/private.h | Updates internal structures to support new transport handle architecture |
| libnvme/src/nvme/no-json.c | Updates JSON stub functions to use new context type |
| libnvme/src/nvme/nbft.c | Updates NBFT parsing to return negative error codes |
| libnvme/src/nvme/mi.h | Updates MI API to use transport handles instead of nvme_mi_ctrl_t |
| libnvme/src/nvme/mi.c | Implements MI changes with transport handle abstraction |
| libnvme/src/nvme/mi-mctp.c | Updates MCTP transport to use new global context |
| libnvme/src/nvme/log.h | Updates logging API to use nvme_global_ctx |
| libnvme/src/nvme/log.c | Implements logging changes for new context type |
| libnvme/scripts/release.sh | Removes separate MI library map file reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The global context will not only be used by the tree API, instead it will be used as default context for the complete API. This addresses the shortcoming that 'global' variables such as the logging infrastructure should not be library global variables. Signed-off-by: Daniel Wagner <[email protected]>
Abstract the file descriptor handle to a more abstract type so it's possible to use different 'backends' to send/receive commands, e.g. via MI instead direct. Signed-off-by: Daniel Wagner <[email protected]>
Since uring not supported for link type mi. Signed-off-by: Tokunori Ikegami <[email protected]>
There is no user left for the file descriptor in the args structs, thus remove it. Signed-off-by: Daniel Wagner <[email protected]>
When the user provides their own implementation of the passtrhu function, it is necessary to use the file descriptor for the ioctl call. Thus allow to retrieve the file descriptor from the transport handle. Signed-off-by: Daniel Wagner <[email protected]>
nvme-cli wants to retrieve the name of the device, add a getter for this. Signed-off-by: Daniel Wagner <[email protected]>
The nvme_transport_handle type is used for block device and char device. User application sometimes need to figure out which type of device is used. Because libnvme already knows the type of device expose this. This avoids tracking of the device type in the user application. Signed-off-by: Daniel Wagner <[email protected]>
struct nvme_transport_handle was introduced to abstract the transport, thus replace nvme_mi_ctrl_t with it. Signed-off-by: Daniel Wagner <[email protected]>
Extend nvme_open and nvme_close to handle MI devices. Signed-off-by: Daniel Wagner <[email protected]>
The struct nvme_transport_handle type is used to abstract the transport type. User application sometimes need to figure out which type of device is used. Because libnvme already knows the type of device expose this. This avoids tracking of the device type in the user application. Signed-off-by: Daniel Wagner <[email protected]>
Hook up the MI backend to the generic commands API. Signed-off-by: Daniel Wagner <[email protected]>
The MI code has an alignment and length check which the direct APIs are missing. Without these checks the download tests for MI will fail after using the generic APIs. Signed-off-by: Daniel Wagner <[email protected]>
With the introduction of the link handle, it's possible to use the generic nvme API. The transport selection happens on lower levels depending on the link object type. Note, the tests rely on using the ioctl interface and not the io_uring. Signed-off-by: Daniel Wagner <[email protected]>
Update to use the new libnvme API. Unfortunately, it's not possible to do step by step updates this change is big. Though the resulting diff is easy to read. Generally, following things are necessary to get things going: - replace struct nvme_dev with nvme_link_t - drop nvme_cli* wrappers - remove MI helper functions - replaced alternative versions of function with latest - remove libnvme-mi dependency - parse_and_open opens root and link object - use consistently cleanup helpers for root and link object - use early returns Signed-off-by: Daniel Wagner <[email protected]> [m.kurz92: - replace 'nvme_root_t r' with 'struct nvme_global_ctx *ctx' - replace 'nvme_link_t l' with 'struct nvme_transport_handle *hdl' ] Signed-off-by: Markus Kurz <[email protected]>
The v1 design used the POSIX way to return errors, return -1 with errno set. Replace this design by returning directly the error code. That means non trivial function have to return any data via a pointer so the function is able to return proper error codes. Signed-off-by: Daniel Wagner <[email protected]>
libnvme returns the error code directly now. Replace indirect errno handling with it. Signed-off-by: Daniel Wagner <[email protected]>
libnvme changed the API for the nvme_lm_cdq command. Update the callsite accordingly. Signed-off-by: Markus Kurz <[email protected]> [wagi: replaced nvme_ini_lm_cdq with nvme_init_lm_cdq_{create|delete}] Signed-off-by: Daniel Wagner <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
libnvme changed the API for the nvme_lm_track_send command. Update the callsite accordingly. Signed-off-by: Markus Kurz <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
libnvme changed the API for the nvme_lm_migration_send command. Update the callsite accordingly. Signed-off-by: Markus Kurz <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
libnvme changed the API for the nvme_lm_migration_recv command. Update the callsite accordingly. Signed-off-by: Markus Kurz <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]>
libnvme changed the API for the nvme_io command. Update the callsite accordingly. Signed-off-by: Tokunori Ikegami <[email protected]>
Replace the struct args approach by providing init function for initializing the passthru commands. This reduces the dependency between callside and library. Signed-off-by: Daniel Wagner <[email protected]> [ikegmi.t: - added switch case break statement missed] Signed-off-by: Tokunori Ikegami <[email protected]>
libnvme changed the API for the nvme_copy command. Update the callsite accordingly. Signed-off-by: Tokunori Ikegami <[email protected]>
There is no user left of this type in ioctl.c. Remove it. Signed-off-by: Daniel Wagner <[email protected]>
There is no need for the api-types.h header anymore. Furthermore, the struct nvme_transport_handle and struct nvme_global_ctx type is used throughout the library and is now part of the main API. We should ensure any user of <nvme/libnvme.h> gets this type. Signed-off-by: Daniel Wagner <[email protected]>
Refactore so we maintain nvme_get_log_partial easier. Signed-off-by: Tokunori Ikegami <[email protected]>
Since the following warnings output. $ ./scripts/kernel-doc-check test/zns.c test/zns.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst test/zns.c:10: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
examples/mi-mctp.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
Since checked by kernel-doc-check then the warning message output. Signed-off-by: Tokunori Ikegami <[email protected]>
To fix the warning message below. test/mi.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst Signed-off-by: Tokunori Ikegami <[email protected]>
As we have renamed nvme_root to nvme_global_ctx, the Python bindings example needs to be adjusted to reflect the refactoring. Signed-off-by: Dennis Maisenbacher <[email protected]>
The error handling has changed from returning -1 and errno set to just returing the error code. Negative values are internal errors, e.g. -ENOMEM. Positive values are status code from the transport. Signed-off-by: Dennis Maisenbacher <[email protected]>
Since the almost nvme-cli caller functions expected the base name. Signed-off-by: Tokunori Ikegami <[email protected]>
nvme_scan creates first a root object and then does a scan. Because the root object is usually already created the parse_and_open helper we don't need to create another object (and leak the first one). Thus call directly nvme_scan_topology. Signed-off-by: Daniel Wagner <[email protected]>
1bc0e35 to
7daa1e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 54 out of 167 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
directly merged |
There patches were developed at https://github.com/nvme-experiments.
167 files changed, 15881 insertions(+), 21482 deletions(-)