Re: [btrfs-progs PATCH 1/4] tests: common: Add check_dm_target_support helper

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1535 bytes --]



On 2019/12/18 上午4:31, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> This function will be used later to test if dm-thin is supported.
> Inspired by fstests.
> 
> Suggested-by: Qu Wenruo <wqu@suse.com>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>

Looks mostly fine, but a small nitpick for the SUDO_HELPER usage.

> ---
>  tests/common | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/tests/common b/tests/common
> index ca098444..f138b17e 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -322,6 +322,19 @@ check_global_prereq()
>  	fi
>  }
>  
> +# check if the targets passed as arguments are available, and if not just skip
> +# the test
> +check_dm_target_support()
> +{
> +	for target in "$@"; do
> +		$SUDO_HELPER modprobe dm-$target >/dev/null 2>&1

To utilize $SUDO_HELPER, we need to call setup_root_helper() in the
first place, just like all the other $SUDO_HELPER users in `tests/common`.

Although nowadays it feels a little unnecessary, since the functionality
is introduced because I'm a lazybone who doesn't bother to startup a VM
to do proper test, but uses current unprivileged user to utilize self tests.

Maybe it's time to get rid of SUDO_HELPER ?

Thanks,
Qu

> +		$SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target
> +		if [ $? -ne 0 ]; then
> +			_not_run "This test requires dm $target support."
> +		fi
> +	done
> +}
> +
>  check_image()
>  {
>  	local image
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]