diff mbox series

[blktests,1/2] nvme/{rc,017,031}: replace def_file_path with _nvme_def_file_path()

Message ID 20231012021152.832553-2-shinichiro.kawasaki@wdc.com (mailing list archive)
State New, archived
Headers show
Series TMPDIR fixes | expand

Commit Message

Shin'ichiro Kawasaki Oct. 12, 2023, 2:11 a.m. UTC
The commit b6356f6 ("nvme/rc: Add common file_path name define") defined
a global variable 'def_file_path' in nvme/rc, which refers TMPDIR.
However, when nvme/rc is sourced and def_file_path is defined for the
nvme test group, TMPDIR is not yet defined since TMPDIR is defined for
each test case. Then an unexpected path is set to def_file_path and
temporary files are created at the unexpected path.

Fix this by replacing the global variable def_file_path with a helper
function _nvme_def_file_path(). This helper function allows to refer
TMPDIR not at nvme/rc source timing but in test() or test_device()
context of each test case.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Fixes: b6356f6 ("nvme/rc: Add common file_path name define")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/nvme/017 |  9 +++++----
 tests/nvme/031 |  6 +++---
 tests/nvme/rc  | 17 +++++++++++------
 3 files changed, 19 insertions(+), 13 deletions(-)

Comments

Daniel Wagner Oct. 12, 2023, 5:57 a.m. UTC | #1
On Thu, Oct 12, 2023 at 11:11:51AM +0900, Shin'ichiro Kawasaki wrote:
> The commit b6356f6 ("nvme/rc: Add common file_path name define") defined
> a global variable 'def_file_path' in nvme/rc, which refers TMPDIR.
> However, when nvme/rc is sourced and def_file_path is defined for the
> nvme test group, TMPDIR is not yet defined since TMPDIR is defined for
> each test case. Then an unexpected path is set to def_file_path and
> temporary files are created at the unexpected path.
> 
> Fix this by replacing the global variable def_file_path with a helper
> function _nvme_def_file_path(). This helper function allows to refer
> TMPDIR not at nvme/rc source timing but in test() or test_device()
> context of each test case.
> 
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Fixes: b6356f6 ("nvme/rc: Add common file_path name define")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Reviewed-by: Daniel Wagner <dwagern@suse.de>

Thanks,
Daniel
Yi Zhang Oct. 12, 2023, 6 a.m. UTC | #2
On Thu, Oct 12, 2023 at 10:12 AM Shin'ichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> The commit b6356f6 ("nvme/rc: Add common file_path name define") defined
> a global variable 'def_file_path' in nvme/rc, which refers TMPDIR.
> However, when nvme/rc is sourced and def_file_path is defined for the
> nvme test group, TMPDIR is not yet defined since TMPDIR is defined for
> each test case. Then an unexpected path is set to def_file_path and
> temporary files are created at the unexpected path.
>
> Fix this by replacing the global variable def_file_path with a helper
> function _nvme_def_file_path(). This helper function allows to refer
> TMPDIR not at nvme/rc source timing but in test() or test_device()
> context of each test case.
>
> Reported-by: Yi Zhang <yi.zhang@redhat.com>
> Fixes: b6356f6 ("nvme/rc: Add common file_path name define")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

LGTM, thanks for the fix.

Reviewed-by: Yi Zhang <yi.zhang@redhat.com>
diff mbox series

Patch

diff --git a/tests/nvme/017 b/tests/nvme/017
index aa0a3fe..c8d9b32 100755
--- a/tests/nvme/017
+++ b/tests/nvme/017
@@ -21,15 +21,16 @@  test() {
 	local port
 	local iterations="${nvme_num_iter}"
 
-	truncate -s "${nvme_img_size}" "${def_file_path}"
+	truncate -s "${nvme_img_size}" "$(_nvme_def_file_path)"
 
 	local genctr=1
 
-	_create_nvmet_subsystem "${def_subsysnqn}" "${def_file_path}" \
+	_create_nvmet_subsystem "${def_subsysnqn}" "$(_nvme_def_file_path)" \
 		"${def_subsys_uuid}"
 
 	for ((i = 2; i <= iterations; i++)); do
-		_create_nvmet_ns "${def_subsysnqn}" "${i}" "${def_file_path}"
+		_create_nvmet_ns "${def_subsysnqn}" "${i}" \
+				 "$(_nvme_def_file_path)"
 	done
 
 	port="$(_create_nvmet_port "${nvme_trtype}")"
@@ -46,7 +47,7 @@  test() {
 
 	_remove_nvmet_subsystem "${def_subsysnqn}"
 
-	rm "${def_file_path}"
+	rm "$(_nvme_def_file_path)"
 
 	echo "Test complete"
 }
diff --git a/tests/nvme/031 b/tests/nvme/031
index 696db2d..ed5f196 100755
--- a/tests/nvme/031
+++ b/tests/nvme/031
@@ -33,9 +33,9 @@  test() {
 	local loop_dev
 	local port
 
-	truncate -s "${nvme_img_size}" "${def_file_path}"
+	truncate -s "${nvme_img_size}" "$(_nvme_def_file_path)"
 
-	loop_dev="$(losetup -f --show "${def_file_path}")"
+	loop_dev="$(losetup -f --show "$(_nvme_def_file_path)")"
 
 	port="$(_create_nvmet_port "${nvme_trtype}")"
 
@@ -52,7 +52,7 @@  test() {
 
 	_remove_nvmet_port "${port}"
 	losetup -d "$loop_dev"
-	rm "${def_file_path}"
+	rm "$(_nvme_def_file_path)"
 
 	echo "Test complete"
 }
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 1ec9eb6..012ca95 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -18,11 +18,16 @@  def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
 def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
 export def_subsysnqn="blktests-subsystem-1"
 export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e"
-export def_file_path="${TMPDIR}/img"
 nvme_trtype=${nvme_trtype:-"loop"}
 nvme_img_size=${nvme_img_size:-"1G"}
 nvme_num_iter=${nvme_num_iter:-"1000"}
 
+# TMPDIR can not be referred out of test() or test_device() context. Instead of
+# global variable def_flie_path, use this getter function.
+_nvme_def_file_path() {
+	echo "${TMPDIR}/img"
+}
+
 _nvme_requires() {
 	_have_program nvme
 	_require_nvme_test_img_size 4m
@@ -300,11 +305,11 @@  _cleanup_blkdev() {
 	local blkdev
 	local dev
 
-	blkdev="$(losetup -l | awk '$6 == "'"${def_file_path}"'" { print $1 }')"
+	blkdev="$(losetup -l | awk '$6 == "'"$(_nvme_def_file_path)"'" { print $1 }')"
 	for dev in ${blkdev}; do
 		losetup -d "${dev}"
 	done
-	rm -f "${def_file_path}"
+	rm -f "$(_nvme_def_file_path)"
 }
 
 _cleanup_nvmet() {
@@ -859,11 +864,11 @@  _nvmet_target_setup() {
 		esac
 	done
 
-	truncate -s "${nvme_img_size}" "${def_file_path}"
+	truncate -s "${nvme_img_size}" "$(_nvme_def_file_path)"
 	if [[ "${blkdev_type}" == "device" ]]; then
-		blkdev="$(losetup -f --show "${def_file_path}")"
+		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
 	else
-		blkdev="${def_file_path}"
+		blkdev="$(_nvme_def_file_path)"
 	fi
 
 	_create_nvmet_subsystem "${def_subsysnqn}" "${blkdev}" \