nvme-cli/fabrics: consume the INI config directly from the CLI#3595
Open
martin-belanger wants to merge 2 commits into
Open
nvme-cli/fabrics: consume the INI config directly from the CLI#3595martin-belanger wants to merge 2 commits into
martin-belanger wants to merge 2 commits into
Conversation
added 2 commits
July 20, 2026 10:27
Add libnvmf_context_apply_params() to set an fctx from the params read from an INI file. An unset key is left alone, and an explicit reset (the empty string) is skipped rather than applied as zero. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
connect-all, discover, and connect -J now consume the INI config directly. Looping over each INI entry is performed by the CLI itself, so hostnames can be resolved (a policy decision) before calling into libnvme. Choosing hostnqn/hostid is likewise a policy decision, so that also stays in the CLI rather than libnvme. The CLI also honors the exclusion list to prevent unwanted connections. Retire the /run/nvme volatile-config reader and the discovery.conf line-parser hooks: nothing reads the former, and the INI config replaces the latter's sole caller. Signed-off-by: Martin Belanger <martin.belanger@dell.com>
igaw
reviewed
Jul 21, 2026
| else if (!strcmp(key, "keep-alive-tmo")) | ||
| /* Applied verbatim; the NQN-conditional default is a | ||
| * connect-time decision, see set_discovery_kato(). | ||
| */ |
Collaborator
There was a problem hiding this comment.
'this comment doesn't make sense to me this is KATO who does the nqn play a role?
igaw
reviewed
Jul 21, 2026
| } | ||
| /* Identity/addressing keys never reach here: get_params() only | ||
| * surfaces tunable and security keys. | ||
| */ |
Collaborator
There was a problem hiding this comment.
I understand the first part of the comment, the second part is what? this is apply_params?
igaw
reviewed
Jul 21, 2026
| return; | ||
|
|
||
| /* strtol(..., 0) matches check_int()'s hex/octal-aware parsing | ||
| * (config-ini.c); atoi() would mis-parse "0x10" as 0. |
Collaborator
There was a problem hiding this comment.
no need to add man page documentation on strtol.
igaw
reviewed
Jul 21, 2026
| const char *ctrlkey; | ||
| const char *keyring; | ||
| const char *tls_key; | ||
| const char *tls_key_identity; |
Collaborator
There was a problem hiding this comment.
libnvmf_context has
const char *hostkey; // !access:write=custom
const char *ctrlkey; // !access:write=custom
const char *keyring; // !access:write=custom
char *tls_key; // !access:write=custom
const char *tls_key_identity; // !access:write=customWhy do we need to treat them special. The comment mentiones 'clobbering' how? There is nothing special about these keys, we can either pass them via the connect string or we load them into the kernel keystore.
igaw
reviewed
Jul 21, 2026
|
|
||
| /* An explicit reset is not applied: @fctx already carries the | ||
| * kernel default it was created with (libnvmf_default_config()). | ||
| */ |
Collaborator
There was a problem hiding this comment.
but only two keys have defaults
void libnvmf_default_config(struct libnvme_fabrics_config *cfg)
{
cfg->tos = -1;
cfg->ctrl_loss_tmo = NVMF_DEF_CTRL_LOSS_TMO;
}what about the rest? and these are not kernel defaults. these are library defaults.
Collaborator
|
For the first commit could you update the commit message. It's really hard to read. Maybe ? |
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
2 participants
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.
Summary
libnvmf_context_apply_params(), a table-driven helper that applies a resolved parameter set (from INI) to a connect context (fctx).connect-all/discover/connect -Jto read the INI configuration directly and iterate connections in the CLI itself, instead of going through libnvme's JSON config loops. Hostname resolution and hostnqn/hostid selection are policy decisions and now live in the CLI. The CLI enforces the exclusion list on every auto-connected entry./run/nvmevolatile-config reader and the discovery.conf line-parser hooks: nothing reads the former, and the INI config replaces the latter's only caller.First slice of moving nvme-cli off the legacy JSON config path onto the INI config from #3566/#3587/#3590/#3591. Self-contained:
-O/--dump-config,nvme config's-M/-R/-U, and the now-unused JSON loops in libnvme are untouched here, addressed in follow-up PRs.Test plan
meson test -C .build— 86/86 passingmake checkpatch— 0 errorsnvme connect -J <ini-file>,nvme discover,nvme connect-allagainst scratch INI configs (default path, custom-J, legacy-JSON auto-convert), under valgrind — no leaks