| Message ID | 20231011072530.1659810-1-yi.zhang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests,V2] check: define TMPDIR earlier in _run_group | expand |
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
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
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.
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"
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(-)