fabrics: update printing an error for ENOENT too

[martin-gpy]

Many nvme commands such as discover, connect, connect-all, etc. simply return to the prompt without printing any useful error message when the respective nvme driver modules are not loaded in the kernel. This is because nothing gets printed when nvme_scan_topology() returns an ENOENT due to missing nvme system files stemming from not loading the appropriate nvme modules. Fix the same.

[igaw]

IIRC, this is done on purpose. I agree it would make sense to print any error though.

@keithbusch do you remember why it is this way?

[calebsander]

This is essentially reverting #2250. As I commented there, I agree suppressing the warning message is not an improvement and am in favor of this revert. #2241 was supposed to be about fixing nvme disconnect-all not to error out if nvme_core isn't loaded, rather than making it error out with no helpful output.

[martin-gpy]

So what are the next steps here?

It is indeed confusing for an end user to see a blank output for these nvme commands during these ENOENT scenarios. Printing an error is definitely helpful/intuitive for such a case...

[igaw]

If I get this right, we have to decide between two opposing behavior If modules are not loaded:

1) don't report anything including no error code

or

2) report it and set error code

I get the arguments for both ways. Personally, I tend to say reporting an error is more user friendly. We could obviously added a global options which like --no-error-if-modules-are-missing...

[martin-gpy]

If I get this right, we have to decide between two opposing behavior If modules are not loaded:

1. don't report anything including no error code

What exactly is the argument for this behavior? We did have multiple instances from the field where this was reported, and it actually took some time to figure out that the respective modules not loaded was the culprit. Printing a simple error could have saved all that effort..

or

2. report it and set error code

I get the arguments for both ways. Personally, I tend to say reporting an error is more user friendly. We could obviously added a global options which like --no-error-if-modules-are-missing...

I honestly don't prefer a global option for this case. Seems like an overkill for something so trivial...

[calebsander]

Agree. My original ask was for nvme disconnect-all not to fail if nvme_core is unloaded, so it could be always be called to "disconnect all NVMe-oF devices" rather than requiring the caller to check if nvme_core is loaded, or if there are connected devices, or always load nvme_core first. But nvme connect can't possibly work without the module loaded. Either it should fail if the module isn't loaded with a helpful error message, or it should automatically load the module if it's not already.

[martin-gpy]

Had chatted with @igaw on this. Seems the concern is primarily with nvme disconnect-all here. So we could probably suppress this ENOENT error for disconnect-all alone, and keep it for the remaining commands. Will make that change now.

[igaw]

Looks good to me. IIRC, we still return an error for the nvme disconnect case. I think we should set it to 0 when errno says ENOENT.

[martin-gpy]

Looks good to me. IIRC, we still return an error for the nvme disconnect case. I think we should set it to 0 when errno says ENOENT.

Fine, will make that change too.

[igaw]

I've refactored the error path slightly and splitted the change for disconnect case out into a separate patch. What do you think?

[martin-gpy]

I've refactored the error path slightly and splitted the change for disconnect case out into a separate patch. What do you think?

Other than a few minor review comments, this looks good to me.

[igaw]

Thanks!