Skip to content

nvme: Default to well-known tr_svcid values when not specified #1128

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

Merged
vojtechtrefny merged 2 commits into from
Sep 24, 2025
Merged

nvme: Default to well-known tr_svcid values when not specified #1128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75da9d7
nvme: Default to well-known tr_svcid values when not specified
tbzatek Sep 24, 2025
cb9fc6c
nvme: Handle memory allocation failures from _nvme_alloc()
tbzatek Sep 24, 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
2 changes: 1 addition & 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 @@ -1270,7 +1270,7 @@ gboolean bd_nvme_set_host_id (const gchar *host_id, GError **error);
* @subsysnqn: The name for the NVMe subsystem to connect to.
* @transport: The network fabric used for a NVMe-over-Fabrics network.
* @transport_addr: (nullable): The network address of the Controller. For transports using IP addressing (e.g. `rdma`) this should be an IP-based address.
* @transport_svcid: (nullable): The transport service id. For transports using IP addressing (e.g. `rdma`) this field is the port number. By default, the IP port number for the `RDMA` transport is `4420`.
* @transport_svcid: (nullable): The transport service ID. For transports using IP addressing (e.g. `tcp`, `rdma`) this field is the port number. The default port number for the `tcp` and `rdma` transports is `4420` and `8009` respectively when the well-known Discovery NQN is specified.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Sep 24, 2025

Choose a reason for hiding this comment

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

medium

The updated documentation for @transport_svcid is a bit ambiguous. The use of 'respectively' could be misinterpreted to mean port 4420 for tcp and 8009 for rdma. A clearer phrasing would explicitly state which port is for I/O controllers and which is for discovery controllers.

 * @transport_svcid: (nullable): The transport service ID.  For transports using IP addressing (e.g. `tcp`, `rdma`) this field is the port number. For `tcp` and `rdma` transports, this defaults to port `4420` for I/O controllers and `8009` for discovery controllers.

Copy link
Copy Markdown
Member Author

@tbzatek tbzatek Sep 24, 2025

Choose a reason for hiding this comment

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

I know, that's what I do.

* @host_traddr: (nullable): The network address used on the host to connect to the Controller. For TCP, this sets the source address on the socket.
* @host_iface: (nullable): The network interface used on the host to connect to the Controller (e.g. IP `eth1`, `enp2s0`). This forces the connection to be made on a specific interface instead of letting the system decide.
* @host_nqn: (nullable): Overrides the default Host NQN that identifies the NVMe Host. If this option is %NULL, the default is read from `/etc/nvme/hostnqn` first.
Expand Down
14 changes: 13 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 @@ -156,7 +156,7 @@ static void parse_extra_args (const BDExtraArg **extra, struct nvme_fabrics_conf
* @subsysnqn: The name for the NVMe subsystem to connect to.
* @transport: The network fabric used for a NVMe-over-Fabrics network.
* @transport_addr: (nullable): The network address of the Controller. For transports using IP addressing (e.g. `rdma`) this should be an IP-based address.
* @transport_svcid: (nullable): The transport service id. For transports using IP addressing (e.g. `rdma`) this field is the port number. By default, the IP port number for the `RDMA` transport is `4420`.
* @transport_svcid: (nullable): The transport service ID. For transports using IP addressing (e.g. `tcp`, `rdma`) this field is the port number. The default port number for the `tcp` and `rdma` transports is `4420` and `8009` respectively when the well-known Discovery NQN is specified.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Sep 24, 2025

Choose a reason for hiding this comment

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

medium

The updated documentation for @transport_svcid is a bit ambiguous. The use of 'respectively' could be misinterpreted to mean port 4420 for tcp and 8009 for rdma. A clearer phrasing would explicitly state which port is for I/O controllers and which is for discovery controllers. This should be consistent with the documentation in nvme.api.

 * @transport_svcid: (nullable): The transport service ID.  For transports using IP addressing (e.g. `tcp`, `rdma`) this field is the port number. For `tcp` and `rdma` transports, this defaults to port `4420` for I/O controllers and `8009` for discovery controllers.

Copy link
Copy Markdown
Member Author

@tbzatek tbzatek Sep 24, 2025

Choose a reason for hiding this comment

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

Nope.

* @host_traddr: (nullable): The network address used on the host to connect to the Controller. For TCP, this sets the source address on the socket.
* @host_iface: (nullable): The network interface used on the host to connect to the Controller (e.g. IP `eth1`, `enp2s0`). This forces the connection to be made on a specific interface instead of letting the system decide.
* @host_nqn: (nullable): Overrides the default Host NQN that identifies the NVMe Host. If this option is %NULL, the default is read from `/etc/nvme/hostnqn` first.
Expand Down Expand Up @@ -300,6 +300,18 @@ gboolean bd_nvme_connect (const gchar *subsysnqn, const gchar *transport, const
if (hostsymname)
nvme_host_set_hostsymname (host, hostsymname);

/* tr_svcid defaults */
if (!transport_svcid) {
if (g_strcmp0 (transport, "tcp") == 0) {
if (g_strcmp0 (subsysnqn, NVME_DISC_SUBSYS_NAME) == 0)
transport_svcid = G_STRINGIFY (NVME_DISC_IP_PORT);
else
transport_svcid = G_STRINGIFY (NVME_RDMA_IP_PORT);
} else
if (g_strcmp0(transport, "rdma") == 0)
transport_svcid = G_STRINGIFY (NVME_RDMA_IP_PORT);
Comment on lines +305 to +312
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Sep 24, 2025

Choose a reason for hiding this comment

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

high

The current implementation for setting the default transport_svcid doesn't seem to match the intent described in the pull request description for the rdma transport. The description states that for both tcp and rdma, the port should be 8009 for discovery controllers. However, the code only implements this for tcp and always defaults to 4420 for rdma.

Additionally, the code structure can be simplified to avoid repetition and improve clarity. The else if chain is also formatted in a confusing way.

I suggest refactoring this block to correctly handle both tcp and rdma transports consistently, which also makes the code more readable and maintainable.

        if (g_strcmp0(transport, "tcp") == 0 || g_strcmp0(transport, "rdma") == 0) {
            if (g_strcmp0(subsysnqn, NVME_DISC_SUBSYS_NAME) == 0)
                transport_svcid = G_STRINGIFY(NVME_DISC_IP_PORT);
            else
                transport_svcid = G_STRINGIFY(NVME_RDMA_IP_PORT);
        }

Copy link
Copy Markdown
Member Author

@tbzatek tbzatek Sep 24, 2025

Choose a reason for hiding this comment

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

Nope.

}

ctrl = nvme_create_ctrl (root, subsysnqn, transport, transport_addr, host_traddr, host_iface, transport_svcid);
if (ctrl == NULL) {
_nvme_fabrics_errno_to_gerror (-1, errno, error);
Expand Down
82 changes: 56 additions & 26 deletions src/plugins/nvme/nvme-info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,13 +405,16 @@ gint _open_dev (const gchar *device, GError **error) {
/* backported from nvme-cli: https://github.com/linux-nvme/nvme-cli/pull/2051 */
#define ROUND_UP(N, S) ((((N) + (S) - 1) / (S)) * (S))

void *_nvme_alloc (size_t len)
void *_nvme_alloc (size_t len, GError **error)
{
size_t _len = ROUND_UP (len, 0x1000);
void *p;

if (posix_memalign ((void *) &p, getpagesize (), _len))
if (posix_memalign ((void *) &p, getpagesize (), _len)) {
g_set_error (error, BD_NVME_ERROR, BD_NVME_ERROR_FAILED,
"Memory allocation failed: %m");
return NULL;
}

memset (p, 0, _len);
return p;
Expand Down Expand Up @@ -475,8 +478,9 @@ BDNVMEControllerInfo * bd_nvme_get_controller_info (const gchar *device, GError
if (fd < 0)
return NULL;

ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl));
g_warn_if_fail (ctrl_id != NULL);
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl), error);
if (!ctrl_id)
return NULL;
/* send the NVME_IDENTIFY_CNS_CTRL ioctl */
ret = nvme_identify_ctrl (fd, ctrl_id);
if (ret != 0) {
Expand Down Expand Up @@ -616,8 +620,11 @@ BDNVMENamespaceInfo *bd_nvme_get_namespace_info (const gchar *device, GError **e
}

/* send the NVME_IDENTIFY_CNS_NS ioctl */
ns_info = _nvme_alloc (sizeof (struct nvme_id_ns));
g_warn_if_fail (ns_info != NULL);
ns_info = _nvme_alloc (sizeof (struct nvme_id_ns), error);
if (!ns_info) {
close (fd);
return NULL;
}
ret = nvme_identify_ns (fd, nsid, ns_info);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand All @@ -628,22 +635,26 @@ BDNVMENamespaceInfo *bd_nvme_get_namespace_info (const gchar *device, GError **e
}

/* send the NVME_IDENTIFY_CNS_CTRL ioctl */
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl));
g_warn_if_fail (ctrl_id != NULL);
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl), error);
if (!ctrl_id) {
close (fd);
free (ns_info);
return NULL;
}
ret_ctrl = nvme_identify_ctrl (fd, ctrl_id);

/* send the NVME_IDENTIFY_CNS_NS_DESC_LIST ioctl, NVMe 1.3 */
if (ret_ctrl == 0 && GUINT32_FROM_LE (ctrl_id->ver) >= 0x10300) {
descs = _nvme_alloc (NVME_IDENTIFY_DATA_SIZE);
g_warn_if_fail (descs != NULL);
ret_desc = nvme_identify_ns_descs (fd, nsid, descs);
descs = _nvme_alloc (NVME_IDENTIFY_DATA_SIZE, NULL);
if (descs != NULL)
ret_desc = nvme_identify_ns_descs (fd, nsid, descs);
}

/* send the NVME_IDENTIFY_CNS_CSI_INDEPENDENT_ID_NS ioctl, NVMe 2.0 */
if (ret_ctrl == 0 && GUINT32_FROM_LE (ctrl_id->ver) >= 0x20000) {
ns_info_ind = _nvme_alloc (sizeof (struct nvme_id_independent_id_ns));
g_warn_if_fail (ns_info_ind != NULL);
ret_ns_ind = nvme_identify_independent_identify_ns (fd, nsid, ns_info_ind);
ns_info_ind = _nvme_alloc (sizeof (struct nvme_id_independent_id_ns), NULL);
if (ns_info_ind != NULL)
ret_ns_ind = nvme_identify_independent_identify_ns (fd, nsid, ns_info_ind);
}
close (fd);

Expand Down Expand Up @@ -762,8 +773,11 @@ BDNVMESmartLog * bd_nvme_get_smart_log (const gchar *device, GError **error) {
return NULL;

/* send the NVME_IDENTIFY_CNS_CTRL ioctl */
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl));
g_warn_if_fail (ctrl_id != NULL);
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl), error);
if (!ctrl_id) {
close (fd);
return NULL;
}
ret_identify = nvme_identify_ctrl (fd, ctrl_id);
if (ret_identify != 0) {
_nvme_status_to_error (ret_identify, FALSE, error);
Expand All @@ -774,8 +788,12 @@ BDNVMESmartLog * bd_nvme_get_smart_log (const gchar *device, GError **error) {
}

/* send the NVME_LOG_LID_SMART ioctl */
smart_log = _nvme_alloc (sizeof (struct nvme_smart_log));
g_warn_if_fail (smart_log != NULL);
smart_log = _nvme_alloc (sizeof (struct nvme_smart_log), error);
if (!smart_log) {
close (fd);
free (ctrl_id);
return NULL;
}
ret = nvme_get_log_smart (fd, NVME_NSID_ALL, FALSE /* rae */, smart_log);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down Expand Up @@ -867,8 +885,11 @@ BDNVMEErrorLogEntry ** bd_nvme_get_error_log_entries (const gchar *device, GErro
return NULL;

/* find out the maximum number of error log entries as reported by the controller */
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl));
g_warn_if_fail (ctrl_id != NULL);
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl), error);
if (!ctrl_id) {
close (fd);
return NULL;
}
ret = nvme_identify_ctrl (fd, ctrl_id);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand All @@ -882,8 +903,11 @@ BDNVMEErrorLogEntry ** bd_nvme_get_error_log_entries (const gchar *device, GErro
free (ctrl_id);

/* send the NVME_LOG_LID_ERROR ioctl */
err_log = _nvme_alloc (sizeof (struct nvme_error_log_page) * elpe);
g_warn_if_fail (err_log != NULL);
err_log = _nvme_alloc (sizeof (struct nvme_error_log_page) * elpe, error);
if (!err_log) {
close (fd);
return NULL;
}
ret = nvme_get_log_error (fd, elpe, FALSE /* rae */, err_log);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down Expand Up @@ -948,8 +972,11 @@ BDNVMESelfTestLog * bd_nvme_get_self_test_log (const gchar *device, GError **err
return NULL;

/* send the NVME_LOG_LID_DEVICE_SELF_TEST ioctl */
self_test_log = _nvme_alloc (sizeof (struct nvme_self_test_log));
g_warn_if_fail (self_test_log != NULL);
self_test_log = _nvme_alloc (sizeof (struct nvme_self_test_log), error);
if (!self_test_log) {
close (fd);
return NULL;
}
ret = nvme_get_log_device_self_test (fd, self_test_log);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down Expand Up @@ -1092,8 +1119,11 @@ BDNVMESanitizeLog * bd_nvme_get_sanitize_log (const gchar *device, GError **erro
return NULL;

/* send the NVME_LOG_LID_SANITIZE ioctl */
sanitize_log = _nvme_alloc (sizeof (struct nvme_sanitize_log_page));
g_warn_if_fail (sanitize_log != NULL);
sanitize_log = _nvme_alloc (sizeof (struct nvme_sanitize_log_page), error);
if (!sanitize_log) {
close (fd);
return NULL;
}
ret = nvme_get_log_sanitize (fd, FALSE /* rae */, sanitize_log);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down
12 changes: 8 additions & 4 deletions src/plugins/nvme/nvme-op.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,9 @@ static __u8 find_lbaf_for_size (int fd, __u32 nsid, guint16 lba_data_size, guint
guint i;

/* TODO: find first attached namespace instead of hardcoding NSID = 1 */
ns_info = _nvme_alloc (sizeof (struct nvme_id_ns));
g_warn_if_fail (ns_info != NULL);
ns_info = _nvme_alloc (sizeof (struct nvme_id_ns), error);
if (!ns_info)
return 0xff;
ret = nvme_identify_ns (fd, nsid == 0xffffffff ? 1 : nsid, ns_info);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down Expand Up @@ -214,8 +215,11 @@ gboolean bd_nvme_format (const gchar *device, guint16 lba_data_size, guint16 met

/* check the FNA controller bit when formatting a single namespace */
if (! ctrl_device) {
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl));
g_warn_if_fail (ctrl_id != NULL);
ctrl_id = _nvme_alloc (sizeof (struct nvme_id_ctrl), error);
if (!ctrl_id) {
close (args.fd);
return FALSE;
}
ret = nvme_identify_ctrl (args.fd, ctrl_id);
if (ret != 0) {
_nvme_status_to_error (ret, FALSE, error);
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/nvme/nvme-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ void _nvme_fabrics_errno_to_gerror (int result, int _errno, GError **error);
G_GNUC_INTERNAL
gint _open_dev (const gchar *device, GError **error);
G_GNUC_INTERNAL
void *_nvme_alloc (size_t len);
void *_nvme_alloc (size_t len, GError **error);

#endif /* BD_NVME_PRIVATE */