[PATCH blktests] nvme/058: detach loop device after test finish

[Shinichiro Kawasaki]

On Jan 16, 2025 / 18:52, Nilay Shroff wrote:
> The nvme/058 creates three (temp file backed) namespaces and
> attach each namespace to a loop device while starting the test.
> However it never detach those namespaces from the loop device
> once test finishes. Ideally, we should detach loop device from
> namespace so that the associated loop device is later destroyed
> and its resources are released. This patch helps detach each
> namespace from its associated loop device after test finishes.

Thank you for catching this issue. This needs a fix.

> 
> Signed-off-by: Nilay Shroff <nilay at linux.ibm.com>
> ---
>  tests/nvme/058 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/nvme/058 b/tests/nvme/058
> index d230a21..99e7e81 100755
> --- a/tests/nvme/058
> +++ b/tests/nvme/058
> @@ -99,6 +99,17 @@ test() {
>  	done
>  
>  	_nvme_disconnect_subsys
> +
> +	for ((d = 1; d <= num_namespaces; d++)); do
> +		local file_path
> +		local blkdev
> +
> +		file_path="${TMPDIR}/img${d}"
> +		blkdev="$(losetup -l | awk -v path="${file_path}" '$6 == path { print $1 }')"
> +
> +		losetup -d "${blkdev}"
> +	done

This for loop looks similar and redundant with the other for loop to set up the
namespaces. How about to keep the blkdevs in an array, and refer to them to
detach? The pseudo code below shows this approach:

 local blkdev
 local -a blkdevs
  ...
 (in the 1st loop)
    ...
    blkdevs+=$("$blkdev")
    ...

  ...
  (after _nvme_disconnect_subsys)

  for blkdev in "${blkdevs[@]}"; do
      losetup --detach "$blkdev"
  done
  ...


Also I suggest to use long option names instead of short names for readability
and code stability: e.g., instead of "losetup -d", let's use "losetup --detach".
(No need to replace short option names in existing code. I'm suggesting this for
the new lines to add.)

> +
>  	_nvmet_target_cleanup
>  
>  	echo "Test complete"
> -- 
> 2.47.1
>