Re: [PATCH v2 3/3] nvme: add test for controller rescan under I/O load

[Martin Wilck <mwilck@suse.com>]

On Tue, 2024-09-03 at 00:16 +0000, Shinichiro Kawasaki wrote:
> On Sep 02, 2024 / 15:53, Martin Wilck wrote:
> > On Thu, 2024-08-29 at 07:36 +0000, Shinichiro Kawasaki wrote:
> > 
> > 
> > > Shellcheck reports a warning here:
> > > 
> > >   tests/nvme/053:47:9: warning: Prefer mapfile or read -a to
> > > split
> > > command output (or quote to avoid splitting). [SC2207]
> > > 
> > > It is a bit lengthy, but let's replace the line above with this:
> > > 
> > >         while read -r line ; do ctrls+=("$line"); done <
> > > <(_nvme_get_ctrl_list)
> > 
> > Thanks for the review. If you don't mind, I'll just use "mapfile -
> > t",
> > as suggested by shellcheck.
> 
> The reason I did not suggest the "mapfile -t" solution was that
> SC2207
> recommends it for bash versions 4.4+ [1]. On the other hand, blktests
> README.md
> requires bash version >= 4.2. So I assume that some blktests users
> use bash
> version 4.2 or 4.3. I took a glance in the bash change history [2],
> and there
> are some mpafile related bug fixes between bash version v4.2 an v4.4.
> So I think
> it's the better to not use "mpafile -t" in blktests.

Sorry, I forgot the requirement to be compatible with bash 4.2.

> Do you have any specific reason to use "mapfile -t"?

Just the fact that the code looks simpler and more elegant.

Side note: as we know that _nvme_get_ctrl_list() prints paths from
sysfs, and that these won't contain spaces or glob characters, we might
as well just ignore SC2207 here.

Anyway, I agree that it's better to be on the safe side.
I'll send a new version.

Thanks,
Martin