[PATCH blktests V2] check: define TMPDIR earlier in _run_group

[PATCH blktests V2] check: define TMPDIR earlier in _run_group

[Yi Zhang]

The TMPDIR was defined in _call_test before running test_func, but it
was used in nvme/rc which has not yet defined, so move the definiation
before calling tests/${group}/rc in _run_group.

Fixes: b6356f6 ("nvme/rc: Add common file_path name define")
Signed-off-by: Yi Zhang <yi.zhang@redhat.com>
---
 check | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/check b/check
index 55871b0..99d8a69 100755
--- a/check
+++ b/check
@@ -364,9 +364,6 @@ _call_test() {
 
 		unset TEST_CLEANUP
 		trap _cleanup EXIT
-		if ! TMPDIR="$(mktemp --tmpdir -p "$OUTPUT" -d "tmpdir.${TEST_NAME//\//.}.XXX")"; then
-			return
-		fi
 
 		TIMEFORMAT="%Rs"
 		pushd . >/dev/null || return
@@ -559,6 +556,10 @@ _run_group() {
 	local tests=("$@")
 	local group="${tests["0"]%/*}"
 
+	if ! TMPDIR="$(mktemp --tmpdir -p "$OUTPUT" -d "tmpdir.${TEST_NAME//\//.}.XXX")"; then
+		return
+	fi
+
 	# shellcheck disable=SC1090
 	. "tests/${group}/rc"
 
-- 
2.34.3

Re: [PATCH blktests V2] check: define TMPDIR earlier in _run_group

[Daniel Wagner]

On Wed, Oct 11, 2023 at 03:25:30PM +0800, Yi Zhang wrote:
 @@ -559,6 +556,10 @@ _run_group() {
>  	local tests=("$@")
>  	local group="${tests["0"]%/*}"
>  
> +	if ! TMPDIR="$(mktemp --tmpdir -p "$OUTPUT" -d "tmpdir.${TEST_NAME//\//.}.XXX")"; then
> +		return
> +	fi
> +
>  	# shellcheck disable=SC1090
>  	. "tests/${group}/rc"

Sorry, I didn't catch this earlier. TMPDIR is newly created for every
single test run and gets removed afterwards, see the _cleanup function.

I think we should keep this behavior. So the question is if we could
make the $def_file_path evaluation just lazy. So something like:

modified   tests/nvme/rc
@@ -18,12 +18,15 @@ 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"}

 _nvme_requires() {
+	# lazy evaluation because TMPDIR is per test run and not
+	# per test group
+	def_file_path="${TMPDIR}/img"
+
 	_have_program nvme
 	_require_nvme_test_img_size 4m
 	case ${nvme_trtype} in

Re: [PATCH blktests V2] check: define TMPDIR earlier in _run_group

[Shinichiro Kawasaki]

Yi, thank you for catching this bug. The nvme image files are now created
at unexpected place, which is bad.

On Oct 11, 2023 / 09:58, Daniel Wagner wrote:
> On Wed, Oct 11, 2023 at 03:25:30PM +0800, Yi Zhang wrote:
>  @@ -559,6 +556,10 @@ _run_group() {
> >  	local tests=("$@")
> >  	local group="${tests["0"]%/*}"
> >  
> > +	if ! TMPDIR="$(mktemp --tmpdir -p "$OUTPUT" -d "tmpdir.${TEST_NAME//\//.}.XXX")"; then
> > +		return
> > +	fi
> > +
> >  	# shellcheck disable=SC1090
> >  	. "tests/${group}/rc"
> 
> Sorry, I didn't catch this earlier. TMPDIR is newly created for every
> single test run and gets removed afterwards, see the _cleanup function.
> 
> I think we should keep this behavior. So the question is if we could
> make the $def_file_path evaluation just lazy. So something like:
> 
> modified   tests/nvme/rc
> @@ -18,12 +18,15 @@ 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"}
> 
>  _nvme_requires() {
> +	# lazy evaluation because TMPDIR is per test run and not
> +	# per test group
> +	def_file_path="${TMPDIR}/img"
> +

_nvme_requires() is called from _run_test() via requires(). This is before
_call_test() which prepares TMPDIR. I think _setup_nvmet() could be the good
place to set def_file_path. All nvme test cases call it in test(), except
nvme/039.

>  	_have_program nvme
>  	_require_nvme_test_img_size 4m
>  	case ${nvme_trtype} in

Re: [PATCH blktests V2] check: define TMPDIR earlier in _run_group

[Shinichiro Kawasaki]

On Oct 11, 2023 / 21:15, Shin'ichiro Kawasaki wrote:
[...]
> _nvme_requires() is called from _run_test() via requires(). This is before
> _call_test() which prepares TMPDIR. I think _setup_nvmet() could be the good
> place to set def_file_path. All nvme test cases call it in test(), except
> nvme/039.

I rethought my comment above. Now it does not look good to have the exception
for nvme/039. As another idea, I suggest to replace the global variable
def_file_path with a helper function so that the TMPDIR reference happens in
test() or test_device() context. I posted this idea as a patch (the first patch
in the two patches series). Comments on the patch will be welcome.