Re: [PATCH blktests v2 2/2] block/031: allow to run with built-in null_blk driver

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

On 1/9/24 02:44, Shin'ichiro Kawasaki wrote:
> The test case block/031 sets null_blk parameter shared_tag_bitmap=1 for
> testing.
> The parameter has been set as a module parameter then null_blk
> driver must be loadable.

It seems like the word "If" is missing from the start of the above sentence?

> -	if ! _init_null_blk nr_devices=0 shared_tag_bitmap=1; then
> -		echo "Loading null_blk failed"
> -		return 1
> +	if _have_null_blk_feature shared_tag_bitmap; then
> +		opts+=(shared_tag_bitmap=1)
> +	else
> +		# Old kernel requires shared_tag_bitmap as a module parameter
> +		# instead of configfs parameter.
> +		if ! _init_null_blk shared_tag_bitmap=1; then
> +			echo "Loading null_blk failed"
> +			return 1
> +		fi
>   	fi

_have_null_blk_feature loads the null_blk kernel module as a side effect. The
above code relies on that side effect. I think that _have_null_blk_feature either
should unload the null_blk kernel module or that a comment should be added above
the above if-statement that explains this side effect. Otherwise readers of this
code will wonder why there is an _init_null_blk call in one branch of the
if-statement and not in the other branch.

Thanks,

Bart.