Re: [PATCH blktests 5/9] common: do not require null_blk support to be modular

From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To: Christoph Hellwig <hch@lst.de>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>
Subject: Re: [PATCH blktests 5/9] common: do not require null_blk support to be modular
Date: Tue, 31 May 2022 12:04:55 +0000	[thread overview]
Message-ID: <20220531120454.3hlprbotkc3ftyyf@shindev> (raw)
In-Reply-To: <20220530130811.3006554-6-hch@lst.de>

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Use _have_driver instead of _have_modules in _have_null_blk for the basic
> null_blk check, and instead only require an actual module in
> _init_null_blk when specific module parameters are passed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/null_blk | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/common/null_blk b/common/null_blk
> index 6611db0..ccf3750 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -5,7 +5,7 @@
>  # null_blk helper functions.
>  
>  _have_null_blk() {
> -	_have_modules null_blk
> +	_have_driver null_blk
>  }
>  
>  _remove_null_blk_devices() {
> @@ -16,15 +16,19 @@ _remove_null_blk_devices() {
>  }
>  
>  _init_null_blk() {
> -	_remove_null_blk_devices
> +	local modparams="$@"

Shellcheck complains on this line:
common/null_blk:19:18: warning: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. [SC2124]

I think array would be the better.

>  
> -	local zoned=""
> -	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
> +	if (( RUN_FOR_ZONED )); then
> +		modparams="${modparams} zoned=1"
> +	fi
>  
> -	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${zoned}" ; then
> +	if [ -n "${modparams}" ] && ! _have_modules null_blk; then

I tried this part on my test machine and it looks that '_have_modules null_blk'
returns 0=success even when null_blk module is built-in. This may not be what
you intended here. I think _have_modules needs modification so that it returns
1=failure when one of the modules is built-in. (At least until all null_blk
parameters get supported by configfs.)

>  		return 1
>  	fi
>  
> +	_remove_null_blk_devices
> +	modprobe -qr null_blk && modprobe -q null_blk ${modparams}

Double quotes for ${modparams} is required for shellcheck.

-- 
Shin'ichiro Kawasaki