[PATCH blktests v4 1/5] nvme/rc: introduce remote target support

[Shinichiro Kawasaki]

Hello Aurelien,

Thank you for rebasing the series. Please find my comments in line.

When I ran "make check", I observed some ShellCheck warnings. This check before
posts will be appreciated. As for the two ShellCheck warnings, please find my
comments below. This patch also triggered ShellCheck warnings in other files.
For those warnings, I created a fix patch and attached for your reference.

On Nov 26, 2024 / 22:38, Aurelien Aptel wrote:
[...]
> diff --git a/common/nvme b/common/nvme
> index fd472fe..f99af5a 100644
> --- a/common/nvme
> +++ b/common/nvme
[...]
> @@ -208,6 +213,18 @@ _cleanup_nvmet() {
>  
>  _setup_nvmet() {
>  	_register_test_cleanup _cleanup_nvmet
> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
> +		def_hostid="$(${nvme_target_control} config --show-hostid)"
> +		def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"

I suggest to remove the line above. It caused ShellCheck warning SC2034. I think
def_host_traddr is not used anywhere.

> +		def_traddr="$(${nvme_target_control} config --show-traddr)"
> +		def_trsvcid="$(${nvme_target_control} config --show-trsvid)"
> +		def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)"
> +		def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)"
> +		return
> +	fi
> +
>  	modprobe -q nvmet
>  	if [[ "${nvme_trtype}" != "loop" ]]; then
>  		modprobe -q nvmet-"${nvme_trtype}"
[...]
> @@ -811,6 +836,29 @@ _nvmet_target_setup() {
>  		fi
>  	fi
>  
> +	if [[ -n "${hostkey}" ]]; then
> +		ARGS+=(--hostkey "${hostkey}")
> +	fi
> +	if [[ -n "${ctrlkey}" ]]; then
> +		ARGS+=(--ctrkey "${ctrlkey}")
> +	fi

This part above sets arguments --hostkey and --ctrkey in ARGS to pass to
_create_nvmet_subsystem(), but I find that _create_nvmet_subsystem() does not
refer to the arguments. Though I know this part was in v3 also, I suggest drop
this part.

> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" setup \
> +			--subsysnqn "${subsysnqn}" \
> +			--subsys-uuid "${subsys_uuid:-$def_subsys_uuid}" \
> +			--hostnqn "${def_hostnqn}" \
> +			"${ARGS[@]}" &> /dev/null

The line above causes the ShellCheck warning SC 2294. Let's replace ${ARGS[@]}
with ${ARGS[*]}.

> +		return
> +	fi
> +
> +	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
> +	if [[ "${blkdev_type}" == "device" ]]; then
> +		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
> +	else
> +		blkdev="$(_nvme_def_file_path)"
> +	fi

This truncate and blkdev setup part causes failure of nvme/052:

nvme/052 (tr=loop) (Test file-ns creation/deletion under one subsystem) [failed]
    runtime  5.728s  ...  5.267s
    --- tests/nvme/052.out      2024-11-05 17:04:40.596903603 +0900
    +++ /home/shin/Blktests/blktests/results/nodev_tr_loop/nvme/052.out.bad     2024-11-29 14:45:23.065861316 +0900
    @@ -1,2 +1,4 @@
     Running nvme/052
    +mkdir: cannot create directory ‘/sys/kernel/config/nvmet//subsystems/blktests-subsystem-1/namespaces/1’: File exists
    +common/nvme: line 554: printf: write error: Device or resource busy
     Test complete

Also, this part looks duplicated with the other part in _nvmet_target_setup().
Please see the 'if [[ "${blkdev_type}" != "none" ]]' block.

I guess this is the part you added "to specify the backing block device on the
target, instead of hardcoding '/dev/vdc'". If so, I think such changes should
be done under 'if [[ -n "${nvme_target_control}" ]]' condition.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-nvme-041-045-051-double-quote-def_-variable-referenc.patch
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20241129/e9f0d196/attachment-0001.ksh>