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

[Eryu Guan <eguan@redhat.com>]

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?

> ---
>  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,
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