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

Qu Wenruo
2019-12-18T00:26:23Z

[-- 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 --]