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

[tbzatek]

In case the transport_svcid argument is NULL, supply sane default values. For the TCP and RDMA transport it means port 4420 for I/O controllers and 8009 for discovery controllers, distinguished by the subsystem NQN specified.

This may change default behaviour slightly from previous libblockdev versions as the behaviour was mostly undefined (from libblockdev side).

---

This should be added to the release notes: "bd_nvme_connect() now defaults to port 4420 or 8009 for discovery NQN respectively when the transport_svcid argument is not specified.".

[gemini-code-assist]

Summary of Changes

Hello @tbzatek, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a significant improvement to NVMe-over-Fabrics connection handling by establishing well-defined default transport_svcid values within the bd_nvme_connect() function. This ensures that when a port number is not explicitly provided for TCP or RDMA transports, the system intelligently defaults to either port 4420 for I/O controllers or 8009 for discovery controllers, thereby standardizing connection behavior and improving robustness.

Highlights

• Default transport_svcid values: The bd_nvme_connect() function now automatically supplies default transport_svcid values (port numbers) when the argument is not explicitly provided.
• Specific port assignments: For TCP and RDMA transports, port 4420 is used for I/O controllers and 8009 for discovery controllers, determined by the subsystem NQN.
• Behavior change: This change may slightly alter previous libblockdev behavior, which was previously undefined regarding default transport_svcid values.
• Documentation update: The API documentation for bd_nvme_connect() has been updated to reflect these new default behaviors.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces default values for transport_svcid when connecting to NVMe-oF targets. The implementation for the tcp transport is correct, but it seems to diverge from the stated intent for the rdma transport, which should also use a different port for discovery controllers. I've suggested a code change to align the implementation with the description and simplify the logic. I've also provided suggestions to clarify the updated documentation, which is currently a bit ambiguous.

[gemini-code-assist]

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);
        }

[tbzatek]

Nope.

[gemini-code-assist]

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.

[tbzatek]

I know, that's what I do.

[gemini-code-assist]

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.

[tbzatek]

Nope.