Re: [PATCH v2 1/6] fstests: btrfs: add functions to set and reset required number of SCRATCH_DEV_POOL

[Anand Jain <anand.jain@oracle.com>]

On 06/21/2016 09:20 PM, Eryu Guan wrote:
> On Wed, Jun 15, 2016 at 04:46:03PM +0800, Anand Jain wrote:
>> From: Anand Jain <Anand.Jain@oracle.com>
>>
>> This patch provides functions
>>  _scratch_dev_pool_get()
>>  _scratch_dev_pool_put()
>>
>> Which will help to set/reset SCRATCH_DEV_POOL with the required
>> number of devices. SCRATCH_DEV_POOL_SAVED will hold all the devices.
>>
>> Usage:
>>   _scratch_dev_pool_get() <ndevs>
>>   :: do stuff
>>
>>   _scratch_dev_pool_put()
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>
> I think the helpers should be introduced when they are first used, so
> that we know why they're introduced, and know how they're used with
> clear examples.
>
> So patch 1, 2 and 4 can be merged as one patch, patch 3 updates
> btrfs/027, patch 4 and patch 5 can be merged as one patch, then comes
> patch 6.
>
> What do you think?

  Hm. I won't bother much on that. will do if needed.
  (just a note- On the other hand it gets noticed about the
  new helper function if its a separate patch for helper
  function).

>> ---
>>  v2: Error message and comment section updates.
>>
>>  common/rc | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>>
>> diff --git a/common/rc b/common/rc
>> index 51092a0644f0..31c46ba1226e 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -802,6 +802,61 @@ _scratch_mkfs()
>>      esac
>>  }
>>
>> +#
>> +# Generally test cases will have..
>> +#   _require_scratch_dev_pool X
>> +# to make sure it has the enough scratch devices including
>> +# replace-target and spare device. Now arg1 here is the
>> +# required number of scratch devices by a-test-case excluding
>> +# the replace-target and spare device. So this function will
>> +# set SCRATCH_DEV_POOL to the specified number of devices.
>> +#
>> +# Usage:
>> +#  _scratch_dev_pool_get() <ndevs>
>> +#     :: do stuff
>> +#
>> +#  _scratch_dev_pool_put()
>> +#
>> +_scratch_dev_pool_get()
>> +{
>> +	if [ $# != 1 ]; then
>
> "-ne" "-eq" etc. are used for comparing integers, "=!" "==" are for
> strings. And I think $#, $? are integers here, "-ne" would be better.

  Thanks for the catch. fixed in next rev.

- Anand


> Thanks,
> Eryu
>
>> +		_fail "Usage: _scratch_dev_pool_get ndevs"
>> +	fi
>> +
>> +	local test_ndevs=$1
>> +	local config_ndevs=`echo $SCRATCH_DEV_POOL| wc -w`
>> +	local devs[]="( $SCRATCH_DEV_POOL )"
>> +
>> +	typeset -p config_ndevs >/dev/null 2>&1
>> +	if [ $? != 0 ]; then
>> +		_fail "Bug: cant find SCRATCH_DEV_POOL ndevs"
>> +	fi
>> +
>> +	if [ $config_ndevs -lt $test_ndevs ]; then
>> +		_notrun "Need at least test requested number of ndevs $test_ndevs"
>> +	fi
>> +
>> +	SCRATCH_DEV_POOL_SAVED=${SCRATCH_DEV_POOL}
>> +	export SCRATCH_DEV_POOL_SAVED
>> +	SCRATCH_DEV_POOL=${devs[@]:0:$test_ndevs}
>> +	export SCRATCH_DEV_POOL
>> +}
>> +
>> +_scratch_dev_pool_put()
>> +{
>> +	typeset -p SCRATCH_DEV_POOL_SAVED >/dev/null 2>&1
>> +	if [ $? != 0 ]; then
>> +		_fail "Bug: unset val, must call _scratch_dev_pool_get before _scratch_dev_pool_put"
>> +	fi
>> +
>> +	if [ -z "$SCRATCH_DEV_POOL_SAVED" ]; then
>> +		_fail "Bug: str empty, must call _scratch_dev_pool_get before _scratch_dev_pool_put"
>> +	fi
>> +
>> +	export SCRATCH_DEV_POOL=$SCRATCH_DEV_POOL_SAVED
>> +	export SCRATCH_DEV_POOL_SAVED=""
>> +}
>> +
>>  _scratch_pool_mkfs()
>>  {
>>      case $FSTYP in
>> --
>> 2.7.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe fstests" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>