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

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Marcos Paulo de Souza <mpdesouza@suse.de>,
	Marcos Paulo de Souza <marcos.souza.org@gmail.com>
Cc: dsterba@suse.com, linux-btrfs@vger.kernel.org,
	Marcos Paulo de Souza <mpdesouza@suse.com>,
	wqu@suse.com
Subject: Re: [btrfs-progs PATCH 4/4] tests: Do not fail is dmsetup is missing
Date: Wed, 18 Dec 2019 08:48:21 +0800	[thread overview]
Message-ID: <a880c99c-982d-135c-4e6e-e82c19e681c1@gmx.com> (raw)
In-Reply-To: <65eb6792b6c9e1c8f58a741866305a6ccb9fee32.camel@suse.de>

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