mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Bart Van Assche <bvanassche@acm.org>
To: Daniel Wagner <dwagner@suse.de>
Cc: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Hannes Reinecke <hare@suse.de>, Sagi Grimberg <sagi@grimberg.me>,
	Jason Gunthorpe <jgg@ziepe.ca>
Subject: Re: [PATCH blktests v3 3/3] nvme: introduce nvmet_target_{setup/cleanup} common code
Date: Mon, 28 Aug 2023 11:55:01 -0700	[thread overview]
Message-ID: <ed518b56-a579-49bb-b2bb-220214ef6e2e@acm.org> (raw)
In-Reply-To: <txsyjpcxb3baog5fgqdnm5hh765nt5qcbcmllsto7uulyzr5kn@zirmqldhosbi>

On 8/28/23 10:02, Daniel Wagner wrote:
> I am against adding code just to make ShellCheck happy.
Hi Daniel,

That's not what my concern is about. My concern is about keeping
the blktests source code maintainable and easy to read. My opinion
is that the ability of bash to pass arguments from caller to callee
implicitly (a) hurts readability, (b) is error prone and (c) hurts
maintainability. This is why I think that this feature should not
be used and hence that disabling SC2119 would be really wrong.

Regarding (a), I think this long e-mail thread is more than enough
evidence that it is not clear what the intention is of the
_nvmet_target_setup calls without arguments - not pass any arguments
or pass the argument list of the caller. Regarding (c): if any
_nvmet_target_setup calls would be added in a function that accepts
arguments, how is _nvmet_target_setup() expected to process arguments
of which it doesn't know how to interpret these?

Hence my proposal to change the _nvmet_target_setup calls with no
arguments into "_nvmet_target_setup --" and also to ignore the double
hyphen argument inside _nvmet_target_setup().

Thanks,

Bart.

  reply	other threads:[~2023-08-28 18:56 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-22  8:38 [PATCH blktests v3 0/3] Introduce nvmet target setup/cleanup helpers Daniel Wagner
2023-08-22  8:38 ` [PATCH blktests v3 1/3] nvme/{033,034,035,036}: use default subsysnqn variable directly Daniel Wagner
2023-08-22  8:38 ` [PATCH blktests v3 2/3] nvme/{033,034,035,036,37}: drop port handle between passthru target setup and cleanup Daniel Wagner
2023-08-22  8:38 ` [PATCH blktests v3 3/3] nvme: introduce nvmet_target_{setup/cleanup} common code Daniel Wagner
2023-08-24  3:09   ` Shinichiro Kawasaki
2023-08-24 14:36     ` Bart Van Assche
2023-08-25  0:53       ` Shinichiro Kawasaki
2023-08-25  6:40         ` Daniel Wagner
2023-08-25  7:34           ` Shinichiro Kawasaki
2023-08-25 11:29             ` Daniel Wagner
2023-08-25 13:45             ` Bart Van Assche
2023-08-25 14:26               ` Daniel Wagner
2023-08-25 16:46                 ` Bart Van Assche
2023-08-28  4:13                   ` Shinichiro Kawasaki
2023-08-28 15:14                     ` Bart Van Assche
2023-08-28 17:02                       ` Daniel Wagner
2023-08-28 18:55                         ` Bart Van Assche [this message]
2023-08-29  2:11                           ` Shinichiro Kawasaki
2023-08-29 13:35                             ` Bart Van Assche
2023-08-30  1:19                               ` Shinichiro Kawasaki
2023-09-01  0:33                                 ` Shinichiro Kawasaki
2023-08-25  6:24       ` Daniel Wagner
2023-08-28  4:17   ` Shinichiro Kawasaki
2023-08-28 15:14   ` Bart Van Assche
2023-08-28 17:03     ` Daniel Wagner
2023-08-22 17:54 ` [PATCH blktests v3 0/3] Introduce nvmet target setup/cleanup helpers Chaitanya Kulkarni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed518b56-a579-49bb-b2bb-220214ef6e2e@acm.org \
    --to=bvanassche@acm.org \
    --cc=dwagner@suse.de \
    --cc=hare@suse.de \
    --cc=jgg@ziepe.ca \
    --cc=kch@nvidia.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    --cc=shinichiro.kawasaki@wdc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®