Re: [btrfs-progs PATCH 4/4] tests: Do not fail is dmsetup is missing

Qu Wenruo
2019-12-18T00:48:42Z

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

On 2019/12/18 上午8:44, Marcos Paulo de Souza wrote:
> On Wed, 2019-12-18 at 08:30 +0800, Qu Wenruo wrote:
>>
>> On 2019/12/18 上午4:31, Marcos Paulo de Souza wrote:
>>> From: Marcos Paulo de Souza <mpdesouza@suse.com>
>>>
>>> Move the check of dmsetup to check_dm_target_support, and adapt the
>> only
>>> two places checking if dmsetup is present in the system. Now we
>> skip the
>>> test if dmsetup isn't available, instead of marking the test as
>> failed.
>>>
>>> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
>>
>> Looks good overall, just a small nitpick inlined below.
>>
>>> ---
>>> tests/common | 9
>> +++++++--
>>> tests/mkfs-tests/005-long-device-name-for-ssd/test.sh | 1 -
>>> .../017-small-backing-size-thin-provision-device/test.sh | 1 -
>>> 3 files changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/common b/tests/common
>>> index f138b17e..dc2d084e 100644
>>> --- a/tests/common
>>> +++ b/tests/common
>>> @@ -322,10 +322,15 @@ check_global_prereq()
>>> fi
>>> }
>>>
>>> -# check if the targets passed as arguments are available, and if
>> not just skip
>>> -# the test
>>> +# check if dmsetup and targets passed as arguments are available,
>> and skip the
>>> +# test if they aren't.
>>> check_dm_target_support()
>>> {
>>> + which dmsetup &> /dev/null
>>> + if [ $? -ne 0 ]; then
>>> + _not_run "This test requires dmsetup tool.";
>>> + fi
>>
>> What about using existing check_global_prereq()?
>
> check_global_prereq call _fail when the executable is not found in
> $PATH, that's why I open coded the implementation and just called
> _not_run.

Makes sense.

Although it would be even better to change from "_fail" to "_not_run"
for check_global_prereq().
That could be a new patch.

Thanks,
Qu

>
>>
>> Despite that,
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Thanks,
>> Qu
>>
>>> +
>>> for target in "$@"; do
>>> $SUDO_HELPER modprobe dm-$target >/dev/null 2>&1
>>> $SUDO_HELPER dmsetup targets 2>&1 | grep -q ^$target
>>> diff --git a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh
>> b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh
>>> index 329deaf2..2df88db4 100755
>>> --- a/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh
>>> +++ b/tests/mkfs-tests/005-long-device-name-for-ssd/test.sh
>>> @@ -4,7 +4,6 @@
>>> source "$TEST_TOP/common"
>>>
>>> check_prereq mkfs.btrfs
>>> -check_global_prereq dmsetup
>>> check_dm_target_support linear
>>>
>>> setup_root_helper
>>> diff --git a/tests/mkfs-tests/017-small-backing-size-thin-
>> provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size-
>> thin-provision-device/test.sh
>>> index 91851945..83f34ecc 100755
>>> --- a/tests/mkfs-tests/017-small-backing-size-thin-provision-
>> device/test.sh
>>> +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision-
>> device/test.sh
>>> @@ -6,7 +6,6 @@ source "$TEST_TOP/common"
>>>
>>> check_prereq mkfs.btrfs
>>> check_global_prereq udevadm
>>> -check_global_prereq dmsetup
>>> check_dm_target_support linear thin
>>>
>>> setup_root_helper
>>>
>>
>

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