Re: [PATCH blktests v2 1/6] common/rc: avoid module load in _have_driver()

[Bart Van Assche <bvanassche@acm.org>]

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.