diff mbox

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

Message ID 1465980363-18906-1-git-send-email-anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Anand Jain June 15, 2016, 8:46 a.m. UTC 
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>
---
 v2: Error message and comment section updates.

 common/rc | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Eryu Guan June 21, 2016, 1:20 p.m. UTC | #1 
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
--
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
Anand Jain June 22, 2016, 11:01 a.m. UTC | #2 
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
>
--
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
diff mbox

Patch

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
+		_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