libnvme/config: add an INI-format NVMe-oF config parser by martin-belanger · Pull Request #3566 · linux-nvme/nvme-cli

martin-belanger · 2026-07-08T18:47:32Z

Hi Daniel,

Here's the big PR I promised — ~4,400 lines total, but only ~1,400 of that is actual code and another ~1,400 tests; the rest is kdoc/comments, blank lines, and the design doc. It's self-contained: nothing calls into it yet, so no regression risk today. It's this size because it does two things the JSON blob didn't: real validation with file:line diagnostics, and a drop-in file layout.

Suggested order:

CONFIG.md first (the design contract),
then config.h (the whole public surface),
then the commits in order:
3.1 tokenizer: libnvme/config: add an internal INI reader
3.2 param bag: libnvme/config: add the parameter bag and key table
3.3 raw parser: libnvme/config: parse a config file into its raw model
3.4 cascade resolver: libnvme/config: resolve the cascade into the flat connection list
3.5 public API: libnvme/config: publish the resolved configuration
3.6 arg emitter: libnvme/config: add the connect argument emitter
Last commit is unrelated — a pre-existing host_iface/trsvcid copy-paste mixup in libnvmf_add_ctrl(), fixed while I was in the area.

A second (much smaller) PR (write side + nvme config convert) follows once this one lands.

Describe the INI-format connection configuration intended to replace /etc/nvme/config.json: the file layout (a main nvme-fabrics.conf plus an optional nvme-fabrics.conf.d/ drop-in directory), the single- and multi-personality models, the precedence/merge rules with file-scoped defaults, the TLS and DH-CHAP security parameters, and the validation tiers. The format is parsed by libnvme so that nvme-cli, nvme-discoverd, and nvme-stas can all share one dependency-free parser. libnvme itself never reads config.json: conversion is a one-shot, nvme-cli-side job (nvme config convert), not a runtime fallback -- matching the "no third-party dependency" rationale this document already gives for choosing INI in the first place. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Tokenize [Section] / "key = value" / # comment files and report events through a callback; all meaning stays with the consumer. An empty value ("key =") is distinct from an absent key, and a malformed section header clears the current section so the lines below it are never misattributed. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

The bag keeps each connection parameter in one of three states -- unset (inherit), "" (reset to the kernel default), or a value -- which is what makes the precedence cascade expressible. The key table declares each recognized key once: its value type and the class that says where it may appear (tunable, security, [Host] identity, endpoint). The legacy numeric --tls_key is deliberately absent, and fast-io-fail-tmo uses the hyphenated spelling the 3.0 rename settles on. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Record one file's sections and lines faithfully; nothing is resolved here (the cascade resolver consumes a list of these). [Global] and the per-type defaults merge with a warning when repeated, [Host] is a hard singleton, and endpoint sections are repeatable. controller= lines split into the path's raw addressing strings and the per-path tunable overrides; unknown or security keys on a path line are errors. Tier 1 problems fail the file with a file:line diagnostic, Tier 2 warns and continues. Addressing stays raw strings, not a TID: a config file is a human interface and traddr/host-traddr may legitimately name a host (libnvme/design/INTEGRATION.md's worked example), while TID construction rejects hostnames (libnvme/design/TID.md) -- resolving them is the consumer's job, not the parser's. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Load the main file plus its sorted drop-ins and apply the precedence cascade; the result is what consumers see: a flat list of (role, addressing, identity, params) connections, main file first. Each resolved DC carries its file scope's DC- and IOC-default parameter blocks, so controllers *discovered* via that DC (referral DCs, DLP entries) inherit the right defaults; mDNS-found DCs draw from the top-level scope. The relational Tier 1 rules reject a nameless drop-in persona, one hostid shared by two personas, and one hostnqn carrying two hostids. Addressing and identity stay raw strings, not a TID: traddr may still be a hostname at this point (the parser never resolves it), and TID construction rejects one -- building it is the consumer's job, after it resolves. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Give the resolved model its public face: read/validate/free on the configuration, iteration over the flat connection list, per-connection getters, and the discovered-controller parameter lookup a discovery daemon needs to parameterize controllers it finds at runtime (DLP, AEN, mDNS) that have no section in any file. Files, sections and drop-ins remain parser-internal; the only things that cross the API boundary are (role, addressing, identity, params) tuples. Addressing and identity are per-field getters, not a TID: traddr/host-traddr may still be a hostname at this point, and TID construction rejects one -- building it is the caller's job, once it has resolved. The parameter bag's read half becomes public with it, since connection parameters are handed out as borrowed bags. The new test links against the real shared library rather than the test library, so it also proves the version-script exports. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

The whole point of the flat (role, addressing, identity, params) list is that each element can become the option list of one "nvme connect" invocation -- a discovery daemon's transient units exec exactly that, and a resolved "config show" can render each connection as a copy-pasteable command line. Emit the TID's addressing and identity first, then the parameters; unset and reset parameters are not emitted so the kernel default applies, and boolean parameters become bare flags. Taking the (TID, params) pair rather than a connection lets the same emitter serve runtime-discovered controllers, whose TID the consumer builds itself and whose parameters come from libnvmf_config_resolve_discovered(). It's also why the pair, not the connection, is the argument: a connection's addressing is raw strings (config.h), so the caller resolves and builds the TID before calling in, exactly as the test now does. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

A copy-paste error left host_iface sourced from trsvcid in the fctx built for libnvmf_ctrl_find(), so an interface-scoped lookup against an existing subsystem never matched on host_iface. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

igaw · 2026-07-09T06:40:00Z

+
+[Host]
+# hostnqn comes from /etc/nvme/hostnqn by default
+# hostid comes from /etc/nvme/hostid by default


I wonder if we should not move them also into the global section. WDYT?

though I haven't figured out what the impact of this would be... if those stay separate, it would be good to document it here why this is the case.

igaw · 2026-07-09T06:45:22Z

+hostnqn       = nqn.2014-08.org.nvmexpress:uuid:1111…-A
+hostid        = 46ba5037-7ce5-41fa-9452-48477bf00080
+hostsymname   = Hydra
+dhchap-secret = DHHC-1:00:…


Maybe remove the dhcap example here, we should not encourage people to use this method for loading the keys into the kernel. Another big TODO on the list is to make the secret loading a bit more useful for production systems.

Or just rename the example testing-fabrics.conf :)

igaw · 2026-07-09T06:55:29Z

+
+### Security parameters
+
+A connection's security settings fall into two families — **authentication** (DH-CHAP, which proves the host and controller identities in-band) and **encryption** (TLS, which protects the data on the wire) — plus one parameter that bridges them. Because they are bound to the host+subsystem relationship, they live on the **endpoint section**, not on a `controller =` path (the exception above).


Maybe add something adding secrets to the config file are there for testing purposes. It's not a good idea to do it in a production system. The idea is that the keys are handled by a 3rd party which loads them into the kernel keyring. I was looking into how this works with the systemd ecosystem. Unfortunately, never got anywhere. That's why currently we have this very simple udev rule which loads the file content in /etc/nvme/keys into the kernel. Not really good either, it's stop gap solution.

igaw · 2026-07-09T07:26:33Z

+ * The tokenizing loop, over a buffer the caller already owns and may
+ * modify in place.  libnvmf_ini_parse_buf() dups its const input once to
+ * get one of these; libnvmf_ini_parse_file() already allocated its own and
+ * passes it straight through, with no second copy.


I'd say the second part should be removed, that is just reasoning not really helping the reader to understand this function.

igaw · 2026-07-09T07:28:08Z

+
+		/* ...unless unterminated, empty, or followed by text. */
+		if (!end || end == libnvmf_trim(s + 1) ||
+		    *libnvmf_trim(end + 1)) {


running libnvmf_trim twice is a bit excessive.

igaw · 2026-07-09T07:31:02Z

+	if (!ctx || !text || !callback)
+		return -EINVAL;
+
+	copy = xstrdup(text);


on line 128 we test for !text so a simple strdup should be fine or drop the check in line 128

igaw · 2026-07-09T07:33:19Z

+{
+	__cleanup_free char *text = NULL;
+	struct stat st;
+	FILE *f;


__cleanup_file FILE *f = NULL would avoid those explicit fclose in the error path

igaw · 2026-07-09T07:35:17Z

+		return -EINVAL;
+
+	return parse_lines(text, callback, user_data);
+}


Nice little parser. I like :)

igaw · 2026-07-09T07:43:12Z

+{
+	int *count = user_data;
+
+	if (++(*count) == 2)


This is not that easy to read (remembering the operator preference rules...), maybe make this a bit explicit.

igaw · 2026-07-09T07:48:23Z

+struct libnvmf_params {
+	struct kv *head;
+	struct kv **tail;
+};


fwiw, there is also a ccan list implementation, would avoid another open coded version...

igaw · 2026-07-09T07:56:37Z

+	}
+	e->value = copy;
+	*p->tail = e;
+	p->tail = &e->next;


this part could go into a smaller function something like libnvmf_params_new_add (a good name obviously is needed), would document what this snippet does.

igaw · 2026-07-09T08:00:35Z

+}
+
+int libnvmf_params_merge(struct libnvmf_params *dst,
+		const struct libnvmf_params *src)


is the order of the arguments sane? I usually prefer it to have it src, dst but that's me. If there are other interfaces which follow this pattern, than okay, but if not, I'd rather have the other way around.

igaw · 2026-07-09T08:04:36Z

+	long v;
+
+	errno = 0;
+	v = strtol(value, &end, 10);


could be also support hex values?

igaw · 2026-07-09T08:21:26Z

 		return -EINVAL;

-	copy = strdup(value);
+	copy = xstrdup(value);


value is NULL checked, so strdup works just fine.

igaw · 2026-07-09T08:21:56Z

 	if (!e)
 		goto fail;
-	e->key = strdup(key);
+	e->key = xstrdup(key);


same here key is NULL checked.

igaw · 2026-07-09T08:22:39Z

-	{ "nqn",			LIBNVMF_KEY_STRING,	LIBNVMF_KEY_NQN },
-	{ "controller",			LIBNVMF_KEY_STRING,	LIBNVMF_KEY_CONTROLLER },
+	{ "nqn",		LIBNVMF_KEY_STRING,	LIBNVMF_KEY_NQN },
+	{ "controller",		LIBNVMF_KEY_STRING,	LIBNVMF_KEY_CONTROLLER }


these whitespace changes are not really necessary. could all go into the previous patch.

igaw · 2026-07-09T08:27:21Z

+	struct libnvmf_params *dc_defaults;
+	struct libnvmf_params *ioc_defaults; /* [I/O Controller Defaults] */
+	bool has_host;			/* file carries a [Host] section */
+	char *hostnqn;			/* "" records an explicit reset */


this comment is redundant, no? I though this is the model for all parameters.

igaw · 2026-07-09T08:28:25Z

+	char *traddr;
+	char *trsvcid;		/* NULL when the file left it unset */
+	char *host_traddr;	/* NULL when the file left it unset */
+	char *host_iface;	/* NULL when the file left it unset */


do we need those comments, I find this obvious.

igaw · 2026-07-09T08:30:05Z

+	 * raw strings, not a TID: a config file is a human interface and may
+	 * legitimately name a host, and TID construction rejects hostnames
+	 * (libnvme/design/TID.md) -- resolving them is the consumer's job.
+	 * subsysnqn and the host identity are filled in at resolve time.


The first part of the documntation is good, the reasoning why is already lenghty explained in the documents.

igaw · 2026-07-09T08:31:55Z

+	char *host_iface;	/* NULL when the file left it unset */
+	/* per-path tunables; never security */
+	struct libnvmf_params *overrides;
+	unsigned int line;	/* 1-based config-file line, for diagnostics */


the comment doesn't really help me at all, need to look at the code. maybe just say used 'for diagnostics'?

igaw · 2026-07-09T08:33:43Z

+
+struct libnvme_global_ctx;
+
+struct libnvmf_conf_path {


The main issue I see with this interface is, that it's makes this part of the API fixed. We can't really change it after the release without breaking existing users. Do we really want to do this?

unless this header is not exposed as API... :)

igaw · 2026-07-09T08:40:17Z

+		return -ENOMEM;
+	}
+
+	for (tail = &pc->f->endpoints; *tail; tail = &(*tail)->next)


if we move the structs into the implementation, we just could use the ccan list APIs...

and after reading through all (and remembering the cover letter...) I'd say it makes sense to use the ccan lists which also give some level of type information.

igaw · 2026-07-09T08:43:58Z

+	pc->ep = NULL;
+
+	if (!strcmp(name, "Global")) {
+		if (pc->f->global)


struct libnvmf_conf_file *f = pc->f;

would make it a bit simpler to read IMO.

igaw · 2026-07-09T08:57:53Z

+	{ "hdr-digest",			LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_TUNABLE },
+	{ "data-digest",		LIBNVMF_KEY_BOOL,	LIBNVMF_KEY_TUNABLE },
+
+	/* security -- bound to (hostnqn, subsysnqn); never per-path */


So that means our config.json is actually wrong, no? There it's model into the port section, which is 'per-path' or do I get this wrong again... confused...

just for reference

igaw · 2026-07-09T09:17:52Z

+}
+
+struct libnvmf_conf *libnvmf_conf_load(struct libnvme_global_ctx *ctx,
+		const char *path, int *err)


I would suggest to follow the error return code pattern used. That is the result is the last argument and not the return code.

igaw · 2026-07-09T09:22:26Z

+ *
+ * Return: the resolved configuration, or NULL on error.
+ */
+struct libnvmf_config *libnvmf_config_read(struct libnvme_global_ctx *ctx,


I'd prefer to return an error code here and have the result as last argument.

igaw · 2026-07-09T09:41:23Z

Generally, I really like this. It's a big PR but that was to be expected. And it's good to see the whole picture. Also the diagnostic part is a nice feature to have. The public API is not that big after realizing what is exposed and what not. Good job!

The main points from my first review pass is mostly nitpicking (function signatures and not using ccan list)

My big question is about config.json: is this correctly modeled? From my current understanding it is not... and the other question is: do we want to keep /etc/nvme/hostnqn and /etc/nvme/hostid alive?

Martin Belanger added 8 commits July 8, 2026 14:09

igaw reviewed Jul 9, 2026

View reviewed changes


		### Security parameters

		A connection's security settings fall into two families — authentication (DH-CHAP, which proves the host and controller identities in-band) and encryption (TLS, which protects the data on the wire) — plus one parameter that bridges them. Because they are bound to the host+subsystem relationship, they live on the endpoint section, not on a `controller =` path (the exception above).

Uh oh!

Conversation

martin-belanger commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

martin-belanger commented Jul 8, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw Jul 9, 2026 •

edited

Loading

igaw commented Jul 9, 2026 •

edited

Loading