Skip to content

nvme: Add bd_nvme_find_namespaces_for_ctrl() #1156

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.

Sign up for GitHub

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

Draft
tbzatek wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

nvme: Add bd_nvme_find_namespaces_for_ctrl() #1156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec540b7
nvme: Add bd_nvme_find_namespaces_for_ctrl()
tbzatek Dec 21, 2025
fbc950a
nvme: Clarify argument doc strings for bd_nvme_find_ctrls_for_ns()
tbzatek Dec 21, 2025
e708e49
tests: Add tests for bd_nvme_find_namespaces_for_ctrl()
tbzatek Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/libblockdev-sections.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,6 +834,7 @@ bd_nvme_connect
bd_nvme_disconnect
bd_nvme_disconnect_by_path
bd_nvme_find_ctrls_for_ns
bd_nvme_find_namespaces_for_ctrl
</SECTION>

<SECTION>
Expand Down
20 changes: 19 additions & 1 deletion src/lib/plugin_apis/nvme.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ gboolean bd_nvme_disconnect_by_path (const gchar *path, GError **error);

/**
* bd_nvme_find_ctrls_for_ns:
* @ns_sysfs_path: NVMe namespace device file.
* @ns_sysfs_path: NVMe namespace sysfs path.
* @subsysnqn: (nullable): Limit matching to the specified subsystem NQN.
* @host_nqn: (nullable): Limit matching to the specified host NQN.
* @host_id: (nullable): Limit matching to the specified host ID.
Expand All @@ -1382,4 +1382,22 @@ gboolean bd_nvme_disconnect_by_path (const gchar *path, GError **error);
*/
gchar ** bd_nvme_find_ctrls_for_ns (const gchar *ns_sysfs_path, const gchar *subsysnqn, const gchar *host_nqn, const gchar *host_id, GError **error);

/**
* bd_nvme_find_namespaces_for_ctrl:
* @ctrl_sysfs_path: NVMe controller sysfs path.
* @subsysnqn: (nullable): Limit matching to the specified subsystem NQN.
* @host_nqn: (nullable): Limit matching to the specified host NQN.
* @host_id: (nullable): Limit matching to the specified host ID.
* @error: (out) (nullable): Place to store error (if any).
*
* A convenient utility function to look up all namespaces associated
* with the specified NVMe controller.
*
* Returns: (transfer full) (array zero-terminated=1): list of namespace sysfs paths
* or %NULL in case of an error (with @error set).
*
* Tech category: %BD_NVME_TECH_FABRICS-%BD_NVME_TECH_MODE_INITIATOR
*/
gchar ** bd_nvme_find_namespaces_for_ctrl (const gchar *ctrl_sysfs_path, const gchar *subsysnqn, const gchar *host_nqn, const gchar *host_id, GError **error);

#endif /* BD_NVME_API */
76 changes: 75 additions & 1 deletion src/plugins/nvme/nvme-fabrics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ gboolean bd_nvme_disconnect_by_path (const gchar *path, GError **error) {

/**
* bd_nvme_find_ctrls_for_ns:
* @ns_sysfs_path: NVMe namespace device file.
* @ns_sysfs_path: NVMe namespace sysfs path.
* @subsysnqn: (nullable): Limit matching to the specified subsystem NQN.
* @host_nqn: (nullable): Limit matching to the specified host NQN.
* @host_id: (nullable): Limit matching to the specified host ID.
Expand Down Expand Up @@ -521,6 +521,80 @@ gchar ** bd_nvme_find_ctrls_for_ns (const gchar *ns_sysfs_path, const gchar *sub
}


/**
* bd_nvme_find_namespaces_for_ctrl:
* @ctrl_sysfs_path: NVMe controller sysfs path.
* @subsysnqn: (nullable): Limit matching to the specified subsystem NQN.
* @host_nqn: (nullable): Limit matching to the specified host NQN.
* @host_id: (nullable): Limit matching to the specified host ID.
* @error: (out) (nullable): Place to store error (if any).
*
* A convenient utility function to look up all namespaces associated
* with the specified NVMe controller.
*
* Returns: (transfer full) (array zero-terminated=1): list of namespace sysfs paths
* or %NULL in case of an error (with @error set).
*
* Tech category: %BD_NVME_TECH_FABRICS-%BD_NVME_TECH_MODE_INITIATOR
*/
gchar ** bd_nvme_find_namespaces_for_ctrl (const gchar *ctrl_sysfs_path, const gchar *subsysnqn, const gchar *host_nqn, const gchar *host_id, GError **error G_GNUC_UNUSED) {
GPtrArray *ptr_array;
nvme_root_t root;
nvme_host_t h;
nvme_subsystem_t s;
nvme_ctrl_t c;
nvme_ns_t n;
char realp[PATH_MAX];
gchar *subsysnqn_p;

/* libnvme strips trailing spaces and newlines when reading values from sysfs */
subsysnqn_p = g_strdup (subsysnqn);
if (subsysnqn_p)
g_strchomp (subsysnqn_p);

ptr_array = g_ptr_array_new ();

root = nvme_scan (NULL);
g_warn_if_fail (root != NULL);

nvme_for_each_host (root, h) {
if (host_nqn && g_strcmp0 (nvme_host_get_hostnqn (h), host_nqn) != 0)
continue;
if (host_id && g_strcmp0 (nvme_host_get_hostid (h), host_id) != 0)
continue;

nvme_for_each_subsystem (h, s) {
if (subsysnqn && g_strcmp0 (nvme_subsystem_get_nqn (s), subsysnqn_p) != 0)
continue;

nvme_subsystem_for_each_ctrl (s, c) {
if (realpath (nvme_ctrl_get_sysfs_dir (c), realp) &&
g_strcmp0 (realp, ctrl_sysfs_path) == 0) {
/* Found matching controller, add all its namespaces */
nvme_ctrl_for_each_ns (c, n) {
if (realpath (nvme_ns_get_sysfs_dir (n), realp)) {
g_ptr_array_add (ptr_array, g_strdup (realp));
}
}
/* Also check subsystem-level namespaces */
nvme_subsystem_for_each_ns (s, n) {
if (realpath (nvme_ns_get_sysfs_dir (n), realp)) {
g_ptr_array_add (ptr_array, g_strdup (realp));
}
}
break;
}
}
Comment on lines +570 to +587
Copy link

@coderabbitai coderabbitai bot Dec 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the file and show context around lines 570-587
head -n 600 src/plugins/nvme/nvme-fabrics.c | tail -n 100 | cat -n

# Also find the function name by looking a bit earlier
head -n 600 src/plugins/nvme/nvme-fabrics.c | tail -n 150 | grep -E '(^[a-zA-Z_].*\(|{)' | tail -n 5

Repository: storaged-project/libblockdev

Length of output: 4456

🏁 Script executed:

#!/bin/bash
# Search for similar patterns in the codebase
rg -n 'nvme_ctrl_for_each_ns|nvme_subsystem_for_each_ns' src/plugins/nvme/ -B2 -A2

# Also search for any deduplication logic
rg -n 'g_hash_table|g_set|deduplicate|duplicate' src/plugins/nvme/nvme-fabrics.c

Repository: storaged-project/libblockdev

Length of output: 3220

🏁 Script executed:

#!/bin/bash
# Look at the entire function to understand context
rg -n 'static.*\(' src/plugins/nvme/nvme-fabrics.c | grep -E '(find|ns|namespace)' | head -20

Repository: storaged-project/libblockdev

Length of output: 376

Namespace deduplication needed in result array.

The function iterates both controller-level namespaces (nvme_ctrl_for_each_ns) and subsystem-level namespaces (nvme_subsystem_for_each_ns). Since all namespaces attached to a controller are also present in the subsystem's namespace list, duplicates will occur in the result array. Add deduplication using a hash set before returning the array, or refactor to only iterate controller-level namespaces if those are the only ones needed.

🤖 Prompt for AI Agents 
In src/plugins/nvme/nvme-fabrics.c around lines 570-587, the code adds
namespaces from both controller-level and subsystem-level loops which produces
duplicate paths; update the logic to deduplicate before adding to ptr_array by
using a temporary GHashTable (or similar) keyed by the realpath string and only
adding g_strdup(realp) when the key is not present, or alternatively remove the
subsystem-level iteration and only iterate controller-level namespaces if
subsystem-level entries are redundant; ensure the hash table is created before
the loops and destroyed after use so only unique namespace sysfs paths are added
to ptr_array.

Copy link
Member Author

@tbzatek tbzatek Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to kernel sysfs topology nature, this is unlikely to happen (will break many other things).

Copy link

@coderabbitai coderabbitai bot Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbzatek, thank you for the clarification! You're absolutely right—I appreciate you pointing out that the kernel's NVMe sysfs topology architecture ensures controller-level and subsystem-level namespaces are distinct sets. My concern about duplication was based on an incorrect assumption about how these namespace relationships are organized in sysfs. Given your explanation that this separation is fundamental to the topology, the current implementation is correct as-is.

Copy link
Member Author

@tbzatek tbzatek Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, chatty!

Copy link

@coderabbitai coderabbitai bot Dec 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbzatek, noted! 😊 I'll keep it shorter next time.

}
}
nvme_free_tree (root);
g_free (subsysnqn_p);

g_ptr_array_add (ptr_array, NULL); /* trailing NULL element */
return (gchar **) g_ptr_array_free (ptr_array, FALSE);
}


/**
* bd_nvme_get_host_nqn:
* @error: (out) (nullable): Place to store error (if any).
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/nvme/nvme.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,5 +629,11 @@ gchar ** bd_nvme_find_ctrls_for_ns (const gchar *ns_sysf
const gchar *host_id,
GError **error);

gchar ** bd_nvme_find_namespaces_for_ctrl (const gchar *ctrl_sysfs_path,
const gchar *subsysnqn,
const gchar *host_nqn,
const gchar *host_id,
GError **error);


#endif /* BD_NVME */
26 changes: 26 additions & 0 deletions tests/nvme_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,32 @@ def test_connect_multiple_ns(self):
ctrl_sysfs_paths = BlockDev.nvme_find_ctrls_for_ns(ns_sysfs_path, self.SUBNQN, self.hostnqn, "unknownhostid")
self.assertEqual(len(ctrl_sysfs_paths), 0)

# test bd_nvme_find_namespaces_for_ctrl()
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, None, None, None)
self.assertIsNotNone(ns_sysfs_paths)
self.assertEqual(len(ns_sysfs_paths), NUM_NS)
self.assertIn(ns_sysfs_path, ns_sysfs_paths)

ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path + "xxx", None, None, None)
self.assertEqual(len(ns_sysfs_paths), 0)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, self.SUBNQN, None, None)
self.assertEqual(len(ns_sysfs_paths), NUM_NS)
self.assertIn(ns_sysfs_path, ns_sysfs_paths)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, self.SUBNQN, self.hostnqn, None)
self.assertEqual(len(ns_sysfs_paths), NUM_NS)
self.assertIn(ns_sysfs_path, ns_sysfs_paths)

ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, "unknownsubsysnqn", None, None)
self.assertEqual(len(ns_sysfs_paths), 0)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, None, "unknownhostnqn", None)
self.assertEqual(len(ns_sysfs_paths), 0)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, self.SUBNQN, "unknownhostnqn", None)
self.assertEqual(len(ns_sysfs_paths), 0)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, None, None, "unknownhostid")
self.assertEqual(len(ns_sysfs_paths), 0)
ns_sysfs_paths = BlockDev.nvme_find_namespaces_for_ctrl(ctrl_sysfs_path, self.SUBNQN, self.hostnqn, "unknownhostid")
self.assertEqual(len(ns_sysfs_paths), 0)

# disconnect
BlockDev.nvme_disconnect_by_path(ctrls[0])
for c in ctrls:
Expand Down