On Aug 18, 2022 / 18:03, Bart Van Assche wrote:
> On 8/18/22 17:34, Shinichiro Kawasaki wrote:
> > On Aug 18, 2022 / 13:02, Bart Van Assche wrote:
> > > On 8/17/22 18:26, Shin'ichiro Kawasaki wrote:
> > > > The helper function _have_driver() checks availability of the specified
> > > > driver, or module, regardless whether it is loadable or not. When the
> > > > driver is loadable, it loads the module for checking, but does not
> > > > unload it. This makes following test cases fail.
> > > >
> > > > Such failure happens when nvmeof-mp test group is executed after nvme
> > > > test group with tcp transport. _have_driver() for tcp transport loads
> > > > nvmet and nvmet-tcp modules. nvmeof-mp test group tries to unload the
> > > > nvmet module but it fails because of dependency to the nvmet-tcp module.
> > > >
> > > > To avoid the failure, do not load module in _have_driver() using -n
> > > > dry run option of the modprobe command. While at it, fix a minor problem
> > > > of modname '-' replacement. Currently, only the first '-' in modname is
> > > > replaced with '_'. Replace all '-'s.
> > > >
> > > > Fixes: e9645877fbf0 ("common: add a helper if a driver is available")
> > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
> > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > > ---
> > > > common/rc | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/common/rc b/common/rc
> > > > index 01df6fa..8150fee 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -30,9 +30,10 @@ _have_root() {
> > > > _have_driver()
> > > > {
> > > > - local modname="${1/-/_}"
> > > > + local modname="${1//-/_}"
> > > > - if [ ! -d "/sys/module/${modname}" ] && ! modprobe -q "${modname}"; then
> > > > + if [[ ! -d "/sys/module/${modname}" ]] &&
> > > > + ! modprobe -qn "${modname}"; then
> > > > SKIP_REASONS+=("driver ${modname} is not available")
> > > > return 1
> > > > fi
> > >
> > > There are two (unrelated?) changes in this patch but only one change has
> > > been explained in the patch description :-(
> >
> > Hi Bart, thanks for the review. Regarding the local modname="${1//-/_}" change,
> > I explained it at the very end of the patch description with one sentence.
> > However, I admit that it was unclear and confusing. Will separate it to another
> > patch in v3.
>
> Hi Shinichiro,
>
> In my comment I was referring to the other change :-)
>
> (changing [ ... ] into [[ ... ]]).
Ah, I see. Then I will modify this patch to keep the [ ... ] as is. And will
keep this patch single.
--
Shin'ichiro Kawasaki
