nvme: Add nvme_cfg global variable for NVME_ARGS default options

[ikegami-t]

To preapare to add NVME_ARGS default option further.

[igaw]

Good idea. I was playing around with a different idea. Let me check what I've done and if there are some more cases to cover

[igaw]

Looking with fresh eyes on this PR, could you explain what do you want to achieve with this change? I mean I understand you want to group those two variable but if it is just that, I don't think we should introduce so much code churn.

BTW, I though you would do something similar what I a just posted in #2288

[ikegami-t]

Sorry for late to do add additional patches. I thought to try the timeout default options into the NVME_ARGS since currently only some commands supported the option but seems almost libnvme APIs support the timeout parameter. If you okay about this will do continue the implementation. Anyway in future for any other default options the nvme_cfg global variable seems be able to use I think.

[igaw]

Sounds alright, the only thing to keep in mind, we should avoid changing existing options.

[ikegami-t]

Just rebased the patch and also noted about the mention. Thank you.

[ikegami-t]

The last patch for delete-ns command depended on the libnvme PR changes linux-nvme/libnvme#811.

[ikegami-t]

Sorry the following commands short option changed as below by the commit c809879 since duplicated with the default timeout option -t added.

- nvme compare [--latency | -t]
+ nvme compare [--latency | -L]
- nvme read [--latency | -t]
+ nvme read [--latency | -L]
- nvme resv-acquire [--rtype=<rtype> | -t <rtype>]
+ nvme resv-acquire [--rtype=<rtype> | -T <rtype>]
- nvme resv-release [--rtype=<rtype> | -t <rtype>]
+ nvme resv-release [--rtype=<rtype> | -T <rtype>]
- nvme security-recv [--al=<allocation-length> | -t <allocation-length>]
+ nvme security-recv [--al=<allocation-length> | -l <allocation-length>]
- nvme security-recv [--tl=<transfer-length> | -t <transfer-length>]
+ nvme security-recv [--tl=<transfer-length> | -l <transfer-length>]
- nvme write [--latency | -t]
+ nvme write [--latency | -L]

[igaw]

That means we would change existing options once again. The last time it introduced regressions and I promised to avoid this in future. See https://lore.kernel.org/linux-nvme/xfypo2wloir5l3wrw4aodzjekertc4dlnb7dvk2i5egiqwo3ao@rokmuhzvvcfe/

I think we could avoid this by just introducing --timeout on global level and update the existing commands which support the short form also to accept -t. Hopefully this is not getting too messy.

[ikegami-t]

Okay I see so will do consider the suggestion to keep the existing commands. Thank you.

[ikegami-t]

Just fixed the short option change issue as below as changed the default timeout option order so the existing short command option -t works correctly but the timtout option -t duplicated does not work. Also reverted the changes the existing short option -t changes so now not changed from before.

  #define NVME_ARGS(n, ...)                                                              \
  	struct argconfig_commandline_options n[] = {                                   \
  		OPT_INCR("verbose",      'v', &nvme_cfg.verbose,       verbose),       \
  		OPT_FMT("output-format", 'o', &nvme_cfg.output_format, output_format), \
-  		OPT_UINT("timeout",      't', &nvme_cfg.timeout,       timeout),       \
  		##__VA_ARGS__,                                                         \
+  		OPT_UINT("timeout",      't', &nvme_cfg.timeout,       timeout),       \
  		OPT_END()                                                              \
  	}

By the way I think we can fix the short option -o issue mentionsed by the linux-nvme mailing list as same so should we need to fix the issue also?

[igaw]

Sorry for the late response. I was pretty busy and now I am trying to catch up.

Just fixed the short option change issue as below as changed the default timeout option order so the existing short command option -t works correctly but the timtout option -t duplicated does not work. Also reverted the changes the existing short option -t changes so now not changed from before.

Okay, sounds good to me

  #define NVME_ARGS(n, ...)                                                              \
  	struct argconfig_commandline_options n[] = {                                   \
  		OPT_INCR("verbose",      'v', &nvme_cfg.verbose,       verbose),       \
  		OPT_FMT("output-format", 'o', &nvme_cfg.output_format, output_format), \
-  		OPT_UINT("timeout",      't', &nvme_cfg.timeout,       timeout),       \
  		##__VA_ARGS__,                                                         \
+  		OPT_UINT("timeout",      't', &nvme_cfg.timeout,       timeout),       \
  		OPT_END()                                                              \
  	}
``

Maybe it is worth to add a comment why timeout is the last argument. E.g the ordering of the arguments matters, as the argument parser uses the first match, thus any command which defines -t shorthand will match first.

By the way I think we can fix the short option -o issue mentionsed by the linux-nvme mailing list as same so should we need to fix the issue also?

I think we just leave it as it is now. Changing it back will cause again troubles.

BTW, could you rebase the PR? Thanks!

[ikegami-t]

Thanks for your review comments. Let me update the patches on next week since traveling now.

[ikegami-t]

Just fixed the comment and rebased the patches.

[igaw]

Thanks a lot! This was a major cleanup!