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

Bart Van Assche
2024-01-09T20:19:25Z

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.