Repository navigation
Port to libnvme3 - #1213
Open
igaw wants to merge 4 commits into
Open
Port to libnvme3#1213igaw wants to merge 4 commits into
igaw wants to merge 4 commits into
Conversation
nvme-cli 3.0 merged libnvme into its tree and ships it as the new libnvme3 library with a reworked API, libnvme-1.x is no longer developed. Devices are now accessed through transport handles, commands are built with the nvme_init_*() helpers and errors are returned as negative errno values. libnvme3 dropped support for the JSON configuration file, the "config" extra argument is ignored for now. Following the DH-HMAC-CHAP to KX-HMAC-CHAP rename in nvme-cli, the "dhchap_key" and "dhchap_ctrl_key" extra arguments are now called "kxchap_secret" and "kxchap_ctrl_secret". The old names are kept as deprecated aliases. Note that libnvme3 refuses a second identical connection to an already connected controller unless "duplicate_connect" is set. The tests now look up controllers and namespaces in sysfs as the 'nvme list' JSON output changed in nvme-cli 3.0. Resolves: storaged-project#1211 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
libnvme3 replaced the JSON configuration with an INI based /etc/nvme/nvme-fabrics.conf. Parameters of the first connection entry matching the bd_nvme_connect() arguments are applied, extra arguments still take precedence. Legacy JSON configuration files are not supported anymore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Command dwords of passthru commands are passed in CPU byte order, the kernel takes care of the conversion. Converting the pattern to little endian beforehand swapped it twice on big endian architectures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
libnvme3 keeps track of the owner of each connected NVMe-oF controller, e.g. boot volumes connected from the NBFT are owned by "nbft". Previously bd_nvme_disconnect() removed all matching controllers, including the ones other components depend on. Controllers connected by bd_nvme_connect() are now registered as owned by "libblockdev" (or by the new "owner" extra argument) and bd_nvme_disconnect() and bd_nvme_disconnect_by_path() only remove controllers owned by libblockdev. An already connected controller is never claimed. The new bd_nvme_disconnect_full() function allows to disconnect controllers of a different owner or, with BD_NVME_DISCONNECT_FORCE, regardless of their owner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The NVMe plugin is ported to libnvme3 with owner-aware disconnect support. The review found no concrete defect that should block merging. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 59.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 8 files. (3 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main change: porting the NVMe plugin to libnvme3. Linked Issues check Issue [ #1211] requires libblockdev to build with nvme-cli 3.0's libnvme3. The change updatesconfigure.acto requirelibnvme3 >= 1.0and ports NVMe implementation code to libnvme3 APIs. The change…Out of Scope Changes check The changes stay within the libnvme3 port requested by [ #1211]. Configuration, transport, command handling, ownership behavior, public API documentation, and supporting tests all implement or validate…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 59.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 8 files. (3 skipped: 3 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Author
|
Depends on linux-nvme/nvme-cli#4157 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nvme-cli 3.0 merged libnvme into its tree and ships it as the new libnvme3 library (libnvme3.so.1) with a reworked API. libnvme-1.x is no longer developed, so this series ports the NVMe plugin over.
nvme_init_*()helpers and connecting useslibnvmf_connect(). The"dhchap_key"and"dhchap_ctrl_key"extra arguments are renamed to"kxchap_secret"and"kxchap_ctrl_secret"following the KX-HMAC-CHAP rename in nvme-cli, the old names are kept as deprecated aliases. The test helpers now read sysfs as thenvme listJSON output changed."config"extra argument now reads the new INI based/etc/nvme/nvme-fabrics.conf. Legacy JSON files are no longer supported (nvme-cli converts them withnvme config-convert).bd_nvme_connect()are registered as owned by"libblockdev"(or the new"owner"extra argument) in the libnvme ownership registry.bd_nvme_disconnect()andbd_nvme_disconnect_by_path()only remove controllers owned by libblockdev, so e.g. NBFT boot volumes are left alone. The newbd_nvme_disconnect_full()disconnects controllers of another owner or, withBD_NVME_DISCONNECT_FORCE, regardless of the owner.Behaviour changes: a second identical connection now fails with
BD_NVME_ERROR_CONNECT_ALREADYunless"duplicate_connect"is set, andbd_nvme_disconnect()no longer removes controllers connected by others (BD_NVME_ERROR_OWNED).Tested on Debian 12 (kernel 6.12) with nvme-cli 3.1 and the nvmet loop target: all 18 NVMe tests pass. The duplicate-connect test needs the libnvme topology scan fix from linux-nvme/nvme-cli (to be linked).
Fixes: #1211
Summary by CodeRabbit