libnvme/tid: numeric-only, sanitizing Transport ID (TID) by martin-belanger · Pull Request #3554 · linux-nvme/nvme-cli

martin-belanger · 2026-07-06T21:31:53Z

libnvme/tid: numeric-only, sanitizing Transport ID (TID)

Cleanup/prep for the upcoming INI connection-config parser, not a feature. That parser needs a TID that's a pure function of numeric input. Removes get_hash()/equal() (upstream API, zero production callers) and formalizes numeric-only, sanitizing construction. A hostname member with deferred resolution was prototyped while designing the config parser but never merged upstream -- rejected, see TID.md.

Follow-up (not in this PR): drop hostname2traddr(), the last real getaddrinfo() caller in libnvme (the other 3 call sites use AI_NUMERICHOST, no DNS). Push resolution to CLI/discoverd, drop NVME_HAVE_NETDB/<netdb.h> entirely. libnvmf_traddr_is_numeric() (added here) is what that follow-up will use.

Commits (review order)

design/INTEGRATION.md -- library/app boundary: libnvme owns protocol I/O, caller owns policy/non-determinism/concurrency.
design/TID.md -- design authority for the TID.
tid: reject hostnames, canonicalize numeric addresses -- traddr/host_traddr canonicalized via inet_pton/inet_ntop; hostname fails construction. New libnvmf_traddr_is_numeric().
tid: drop get_hash() and equal() -- dead public API; hashing moves to the one caller that needs it (discoverd, via get_canonical()).
tid: replace per-field setters with a set_identity triplet -- addressing construction-only; set_identity() owns host-identity rules.
fabrics: use str(), not get_canonical(), in the exclusion log -- fixes a log line that could exceed 500 chars.
tid: group functions by role -- pure reorder, no content change.

Numbers

Code: +409/-328 | Tests: +171/-101 | Docs: +170
meson test: 81/0 fail | checkpatch + valgrind clean

igaw · 2026-07-07T08:57:18Z

Looks all fine, my biggest concern is the amount of documentation which is bloaty/repetitive and sometimes too specific (e.g. time references, change logs,...). It overloads not just me, I got feedback that people are just not reading the lengthy commit messages and documentation. While it is nice to document the code base, we also have to keep it balanced, it is not just for the machines.

martin-belanger · 2026-07-07T09:14:41Z

Looks all fine, my biggest concern is the amount of documentation which is bloaty/repetitive and sometimes too specific (e.g. time references, change logs,...). It overloads not just me, I got feedback that people are just not reading the lengthy commit messages and documentation. While it is nice to document the code base, we also have to keep it balanced, it is not just for the machines.

Agreed, I'll trim the docs.

martin-belanger · 2026-07-07T10:43:31Z

@igaw - trimmed the repeated rationale in INTEGRATION.md, TID.md, and the matching kdoc. Reworded the hostid "never random" claim. Docs are now roughly 40% of their original length.

igaw · 2026-07-07T11:30:46Z

Looks good, just my usual nitpicking.

and the reduction/cleanup in the docs is way better, thanks a lot!

Add INTEGRATION.md, a design doc explaining where the line falls between libnvme and the application that uses it: libnvme owns NVMe protocol I/O, the caller owns policy, non-determinism, and its own concurrency. It gives the main-loop contract (offload every blocking call via a worker thread or a subprocess/unit) and works the hostname- resolution example through the boundary. The CLI/library split never came with a stated rule for what belongs on each side, which is how resolution ended up inside the library. This is that rule, so future decisions have a lens to be judged against. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Add TID.md, the design authority for the NVMe-oF Transport ID: what a TID carries, how the constructors sanitize numeric addressing, why a hostname is rejected rather than resolved or deferred (addressing must be a pure function of numeric inputs -- see INTEGRATION.md), how subsysnqn/hostnqn/hostid are layered on afterward, and why the canonical form it produces is not a connection-equality oracle. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Sanitize traddr/host_traddr on the IP transports (tcp, rdma) at construction: canonicalize a numeric address (inet_pton/inet_ntop, so a compressed vs. expanded IPv6 or an IPv4-mapped form collapses to one spelling) and reject a hostname outright. There is no deferred or pending state -- from_fields()/parse() simply fail. Resolving a hostname is the caller's job, not libnvme's: a name can resolve to several addresses, and choosing one is a policy decision libnvme has no basis to make, on top of which the lookup can block on DNS (see design/INTEGRATION.md). libnvmf_traddr_is_numeric() lets a caller check a candidate address, using the same definition of numeric the constructors enforce, before deciding whether it needs to resolve. dup() keeps its verbatim xstrdup() copy rather than routing through from_fields(): the source is already sanitized, so re-running it through canon_ip() would only re-canonicalize and re-allocate addresses that are already in canonical form. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Both are pre-existing public API with no production caller in the tree (only their own unit tests). get_hash() derives a 48-bit FNV-1a name from the canonical string -- an algorithm, width, and format that are naming policy for whichever consumer needs it (discoverd, for its systemd unit names), not something libnvme has a basis to own. That consumer computes its own hash from libnvmf_tid_get_canonical() instead. equal() compares all eight fields with exact string equality, which diverges from the two comparators the tree already has for a reason: libnvme_ipaddrs_eq()/addr_equal() (address-aware) and the kernel-aware sysfs connection matcher. A third, byte-exact equality with no caller is more likely to be reached for by mistake than to be missed. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Addressing is now construction-only (from_fields/parse/dup, all sanitizing), so the eight per-field setters are removed and the fields marked write=none: there is no longer any way to set an address that bypasses sanitization, and transport -- the root of a TID's identity -- is immutable. The subsystem and host identity are layered on afterward through one call, libnvmf_tid_set_identity(subsysnqn, hostnqn, hostid), where NULL leaves a field unchanged. It is also where the host identity rules live: a missing hostid is derived from a UUID-format hostnqn (TP4126) so it is deterministic and never random, and a hostid without a hostnqn is rejected -- one host is one (hostnqn, hostid) pair. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

The exclusion-skip INFO message rendered the TID with get_canonical(), which includes the full 8-field identity. subsysnqn and hostnqn can each run to 223 characters, so that single log line could exceed 500 characters. get_canonical() is for byte-stable identity (matching for exclusion, and eventually hashing); str() is the compact, human-scannable rendering meant for exactly this kind of log line. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

Reorder with no content change: constructors first (from_fields, its companion set_identity, dup, parse, parse_strict), then the construction-adjacent traddr_is_numeric, then predicates and derived views on an existing TID (is_empty, get_canonical, str, validate). set_identity immediately follows from_fields because from_fields alone produces an incomplete TID -- set_identity is what completes it. Signed-off-by: Martin Belanger <martin.belanger@dell.com>

igaw · 2026-07-07T11:59:41Z

Thanks a lot!