Improve unique identifier support to include NVMe devices#557
Open
tasleson wants to merge 6 commits into
Open
Conversation
NVMe devices were discovered by lsm_local_disk_list() and health status worked, but vpd83_get, serial_num_get, link_type_get, and rpm_get all rejected NVMe paths with "only support /dev/sd" errors. NVMe devices don't expose SCSI VPD pages, but the kernel provides native namespace identifiers via sysfs: nguid (128-bit) and eui (64-bit) for disk ID, and device/serial for serial number. For vpd83_get, try nguid first (/sys/block/<name>/nguid), falling back to eui (/sys/block/<name>/eui) if nguid is absent or all zeros. For serial_num_get, read /sys/block/<name>/device/serial. For link_type_get, return LSM_DISK_LINK_TYPE_PCIE. For rpm_get, return LSM_DISK_RPM_NON_ROTATING_MEDIUM. Tested on RHEL 10.1 (hpe-dl560gen11) against /dev/nvme0n1 (EUI fallback path exercised, nguid absent) and /dev/sd[abcd]. Resolves: libstorage#135 Assisted-by: Claude Code (Claude Opus 4.6)
Some controllers (e.g. HPE Smart Array on DL560 Gen11) expose VPD page 83 with only EUI-64 designator type (0x2) and no NAA (0x3). The existing code only matched NAA, causing these disks to fail with "SCSI VPD 83 NAA logical unit ID is not supported". Add a second pass that looks for EUI-64 designators when no NAA is found. NAA remains preferred when both are present. Tested on HPE DL560 Gen11 with /dev/sd[abcd] (EUI-64 only) and /dev/nvme0n1 — all five disks now return valid identifiers. Assisted-by: Claude Code (Claude Opus 4.6)
The Disk constructor reused Volume.vpd83_verify() which only accepts NAA-format identifiers (leading 2/3/5 for 16-char, leading 6 for 32-char). EUI-64 identifiers from VPD page 83 designator type 0x2 don't follow NAA format — they're raw hex with no prefix constraint. Add a separate _disk_regex_vpd83 that accepts any 16, 24, or 32 character hex string (covering EUI-64 8/12/16-byte variants and NAA identifiers). The Volume validator remains strict since volumes should always have NAA-format IDs from array controllers. Assisted-by: Claude Code (Claude Opus 4.6)
The vpd83_get and vpd83_search header docs still promised NAA-only format, but we now return EUI-64 and NVMe NGUID/EUI identifiers too. Update the documented formats to match the actual behavior. The man pages are generated from these header comments. Assisted-by: Claude Code (Claude Opus 4.6)
When disks are members of a RAID array, ssacli no longer reports the "Disk Name" (block device path), so LocalDisk.vpd83_get() cannot be called. Fall back to the controller-reported "Drive Unique ID" field which contains the EUI-64 identifier. Assisted-by: Claude Code (Claude Opus 4.6)
$ make format Signed-off-by: Tony Asleson <tasleson@redhat.com>
📝 WalkthroughWalkthroughThis PR extends libstoragemgmt to recognize and retrieve VPD 0x83 disk identifiers from NVMe devices via sysfs, alongside existing SCSI support. It updates API documentation, introduces NVMe parsing helpers, integrates NVMe branching into device serial and VPD lookup paths, optimizes NVMe device classification, and broadens Python validation to accept multiple identifier formats. ChangesNVMe VPD 0x83 Support Throughout Stack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@c_binding/lsm_local_disk.cpp`:
- Around line 833-836: The NVMe fast-path currently treats any path starting
with "/dev/nvme" as valid; before setting *rpm =
LSM_DISK_RPM_NON_ROTATING_MEDIUM (and similarly before setting link_type=PCIE in
the other block), perform a proper existence check (e.g., stat() or access()) on
disk_path and verify it's a valid device; if the check fails return
LSM_ERR_NOT_FOUND_DISK instead of LSM_ERR_OK. Locate the strncmp(disk_path,
"/dev/nvme", strlen("/dev/nvme")) checks and wrap them with the
existence/device-type validation so only real disks are accepted.
- Around line 331-332: The current conditional that allows any designator_len
between 1 and 16 for the EUI-64 fallback (checking dps[i]->header.designator_len
> 0 && dps[i]->header.designator_len <= 16) is too permissive and can emit
malformed VPD83 strings; change this check to only accept supported EUI
identifier byte lengths (exactly 8, 12, or 16) by replacing the range test with
an explicit check (e.g., dps[i]->header.designator_len == 8 ||
dps[i]->header.designator_len == 12 || dps[i]->header.designator_len == 16) so
that the fallback path in the code that formats the EUI only runs for valid
descriptor sizes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aefdd28a-bda2-4ec6-ad6f-1e4b28a53f4a
📒 Files selected for processing (8)
INSTALLc_binding/include/libstoragemgmt/libstoragemgmt_local_disk.hc_binding/libsg.hc_binding/lsm_local_disk.cppc_binding/lsm_mgmt.cppplugin/arcconf_plugin/arcconf.pyplugin/hpsa_plugin/hpsa.pypython_binding/lsm/_data.py
💤 Files with no reviewable changes (1)
- plugin/arcconf_plugin/arcconf.py
Comment on lines
+331
to
+332
| (dps[i]->header.designator_len > 0) && | ||
| (dps[i]->header.designator_len <= 16)) { |
There was a problem hiding this comment.
Restrict EUI-64 fallback lengths to valid descriptor sizes.
Line 331 and Line 332 currently accept any designator_len from 1 to 16. EUI identifiers should be constrained to supported byte lengths (8, 12, or 16), otherwise malformed VPD83 strings can be emitted.
Proposed fix
- (dps[i]->header.designator_len > 0) &&
- (dps[i]->header.designator_len <= 16)) {
+ ((dps[i]->header.designator_len == 8) ||
+ (dps[i]->header.designator_len == 12) ||
+ (dps[i]->header.designator_len == 16))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@c_binding/lsm_local_disk.cpp` around lines 331 - 332, The current conditional
that allows any designator_len between 1 and 16 for the EUI-64 fallback
(checking dps[i]->header.designator_len > 0 && dps[i]->header.designator_len <=
16) is too permissive and can emit malformed VPD83 strings; change this check to
only accept supported EUI identifier byte lengths (exactly 8, 12, or 16) by
replacing the range test with an explicit check (e.g.,
dps[i]->header.designator_len == 8 || dps[i]->header.designator_len == 12 ||
dps[i]->header.designator_len == 16) so that the fallback path in the code that
formats the EUI only runs for valid descriptor sizes.
Comment on lines
+833
to
+836
| if (strncmp(disk_path, "/dev/nvme", strlen("/dev/nvme")) == 0) { | ||
| *rpm = LSM_DISK_RPM_NON_ROTATING_MEDIUM; | ||
| goto out; | ||
| } |
There was a problem hiding this comment.
Validate disk existence before NVMe fast-path success.
Line 833 and Line 1099 return LSM_ERR_OK purely from /dev/nvme prefix matching. A nonexistent path like /dev/nvme999n999 can be reported as valid (rpm=non-rotating / link_type=PCIE) instead of LSM_ERR_NOT_FOUND_DISK.
Proposed fix
int lsm_local_disk_rpm_get(const char *disk_path, int32_t *rpm,
lsm_error **lsm_err) {
@@
_lsm_err_msg_clear(err_msg);
*lsm_err = NULL;
+ if (!_file_exists(disk_path)) {
+ rc = LSM_ERR_NOT_FOUND_DISK;
+ _lsm_err_msg_set(err_msg, "Disk %s not found", disk_path);
+ goto out;
+ }
+
if (strncmp(disk_path, "/dev/nvme", strlen("/dev/nvme")) == 0) {
*rpm = LSM_DISK_RPM_NON_ROTATING_MEDIUM;
goto out;
@@
int lsm_local_disk_link_type_get(const char *disk_path,
lsm_disk_link_type *link_type,
lsm_error **lsm_err) {
@@
*link_type = LSM_DISK_LINK_TYPE_NO_SUPPORT;
*lsm_err = NULL;
+ if (!_file_exists(disk_path)) {
+ rc = LSM_ERR_NOT_FOUND_DISK;
+ _lsm_err_msg_set(err_msg, "Disk %s not found", disk_path);
+ goto out;
+ }
+
if (strncmp(disk_path, "/dev/nvme", strlen("/dev/nvme")) == 0) {
*link_type = LSM_DISK_LINK_TYPE_PCIE;
goto out;
}Also applies to: 1099-1102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@c_binding/lsm_local_disk.cpp` around lines 833 - 836, The NVMe fast-path
currently treats any path starting with "/dev/nvme" as valid; before setting
*rpm = LSM_DISK_RPM_NON_ROTATING_MEDIUM (and similarly before setting
link_type=PCIE in the other block), perform a proper existence check (e.g.,
stat() or access()) on disk_path and verify it's a valid device; if the check
fails return LSM_ERR_NOT_FOUND_DISK instead of LSM_ERR_OK. Locate the
strncmp(disk_path, "/dev/nvme", strlen("/dev/nvme")) checks and wrap them with
the existence/device-type validation so only real disks are accepted.
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.
#135 discussed the lack of NVMe support. This pull request addresses this need.
Additionally, a small update to hpsa plugin was done, so that we could have this information available when it's not available once the storage devices are part of a RAID group.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation