Re: [PATCH blktests v3 3/3] nvme: introduce nvmet_target_{setup/cleanup} common code

[Shinichiro Kawasaki]

On Aug 25, 2023 / 08:40, Daniel Wagner wrote:
> On Fri, Aug 25, 2023 at 12:53:26AM +0000, Shinichiro Kawasaki wrote:
> > This works, but a bit ugly. Another idea is to make one of the optional
> > arguments mandatory, a positional argument. I think the option --device_type
> > worth making mandatory and explicit, like,
> > 
> >     _nvmet_target_setup device
> >     _nvmet_target_setup file
> 
> Possible but as I said in the other mail, we have a lot more of default
> arguments to functions which I would like to drop too.
> 
> > This will make it easier for me to review which test case uses which type.
> > (This might be against Sagi's comment, though.)
> > 
> > Daniel, what do you think?
> 
> I don't think it is good strategy to change blktests just to make
> ShellCheck happy, when we know it is broken. It rather have SC2119
> ignored until ShellCheck is fixed.

Thanks for the comments.

IMO, SC2119 is not broken. SC2119 (and its companion SC2120) assumes that bash
functions do not have optional arguments. If any functions which refer arguments
are called without arguments, it complains. With the assumption, SC2119 is
useful to detect missing arguments of function calls. (I guess Bart thinks this
is useful.)

However, when we implement argument parsers in bash functions so that the
arguments can be optional, the assumption for the SC2119 is wrong. Then SC2119
reports are useless. Until recently, blktests had few functions with such
optional arguments, such as _init_null_blk or _init_scsi_debug. But most of
calls to those functions had some arguments, and it was rare to call them
without any argument. So SC2119 reports were easy to suppress and not a pain.

    I doubt Shellcheck can be improved and detect if functions have the optional
    argument parsers...

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.