| Message ID | 20220818012624.71544-2-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix module check issues | expand |
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@wdc.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > 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 :-( Bart.
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@wdc.com> > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > --- > > 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.
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@wdc.com> >>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>> --- >>> 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 [[ ... ]]). Best regards, Bart.
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@wdc.com> > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > --- > > > > 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.
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
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@wdc.com> Reviewed-by: Christoph Hellwig <hch@lst.de> --- common/rc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)