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.
From b86f26b4772ef5a458685fbf7dca14a50551a815 Mon Sep 17 00:00:00 2001
From: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
Date: Fri, 29 Nov 2024 10:48:31 +0900
Subject: [PATCH] nvme/{041-045,051}: double quote def_* variable references
The following commit will add the code to set nvme command output to
def_* variables. This will trigger the ShellCheck warning SC2086. To
prepare for the change, double quote the references to the def_*
variables. As for nvme/051, the local variable ns is initialized with
def_subsysnqn, then it requires double quote also.
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx>
---
tests/nvme/041 | 2 +-
tests/nvme/042 | 4 ++--
tests/nvme/043 | 2 +-
tests/nvme/044 | 4 ++--
tests/nvme/045 | 8 ++++----
tests/nvme/051 | 4 ++--
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/nvme/041 b/tests/nvme/041
index aa44f04..94f84f1 100755
--- a/tests/nvme/041
+++ b/tests/nvme/041
@@ -31,7 +31,7 @@ test() {
local hostkey
local ctrldev
- hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "nvme gen-dhchap-key failed"
return 1
diff --git a/tests/nvme/042 b/tests/nvme/042
index 70c9056..17d8a73 100755
--- a/tests/nvme/042
+++ b/tests/nvme/042
@@ -37,7 +37,7 @@ test() {
for hmac in 0 1 2 3; do
echo "Testing hmac ${hmac}"
- hostkey="$(nvme gen-dhchap-key --hmac=${hmac} -n ${def_subsysnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key --hmac=${hmac} -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "couldn't generate host key for hmac ${hmac}"
return 1
@@ -51,7 +51,7 @@ test() {
for key_len in 32 48 64; do
echo "Testing key length ${key_len}"
- hostkey="$(nvme gen-dhchap-key --key-length=${key_len} -n ${def_subsysnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key --key-length=${key_len} -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "couldn't generate host key for length ${key_len}"
return 1
diff --git a/tests/nvme/043 b/tests/nvme/043
index cf99865..7f9e67e 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -34,7 +34,7 @@ test() {
local hostkey
local ctrldev
- hostkey="$(nvme gen-dhchap-key -n ${def_hostnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key -n "${def_hostnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "nvme gen-dhchap-key failed"
return 1
diff --git a/tests/nvme/044 b/tests/nvme/044
index 9ed46c9..7c08328 100755
--- a/tests/nvme/044
+++ b/tests/nvme/044
@@ -33,13 +33,13 @@ test() {
local ctrlkey
local ctrldev
- hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "failed to generate host key"
return 1
fi
- ctrlkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+ ctrlkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$ctrlkey" ] ; then
echo "failed to generate ctrl key"
return 1
diff --git a/tests/nvme/045 b/tests/nvme/045
index be81316..4dd0f94 100755
--- a/tests/nvme/045
+++ b/tests/nvme/045
@@ -38,13 +38,13 @@ test() {
local rand_io_size
local ns
- hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+ hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$hostkey" ] ; then
echo "failed to generate host key"
return 1
fi
- ctrlkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+ ctrlkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
if [ -z "$ctrlkey" ] ; then
echo "failed to generate ctrl key"
return 1
@@ -69,7 +69,7 @@ test() {
echo "Renew host key on the controller"
- new_hostkey="$(nvme gen-dhchap-key --nqn ${def_subsysnqn} 2> /dev/null)"
+ new_hostkey="$(nvme gen-dhchap-key --nqn "${def_subsysnqn}" 2> /dev/null)"
_set_nvmet_hostkey "${def_hostnqn}" "${new_hostkey}"
@@ -79,7 +79,7 @@ test() {
echo "Renew ctrl key on the controller"
- new_ctrlkey="$(nvme gen-dhchap-key --nqn ${def_subsysnqn} 2> /dev/null)"
+ new_ctrlkey="$(nvme gen-dhchap-key --nqn "${def_subsysnqn}" 2> /dev/null)"
_set_nvmet_ctrlkey "${def_hostnqn}" "${new_ctrlkey}"
diff --git a/tests/nvme/051 b/tests/nvme/051
index 4757b80..323fac7 100755
--- a/tests/nvme/051
+++ b/tests/nvme/051
@@ -37,8 +37,8 @@ test() {
# fire off two enable/disable loops concurrently and wait
# for them to complete...
- ns_enable_disable_loop $ns &
- ns_enable_disable_loop $ns &
+ ns_enable_disable_loop "$ns" &
+ ns_enable_disable_loop "$ns" &
wait
_nvmet_target_cleanup
--
2.46.2
