On Fri, Aug 25, 2023 at 06:45:25AM -0700, Bart Van Assche wrote:

On 8/25/23 00:34, Shinichiro Kawasaki wrote:

quoted

Recently, you actively cleans up tests/nvme/* (which is great!), and introduced
argument parsers in test/nvme/rc. The first one is _nvme_connect_subsys, and the
second one is this _nvme_target_setup. It looks for me this is a bash coding
style change in blktests, from "don't use optional arguments often" to "use
optional arguments aggressively". If we apply this change, we should suppress
SC2119. If we keep the old coding style, we should keep on enabling SC2119. What
I see here is the style difference between you and Bart.

Now I'm tempted to disable SC2119, and to go with the new coding style...

If I have any misunderstanding, or if anyone has more comments on this, please
let me know.

I don't like the "new style". What is so hard about typing "$@" to pass all function
arguments to _nvmet_target_setup()? Leaving out "$@" makes it much harder than
necessary to figure out the intent of the code author - not passing any arguments
or passing all caller arguments implicitly.

Because "$@" is just not correct. Also by using defaults we really see
where the test is special.

Let's look at this here:

 _create_nvmet_subsystem "${def_subsysnqn}" "${def_file_path}"

Both arguments are default values and could just be left out. It makes
reading the code way simpler,

 _create_nvmet_subsystem

Another example, if setup a default target

 _nvmet_target_setup

and if we want to enable the auth code:

 _nvmet_target_setup --ctrlkey "${ctrlkey}" --hostkey "${hostkey}"

and that's all. You can easily see what's is different from the default
values.

The "old" style is expecting that the caller gets the number of
arguments and position correct:

 _create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}" "${hostkey}" "${ctrlkey}"

And this isn't always the case. I already fixed a couple of bugs where
the test got the order wrong.