Passing -J <JSON_config_file> option results in error

[martin-gpy]

I'm aware that INI config file changes are in place now. But reading a JSON config file now results in a malformed line error which used to work properly in the past. For e.g.

# cat /usr/local/etc/nvme/config.json
[
  {
    "hostnqn":"nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a",
    "hostid":"e6550026-173e-4959-ba74-be367844bd8a",
    "subsystems":[
      {
        "nqn":"nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0",
        "ports":[
          {
            "transport":"tcp",
            "traddr":"192.168.1.117",
            "host_traddr":"192.168.1.6",
            "trsvcid":"4420"
          }
        ]
      }
    ]
  }
]

# nvme connect-all -J /usr/local/etc/nvme/config.json
Failed to lookup host 'nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a'
/usr/local/etc/nvme/config.json:1: malformed line "["
failed to read /usr/local/etc/nvme/config.json: Invalid argument
#

Is this expected? Or it no longer handles the -J <JSON_config_file> option anymore?

[igaw]

This is supposed to work, especially with the automatic conversion. During the work on the configuration format we found out that the were bugs in the json parser which didn't follow the schema definition. I think your config example is a version before the hosts level was introduced. The 37a7247 ("libnvme/json: write the config as an object, keep reading the legacy array") change is likely causing this regression. We should add your example to the tests and fix the parser.

@martin-belanger

[martin-belanger]

Thanks for the report — you're right that this used to work and doesn't anymore. Confirmed: the current INI parser doesn't detect a legacy JSON file by its .json extension, so it tries to read it as INI syntax and fails with a confusing "malformed line" error instead of a clear one pointing you at nvme config-convert. Working on a fix.

[martin-belanger]

Update: I tried to reproduce this directly with your exact config.json content on current master and couldn't — nvme connect-all -J config.json correctly detects the .json extension, auto-converts it, and proceeds (renaming the original to .converted). So my first read was wrong; that detection logic is there and working.

Two things that would help narrow it down: was there already a config.conf (or similarly-named file) sitting at that location from an earlier attempt? A stale partial conversion at the target path could explain it. And what's the exact full command you ran — any other flags, like -o json?

[martin-gpy]

Two things that would help narrow it down: was there already a config.conf (or similarly-named file) sitting at that location from an earlier attempt?

Yes, there is a discovery.conf at that location, but no config.conf file there.

A stale partial conversion at the target path could explain it. And what's the exact full command you ran — any other flags, like -o json?

I ran a connect-all with the -J option itself, and passed no other flags such -o json. Exact command was nvme connect-all -J /usr/local/etc/nvme/config.json

[martin-belanger]

@martin-gpy - I'm not able to reproduce on my system running the latest master.

One more thing worth checking: could this be a stale libnvme? If you've built with /usr/local as the prefix before, it's easy to end up with an old libnvme.so still installed under /usr (or vice versa) that the dynamic linker picks up instead of your freshly built one -- I've hit this myself before.

A few things that would help:

• nvme --version -- confirms exactly what you're running.
• ldd $(which nvme) | grep libnvme -- shows which libnvme.so the binary is actually loading at runtime.
• find /usr /usr/local -name 'libnvme*.so*' 2>/dev/null -- lists every copy installed on the system, in case there's a stale one lying around.

If it turns out to be stale, a clean rebuild + reinstall off the latest master should resolve it -- but the above will tell us for sure either way.

[martin-gpy]

Yes, turned out several stale entries were lying around. A clean rebuild + reinstall helped resolve it.

# nvme connect-all -J /usr/local/etc/nvme/config.json
Failed to lookup host 'nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a'
no /usr/local/etc/nvme/config.conf found; converted legacy /usr/local/etc/nvme/config.json to it -- the original is renamed to *.converted, use /usr/local/etc/nvme/config.conf from now on

# nvme connect-all -J /usr/local/etc/nvme/config.conf
Failed to lookup host 'nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a'
already connected to hostnqn=nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a,nqn=nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0,transport=tcp,traddr=192.168.1.117,trsvcid=4420
/usr/local/etc/nvme/config.conf.d/100-persona.conf: Operation already in progress

So it is working now, and I'm able to see it properly connect-all via the -J option on the JSON file.

Couple of follow-up queries:

1. Is the Failed to lookup host warning expected above?

2. Will the -J option now only work on a .json file alone? Previously, the -J option could work on any file (not necessarily with those having a .json extension alone).

[martin-gpy]

Tried connect-all with the -J option on another JSON file as follows:

# cat /usr/local/etc/nvme/config.json.converted
[
  {
    "hostnqn":"nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a",
    "hostid":"e6550026-173e-4959-ba74-be367844bd8a",
    "dhchap_key":"DHHC-1:01:fnitvevFrPTt0ITUwYJZlPVwUhbFS5bStvgb0F4gzzTPwRUc:",
    "subsystems":[
      {
        "nqn":"nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0",
        "ports":[
          {
            "transport":"tcp",
            "traddr":"192.168.1.117",
            "host_traddr":"192.168.1.6",
            "trsvcid":"4420",
            "persistent":true,
            "keep_alive_tmo":15,
            "dhchap_ctrl_key":"DHHC-1:03:DvxTgGTyHKTbAL72HYO8ua9djByiArJ70xsIewT+GY3nl+UBBh7XJbedgB+Wo0M4bs9s+aim3bkEWgIvn7UkrAGJINs=:",
            "concat":true
          }
        ]
      }
    ]
  }
]

And this got converted to the .conf file as follows:

# cat /usr/local/etc/nvme/config.conf.d/100-persona.conf
[Host]
hostnqn = nqn.2014-08.org.nvmexpress:uuid:e6550026-173e-4959-ba74-be367844bd8a
hostid = e6550026-173e-4959-ba74-be367844bd8a

[Subsystem]
nqn = nqn.1992-08.com.netapp:sn.48391d66c0a611ecaaa5d039ea165514:subsystem.subsys_CLIENT116_0
keep-alive-tmo = 15
dhchap-ctrl-secret = DHHC-1:03:DvxTgGTyHKTbAL72HYO8ua9djByiArJ70xsIewT+GY3nl+UBBh7XJbedgB+Wo0M4bs9s+aim3bkEWgIvn7UkrAGJINs=:
concat = true
dhchap-secret = DHHC-1:01:fnitvevFrPTt0ITUwYJZlPVwUhbFS5bStvgb0F4gzzTPwRUc:
controller = transport=tcp;traddr=192.168.1.117;trsvcid=4420;host-traddr=192.168.1.6

So couple of points seeing the above:

1. The dhchap_secret key is defined in the Host section of the JSON file, but it shows up in the Subsystem section of the 100-persona.conf. So that may need to be corrected.

2. Setting persistent to true (i.e. create a PDC as well) in the JSON file is not showing up in the respective conf file. But that I suppose requires the fixes described at json: pdc related fixes while using JSON config file #3594 to be merged first here?

[martin-belanger]

Thanks for confirming — glad the stale-libnvme theory panned out.

To your two questions:

1. The "Failed to lookup host" warning: not expected, and your report helped me track it down. It's a bug in libnvmf_discovery_nbft() — it looks up the host using a couple of variables that haven't been assigned yet at that point (they're only set later, per NBFT-table entry), so the lookup always fails and logs that error on every connect-all/discover unless --no-nbft is passed. It's harmless in your case because that failure gets silently overwritten by the successful result of processing your config file right after — but it's still wrong and needs fixing.

2. -J and file extensions: no change there — only files that literally end in .json get auto-converted; anything else is still read directly as INI, same as before.

While chasing down #1 I found a second, related bug your report surfaced: when you point -J explicitly at the default config path (.../config.json), the auto-conversion names the result config.conf instead of the canonical nvme-fabrics.conf. If you'd instead run a bare nvme connect-all with no -J at all, it would have converted to nvme-fabrics.conf. Same file, two different output names depending on whether -J was typed explicitly. Worse, it orphans the config: a later bare nvme connect-all looks for nvme-fabrics.conf and the original config.json (now renamed .converted), finds neither, and silently proceeds with an empty config instead of erroring.

Both are real bugs against current master, not leftovers of the stale-build issue you had. I'm fixing both on a new branch as a separate PR from the one currently up for review — I'll link it here once it's ready. Thanks again for the detailed follow-up, it made both easy to pin down.

[martin-belanger]

Regarding your second round of questions:

1. The dhchap_secret key is defined in the Host section of the JSON file, ...

1. The dhchap_secret/dhchap-secret placement isn't a conversion bug — it's intentional, and it's actually the more spec-correct placement of the two. The spec scopes DH-HMAC-CHAP secrets to the (hostnqn, subsysnqn) pair, not to the host alone: Base Spec 2.3 §8.3.5.5.7 defines the key as a function of the entity's own NQN, and its sanctioned variation axis is "a different secret with each host" or "with each NVM subsystem" — a host can legitimately use a different secret per subsystem it connects to. NVMe/TCP Transport 1.2 §3.6.1.4 goes further and makes per-path divergence within the same (host, subsystem) pair a SHALL-NOT.

The legacy JSON's dhchap_key sitting on the host object was a convenience default, not that scope — a pure host-level constant can't represent "a different secret per subsystem." The converter already resolves that (a port's explicit value overrides the host default; if they disagree it's flagged, since that combination was already non-compliant in the source file) down to one effective value, and writes it once per subsystem — which is why it lands under [Subsystem] in the .conf output rather than [Host]. So what you're seeing is the intended model, not something to fix.

2. Setting persistent to true (i.e. create a PDC as well)

2. On PDC: yes, exactly — that gap is real and I'm aware of it. I've been keeping an eye on json: pdc related fixes while using JSON config file #3594 (which I see is your own PR) as well as (Make persistent discovery controller configurable for the subsystem #3536, Use persistent discovery controller by default #3472, Create NVMe PDC by default for each unique target port #3112) and the plan is to bring equivalent PDC support into the INI path once it settles; there's been a fair amount of churn on the PDC design still to shake out first.

[martin-belanger]

@martin-gpy - One more thing worth flagging while we're on the PDC topic: #3594 is going to become moot once #3613 merges (a separate PR I have up for review right now) — that one removes the legacy JSON config path (json.c) from libnvme entirely, so the code your fixes target won't exist anymore. It's not a rejection of the fixes themselves, just a heads-up before you sink more time into it against a path that's on its way out.

Once #3536 settles enough to know what PDC support actually looks like, I'll loop you in on bringing equivalent support to the INI side — that's the path forward from here.

[igaw]

FWIW, there are only a couple of minor things left in #3613 and I expect to merge it today. On PDC, let's get the INI feature fully implemented first before looking into the PDC story. Too many moving parts in this area.