nvme: set --tls and --concat correctly

[martin-gpy]

There are a few instances where --tls and --concat are still not set correctly. Fix them.

[martin-gpy]

@igaw ,

Is there something wrong with the build here? Other than the usual checkpatch warning of exceeding 80 chars per line, there should not be any other errors/warnings here for the above commits, but still seeing the above build errors.

[igaw]

The CI build just works fine. Your first patch is missing a closing bracket.

Anyway, given this a plain copy of existing code, I suggest to introduce a helper for this instead duplicating it. Then we only have one snippet to maintain.

Second patch, it looks correct. But I am pretty sure it was not there randomly. IIRC, some other configuration setups are depending on it. That means I am not against this, but I think something will break. Given with the --concat stuff, I need to go over these things once again. It's unfortunately a bit tricky to test right now.

I am using a test script to go over 'all' configuration possibilities. It is WIP, thus has bugs and quirks. I still hope to get this integrated into CI. The biggest problem is that it wants to host and target to run on different machines, because of the secrets stored in the keyring. @hreinecke had the idea to use a different keyring for nvmet. This would allow to run this type of tests on one machine. Need to look into this first, I suppose.

test-tls.sh

[martin-gpy]

The CI build just works fine. Your first patch is missing a closing bracket.

Ah ok.

Anyway, given this a plain copy of existing code, I suggest to introduce a helper for this instead duplicating it. Then we only have one snippet to maintain.

Ok, I can take a look at that.

Second patch, it looks correct. But I am pretty sure it was not there randomly. IIRC, some other configuration setups are depending on it. That means I am not against this, but I think something will break. Given with the --concat stuff, I need to go over these things once again. It's unfortunately a bit tricky to test right now.

Got your point. Honestly I'd prefer this patch itself to avoid incorrectly setting --tls again here, because either --tls or --concat should have already been set earlier. That said, if we want to go down this route itself, can we rely on other tls sysfs attributes like say tls_configured_key & tls_keyring to determine whether --tls or --concat was used? If so, we can use that instead here in the code.

[igaw]

Got your point. Honestly I'd prefer this patch itself to avoid incorrectly setting --tls again here, because either --tls or --concat should have already been set earlier. That said, if we want to go down this route itself, can we rely on other tls sysfs attributes like say tls_configured_key & tls_keyring to determine whether --tls or --concat was used? If so, we can use that instead here in the code.

IIRC, the issue is that it's not possible to get the 'input' back from the kernel. And this line was there to workaround this problem. As I said, I am not against to remove it. The read sysfs function should not have side effects like this.

I just wanted to point out, that it likely will cause a regression somewhere else. It's a bit of Whac-A-Mole situation. Hannes is taken up the task to cleanup/refactor the fabrics code and also started to look into the tree code. Hopefully we can improve this part of the code base.