[PATCH 1/2] xfstests: fix a typo in _require_block_device()

[PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Zhaolei]

From: Zhao Lei <zhaolei@cn.fujitsu.com>

We need to check "$1" instead "$SCRATCH_DEV" in this function.
And make tabs same with other code.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 common/rc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/common/rc b/common/rc
index 7449a1d..76522d4 100644
--- a/common/rc
+++ b/common/rc
@@ -1284,13 +1284,13 @@ _require_command()
 # $1 - device
 _require_block_device()
 {
-	if [ -z "$1" ]; then
-		echo "Usage: _require_block_device <dev>" 1>&2
-		exit 1
-	fi
-	if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
-		_notrun "require $1 to be valid block disk"
-	fi
+    if [ -z "$1" ]; then
+	echo "Usage: _require_block_device <dev>" 1>&2
+	exit 1
+    fi
+    if [ "`_is_block_dev $1`" == "" ]; then
+	_notrun "require $1 to be valid block disk"
+    fi
 }
 
 # this test requires the device mapper flakey target
-- 
1.8.5.1

[PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"

[Zhaolei]

From: Zhao Lei <zhaolei@cn.fujitsu.com>

_is_block_dev() will show above warning when "$dev" is not exist.
It happened when the program check $TEST_DEV with blank $SCRATCH_DEV
which is optional.

Changelog v1->v2:
 Rewrite _is_block_dev() to make caller code simple.
 Suggested by: Dave Chinner <david@fromorbit.com>

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 common/rc | 63 ++++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/common/rc b/common/rc
index 76522d4..c5f6953 100644
--- a/common/rc
+++ b/common/rc
@@ -947,12 +947,11 @@ _fs_options()
 
 # returns device number if a file is a block device
 #
-_is_block_dev()
+_dev_type()
 {
     if [ $# -ne 1 ]
     then
-	echo "Usage: _is_block_dev dev" 1>&2
-	exit 1
+        return
     fi
 
     _dev=$1
@@ -965,6 +964,25 @@ _is_block_dev()
     fi
 }
 
+_is_block_dev()
+{
+    _dev_type=`_dev_type "$1"`
+    if [ -z "$_dev_type" ]; then
+	return 1
+    fi
+
+    _not_same_dev_type=`_dev_type "$2"`
+    if [ -z "$_not_same_dev_type" ]; then
+	return 0
+    fi
+
+    if [ "$_dev_type" = "$_not_same_dev_type" ]; then
+	return 1
+    fi
+
+    return 0
+}
+
 # Do a command, log it to $seqres.full, optionally test return status
 # and die if command fails. If called with one argument _do executes the
 # command, logs it, and returns its exit status. With two arguments _do
@@ -1095,19 +1113,14 @@ _require_scratch_nocheck()
 		fi
 		;;
 	*)
-		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = "" ]
-		 then
-		     _notrun "this test requires a valid \$SCRATCH_DEV"
-		 fi
-		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
-		 then
-		     _notrun "this test requires a valid \$SCRATCH_DEV"
-		 fi
+		if ! _is_block_dev "$SCRATCH_DEV" "$TEST_DEV"; then
+		    _notrun "this test requires a valid \$SCRATCH_DEV"
+		fi
 		if [ ! -d "$SCRATCH_MNT" ]
 		then
-		     _notrun "this test requires a valid \$SCRATCH_MNT"
+		    _notrun "this test requires a valid \$SCRATCH_MNT"
 		fi
-		 ;;
+		;;
     esac
 
     # mounted?
@@ -1167,19 +1180,14 @@ _require_test()
 		fi
 		;;
 	*)
-		 if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ]
-		 then
-		     _notrun "this test requires a valid \$TEST_DEV"
-		 fi
-		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
-		 then
-		     _notrun "this test requires a valid \$TEST_DEV"
-		 fi
+		if ! _is_block_dev "$TEST_DEV" "$SCRATCH_DEV"; then
+		    _notrun "this test requires a valid \$TEST_DEV"
+		fi
 		if [ ! -d "$TEST_DIR" ]
 		then
-		     _notrun "this test requires a valid \$TEST_DIR"
+		    _notrun "this test requires a valid \$TEST_DIR"
 		fi
-		 ;;
+		;;
     esac
 
     # mounted?
@@ -1288,7 +1296,7 @@ _require_block_device()
 	echo "Usage: _require_block_device <dev>" 1>&2
 	exit 1
     fi
-    if [ "`_is_block_dev $1`" == "" ]; then
+    if ! _is_block_dev "$1"; then
 	_notrun "require $1 to be valid block disk"
     fi
 }
@@ -2236,11 +2244,8 @@ _require_scratch_dev_pool()
 	esac
 
 	for i in $SCRATCH_DEV_POOL; do
-		if [ "`_is_block_dev $i`" = "" ]; then
-			_notrun "this test requires valid block disk $i"
-		fi
-		if [ "`_is_block_dev $i`" = "`_is_block_dev $TEST_DEV`" ]; then
-			_notrun "$i is part of TEST_DEV, this test requires unique disks"
+		if ! _is_block_dev "$i" "$TEST_DEV"; then
+			_notrun "$i is not valid valid block disk, or part of TEST_DEV, this test requires unique valid disks"
 		fi
 		if _mount | grep -q $i; then
 			if ! $UMOUNT_PROG $i; then
-- 
1.8.5.1

Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Dave Chinner]

On Mon, Feb 16, 2015 at 02:50:39PM +0800, Zhaolei wrote:
> From: Zhao Lei <zhaolei@cn.fujitsu.com>
> 
> We need to check "$1" instead "$SCRATCH_DEV" in this function.
> And make tabs same with other code.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  common/rc | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7449a1d..76522d4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1284,13 +1284,13 @@ _require_command()
>  # $1 - device
>  _require_block_device()
>  {
> -	if [ -z "$1" ]; then
> -		echo "Usage: _require_block_device <dev>" 1>&2
> -		exit 1
> -	fi
> -	if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
> -		_notrun "require $1 to be valid block disk"
> -	fi
> +    if [ -z "$1" ]; then
> +	echo "Usage: _require_block_device <dev>" 1>&2
> +	exit 1
> +    fi
> +    if [ "`_is_block_dev $1`" == "" ]; then
> +	_notrun "require $1 to be valid block disk"
> +    fi
>  }

The change is fine - the reformating of the code is not. 8 space
tabs for all new code and all new modifications, please.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"

[Dave Chinner]

On Mon, Feb 16, 2015 at 02:50:40PM +0800, Zhaolei wrote:
> From: Zhao Lei <zhaolei@cn.fujitsu.com>
> 
> _is_block_dev() will show above warning when "$dev" is not exist.
> It happened when the program check $TEST_DEV with blank $SCRATCH_DEV
> which is optional.
> 
> Changelog v1->v2:
>  Rewrite _is_block_dev() to make caller code simple.
>  Suggested by: Dave Chinner <david@fromorbit.com>

You haven't implemented what I suggested.

> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  common/rc | 63 ++++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 76522d4..c5f6953 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -947,12 +947,11 @@ _fs_options()
>  
>  # returns device number if a file is a block device
>  #
> -_is_block_dev()
> +_dev_type()
>  {
>      if [ $# -ne 1 ]
>      then
> -	echo "Usage: _is_block_dev dev" 1>&2
> -	exit 1
> +        return
>      fi
>  
>      _dev=$1

This will only output something if the device is a block device.
It's not a generic function. it still only returns a major:minor
device number if the device is a block device. Most callers do not
use this value.

# Returns:
#	-1 if no argument passed
#	0 if filename is not a block device
#	1 if filename is a block device.
_is_block_dev()
{
	if [ $# -ne 1 ]; then
		return -1;
	fi

	if [ -b $1 ]; then
		return 1;
	fi
	return 0;
}


> @@ -965,6 +964,25 @@ _is_block_dev()
>      fi
>  }
>  
> +_is_block_dev()
> +{
> +    _dev_type=`_dev_type "$1"`
> +    if [ -z "$_dev_type" ]; then
> +	return 1
> +    fi
> +
> +    _not_same_dev_type=`_dev_type "$2"`
> +    if [ -z "$_not_same_dev_type" ]; then
> +	return 0
> +    fi
> +
> +    if [ "$_dev_type" = "$_not_same_dev_type" ]; then
> +	return 1
> +    fi
> +
> +    return 0
> +}

This function is testing if two devices are the same device.

# Returns:
# 	0 if the devices are not the same
# 	1 if the devices are the same.
__same_block_dev()
{
	if [ $# -ne 2 ]; then
		return 0;
	fi

	if [ ! -b $1 -o ! -b $2 ]; then
		return 0;
	fi

	if [ `stat -c %t,%T $1` != `stat -c %t,%T $2` ]; then
		return 0;
	fi
	return 1;
}

> +
>  # Do a command, log it to $seqres.full, optionally test return status
>  # and die if command fails. If called with one argument _do executes the
>  # command, logs it, and returns its exit status. With two arguments _do
> @@ -1095,19 +1113,14 @@ _require_scratch_nocheck()
>  		fi
>  		;;
>  	*)
> -		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = "" ]
> -		 then
> -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> -		 fi
> -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
> -		 then
> -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> -		 fi
> +		if ! _is_block_dev "$SCRATCH_DEV" "$TEST_DEV"; then
> +		    _notrun "this test requires a valid \$SCRATCH_DEV"
> +		fi

This doesn't make sense when you read it - the two checks are
logically separate, self documenting checks and should stay that
way. i.e.  the two checks are "if (!_is_block_dev $SCRATCH_DEV)
_notrun..." and "if (_same_block_dev $TEST_DEV $SCRATCH_DEV) _notrun
...."

>  		if [ ! -d "$SCRATCH_MNT" ]
>  		then
> -		     _notrun "this test requires a valid \$SCRATCH_MNT"
> +		    _notrun "this test requires a valid \$SCRATCH_MNT"
>  		fi
> -		 ;;
> +		;;
>      esac
>  
>      # mounted?
> @@ -1167,19 +1180,14 @@ _require_test()
>  		fi
>  		;;
>  	*)
> -		 if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ]
> -		 then
> -		     _notrun "this test requires a valid \$TEST_DEV"
> -		 fi
> -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ]
> -		 then
> -		     _notrun "this test requires a valid \$TEST_DEV"
> -		 fi
> +		if ! _is_block_dev "$TEST_DEV" "$SCRATCH_DEV"; then
> +		    _notrun "this test requires a valid \$TEST_DEV"
> +		fi

Same, but this time for $TEST_DEV

>  		if [ ! -d "$TEST_DIR" ]
>  		then
> -		     _notrun "this test requires a valid \$TEST_DIR"
> +		    _notrun "this test requires a valid \$TEST_DIR"
>  		fi
> -		 ;;
> +		;;
>      esac
>  
>      # mounted?
> @@ -1288,7 +1296,7 @@ _require_block_device()
>  	echo "Usage: _require_block_device <dev>" 1>&2
>  	exit 1
>      fi
> -    if [ "`_is_block_dev $1`" == "" ]; then
> +    if ! _is_block_dev "$1"; then
>  	_notrun "require $1 to be valid block disk"

Please keep the code using the if [ ]; then syntax.
>      fi
>  }
> @@ -2236,11 +2244,8 @@ _require_scratch_dev_pool()
>  	esac
>  
>  	for i in $SCRATCH_DEV_POOL; do
> -		if [ "`_is_block_dev $i`" = "" ]; then
> -			_notrun "this test requires valid block disk $i"
> -		fi
> -		if [ "`_is_block_dev $i`" = "`_is_block_dev $TEST_DEV`" ]; then
> -			_notrun "$i is part of TEST_DEV, this test requires unique disks"
> +		if ! _is_block_dev "$i" "$TEST_DEV"; then
> +			_notrun "$i is not valid valid block disk, or part of TEST_DEV, this test requires unique valid disks"

And that error message is too long. Seperate tests tell us exactly
what the error is, not make us guess which of many errors it could
have been.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Andrew Price]

On 18/02/15 01:21, Dave Chinner wrote:
> On Mon, Feb 16, 2015 at 02:50:39PM +0800, Zhaolei wrote:
>> From: Zhao Lei <zhaolei@cn.fujitsu.com>
>>
>> We need to check "$1" instead "$SCRATCH_DEV" in this function.
>> And make tabs same with other code.
>>
>> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
>> ---
>>   common/rc | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 7449a1d..76522d4 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -1284,13 +1284,13 @@ _require_command()
>>   # $1 - device
>>   _require_block_device()
>>   {
>> -	if [ -z "$1" ]; then
>> -		echo "Usage: _require_block_device <dev>" 1>&2
>> -		exit 1
>> -	fi
>> -	if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
>> -		_notrun "require $1 to be valid block disk"
>> -	fi
>> +    if [ -z "$1" ]; then
>> +	echo "Usage: _require_block_device <dev>" 1>&2
>> +	exit 1
>> +    fi
>> +    if [ "`_is_block_dev $1`" == "" ]; then
>> +	_notrun "require $1 to be valid block disk"
>> +    fi
>>   }
>
> The change is fine - the reformating of the code is not. 8 space
> tabs

To be clear, do you mean 8 spaces or single tabs?

Andy

> for all new code and all new modifications, please.
>
> Cheers,
>
> Dave.
>

Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Dave Chinner]

On Wed, Feb 18, 2015 at 10:21:16AM +0000, Andrew Price wrote:
> On 18/02/15 01:21, Dave Chinner wrote:
> >On Mon, Feb 16, 2015 at 02:50:39PM +0800, Zhaolei wrote:
> >>From: Zhao Lei <zhaolei@cn.fujitsu.com>
> >>
> >>We need to check "$1" instead "$SCRATCH_DEV" in this function.
> >>And make tabs same with other code.
> >>
> >>Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> >>---
> >>  common/rc | 14 +++++++-------
> >>  1 file changed, 7 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/common/rc b/common/rc
> >>index 7449a1d..76522d4 100644
> >>--- a/common/rc
> >>+++ b/common/rc
> >>@@ -1284,13 +1284,13 @@ _require_command()
> >>  # $1 - device
> >>  _require_block_device()
> >>  {
> >>-	if [ -z "$1" ]; then
> >>-		echo "Usage: _require_block_device <dev>" 1>&2
> >>-		exit 1
> >>-	fi
> >>-	if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
> >>-		_notrun "require $1 to be valid block disk"
> >>-	fi
> >>+    if [ -z "$1" ]; then
> >>+	echo "Usage: _require_block_device <dev>" 1>&2
> >>+	exit 1
> >>+    fi
> >>+    if [ "`_is_block_dev $1`" == "" ]; then
> >>+	_notrun "require $1 to be valid block disk"
> >>+    fi
> >>  }
> >
> >The change is fine - the reformating of the code is not. 8 space
> >tabs
> 
> To be clear, do you mean 8 spaces or single tabs?

Single tab, but tabs are the width of 8 spaces. Same as you'd use
for writing new kernel code.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

RE: [PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"

[Zhao Lei]

Hi, Dave Chinner

Thanks for suggest.

* From: Dave Chinner [mailto:david@fromorbit.com]
> Subject: Re: [PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"
> 
> On Mon, Feb 16, 2015 at 02:50:40PM +0800, Zhaolei wrote:
> > From: Zhao Lei <zhaolei@cn.fujitsu.com>
> >
> > _is_block_dev() will show above warning when "$dev" is not exist.
> > It happened when the program check $TEST_DEV with blank $SCRATCH_DEV
> > which is optional.
> >
> > Changelog v1->v2:
> >  Rewrite _is_block_dev() to make caller code simple.
> >  Suggested by: Dave Chinner <david@fromorbit.com>
> 
> You haven't implemented what I suggested.
> 
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  common/rc | 63
> > ++++++++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 34 insertions(+), 29 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 76522d4..c5f6953 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -947,12 +947,11 @@ _fs_options()
> >
> >  # returns device number if a file is a block device  #
> > -_is_block_dev()
> > +_dev_type()
> >  {
> >      if [ $# -ne 1 ]
> >      then
> > -	echo "Usage: _is_block_dev dev" 1>&2
> > -	exit 1
> > +        return
> >      fi
> >
> >      _dev=$1
> 
> This will only output something if the device is a block device.
> It's not a generic function. it still only returns a major:minor device number if
> the device is a block device. Most callers do not use this value.
> 
> # Returns:
> #	-1 if no argument passed
> #	0 if filename is not a block device
> #	1 if filename is a block device.
> _is_block_dev()
> {
> 	if [ $# -ne 1 ]; then
> 		return -1;
> 	fi
> 
> 	if [ -b $1 ]; then
> 		return 1;
> 	fi
> 	return 0;
> }
> 
Maybe better to return 0 if it is block dev,
to avoid following code in caller:
_is_block_dev "$dev"
if [ "$?" != 1 ] ...

Will send v3.

Thanks
Zhaolei

> 
> > @@ -965,6 +964,25 @@ _is_block_dev()
> >      fi
> >  }
> >
> > +_is_block_dev()
> > +{
> > +    _dev_type=`_dev_type "$1"`
> > +    if [ -z "$_dev_type" ]; then
> > +	return 1
> > +    fi
> > +
> > +    _not_same_dev_type=`_dev_type "$2"`
> > +    if [ -z "$_not_same_dev_type" ]; then
> > +	return 0
> > +    fi
> > +
> > +    if [ "$_dev_type" = "$_not_same_dev_type" ]; then
> > +	return 1
> > +    fi
> > +
> > +    return 0
> > +}
> 
> This function is testing if two devices are the same device.
> 
> # Returns:
> # 	0 if the devices are not the same
> # 	1 if the devices are the same.
> __same_block_dev()
> {
> 	if [ $# -ne 2 ]; then
> 		return 0;
> 	fi
> 
> 	if [ ! -b $1 -o ! -b $2 ]; then
> 		return 0;
> 	fi
> 
> 	if [ `stat -c %t,%T $1` != `stat -c %t,%T $2` ]; then
> 		return 0;
> 	fi
> 	return 1;
> }
> 
> > +
> >  # Do a command, log it to $seqres.full, optionally test return status
> > # and die if command fails. If called with one argument _do executes
> > the  # command, logs it, and returns its exit status. With two
> > arguments _do @@ -1095,19 +1113,14 @@ _require_scratch_nocheck()
> >  		fi
> >  		;;
> >  	*)
> > -		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = "" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> > -		 fi
> > -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev
> $TEST_DEV`" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> > -		 fi
> > +		if ! _is_block_dev "$SCRATCH_DEV" "$TEST_DEV"; then
> > +		    _notrun "this test requires a valid \$SCRATCH_DEV"
> > +		fi
> 
> This doesn't make sense when you read it - the two checks are logically
> separate, self documenting checks and should stay that way. i.e.  the two
> checks are "if (!_is_block_dev $SCRATCH_DEV) _notrun..." and "if
> (_same_block_dev $TEST_DEV $SCRATCH_DEV) _notrun ...."
> 
> >  		if [ ! -d "$SCRATCH_MNT" ]
> >  		then
> > -		     _notrun "this test requires a valid \$SCRATCH_MNT"
> > +		    _notrun "this test requires a valid \$SCRATCH_MNT"
> >  		fi
> > -		 ;;
> > +		;;
> >      esac
> >
> >      # mounted?
> > @@ -1167,19 +1180,14 @@ _require_test()
> >  		fi
> >  		;;
> >  	*)
> > -		 if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$TEST_DEV"
> > -		 fi
> > -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev
> $TEST_DEV`" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$TEST_DEV"
> > -		 fi
> > +		if ! _is_block_dev "$TEST_DEV" "$SCRATCH_DEV"; then
> > +		    _notrun "this test requires a valid \$TEST_DEV"
> > +		fi
> 
> Same, but this time for $TEST_DEV
> 
> >  		if [ ! -d "$TEST_DIR" ]
> >  		then
> > -		     _notrun "this test requires a valid \$TEST_DIR"
> > +		    _notrun "this test requires a valid \$TEST_DIR"
> >  		fi
> > -		 ;;
> > +		;;
> >      esac
> >
> >      # mounted?
> > @@ -1288,7 +1296,7 @@ _require_block_device()
> >  	echo "Usage: _require_block_device <dev>" 1>&2
> >  	exit 1
> >      fi
> > -    if [ "`_is_block_dev $1`" == "" ]; then
> > +    if ! _is_block_dev "$1"; then
> >  	_notrun "require $1 to be valid block disk"
> 
> Please keep the code using the if [ ]; then syntax.
> >      fi
> >  }
> > @@ -2236,11 +2244,8 @@ _require_scratch_dev_pool()
> >  	esac
> >
> >  	for i in $SCRATCH_DEV_POOL; do
> > -		if [ "`_is_block_dev $i`" = "" ]; then
> > -			_notrun "this test requires valid block disk $i"
> > -		fi
> > -		if [ "`_is_block_dev $i`" = "`_is_block_dev $TEST_DEV`" ]; then
> > -			_notrun "$i is part of TEST_DEV, this test requires unique disks"
> > +		if ! _is_block_dev "$i" "$TEST_DEV"; then
> > +			_notrun "$i is not valid valid block disk, or part of TEST_DEV, this
> test requires unique valid disks"
> 
> And that error message is too long. Seperate tests tell us exactly what the
> error is, not make us guess which of many errors it could have been.
> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

RE: [PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"

[Zhao Lei]

Hi, Dave Chinner

> From: Dave Chinner [mailto:david@fromorbit.com]
> Subject: Re: [PATCH 2/2] Fix warning of "Usage: _is_block_dev dev"
> 
> On Mon, Feb 16, 2015 at 02:50:40PM +0800, Zhaolei wrote:
> > From: Zhao Lei <zhaolei@cn.fujitsu.com>
> >
> > _is_block_dev() will show above warning when "$dev" is not exist.
> > It happened when the program check $TEST_DEV with blank $SCRATCH_DEV
> > which is optional.
> >
> > Changelog v1->v2:
> >  Rewrite _is_block_dev() to make caller code simple.
> >  Suggested by: Dave Chinner <david@fromorbit.com>
> 
> You haven't implemented what I suggested.
> 
> > Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > ---
> >  common/rc | 63
> > ++++++++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 34 insertions(+), 29 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 76522d4..c5f6953 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -947,12 +947,11 @@ _fs_options()
> >
> >  # returns device number if a file is a block device  #
> > -_is_block_dev()
> > +_dev_type()
> >  {
> >      if [ $# -ne 1 ]
> >      then
> > -	echo "Usage: _is_block_dev dev" 1>&2
> > -	exit 1
> > +        return
> >      fi
> >
> >      _dev=$1
> 
> This will only output something if the device is a block device.
> It's not a generic function. it still only returns a major:minor device number if
> the device is a block device. Most callers do not use this value.
> 
> # Returns:
> #	-1 if no argument passed
> #	0 if filename is not a block device
> #	1 if filename is a block device.
> _is_block_dev()
> {
> 	if [ $# -ne 1 ]; then
> 		return -1;
> 	fi
> 
> 	if [ -b $1 ]; then
> 		return 1;
> 	fi
> 	return 0;
> }
> 
> 
> > @@ -965,6 +964,25 @@ _is_block_dev()
> >      fi
> >  }
> >
> > +_is_block_dev()
> > +{
> > +    _dev_type=`_dev_type "$1"`
> > +    if [ -z "$_dev_type" ]; then
> > +	return 1
> > +    fi
> > +
> > +    _not_same_dev_type=`_dev_type "$2"`
> > +    if [ -z "$_not_same_dev_type" ]; then
> > +	return 0
> > +    fi
> > +
> > +    if [ "$_dev_type" = "$_not_same_dev_type" ]; then
> > +	return 1
> > +    fi
> > +
> > +    return 0
> > +}
> 
> This function is testing if two devices are the same device.
> 
> # Returns:
> # 	0 if the devices are not the same
> # 	1 if the devices are the same.
> __same_block_dev()
> {
> 	if [ $# -ne 2 ]; then
> 		return 0;
> 	fi
> 
> 	if [ ! -b $1 -o ! -b $2 ]; then
> 		return 0;
> 	fi
> 
> 	if [ `stat -c %t,%T $1` != `stat -c %t,%T $2` ]; then
> 		return 0;
> 	fi
> 	return 1;
> }
> 
> > +
> >  # Do a command, log it to $seqres.full, optionally test return status
> > # and die if command fails. If called with one argument _do executes
> > the  # command, logs it, and returns its exit status. With two
> > arguments _do @@ -1095,19 +1113,14 @@ _require_scratch_nocheck()
> >  		fi
> >  		;;
> >  	*)
> > -		 if [ -z "$SCRATCH_DEV" -o "`_is_block_dev $SCRATCH_DEV`" = "" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> > -		 fi
> > -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev
> $TEST_DEV`" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$SCRATCH_DEV"
> > -		 fi
> > +		if ! _is_block_dev "$SCRATCH_DEV" "$TEST_DEV"; then
> > +		    _notrun "this test requires a valid \$SCRATCH_DEV"
> > +		fi
> 
> This doesn't make sense when you read it - the two checks are logically
> separate, self documenting checks and should stay that way. i.e.  the two
> checks are "if (!_is_block_dev $SCRATCH_DEV) _notrun..." and "if
> (_same_block_dev $TEST_DEV $SCRATCH_DEV) _notrun ...."

ok.

> 
> >  		if [ ! -d "$SCRATCH_MNT" ]
> >  		then
> > -		     _notrun "this test requires a valid \$SCRATCH_MNT"
> > +		    _notrun "this test requires a valid \$SCRATCH_MNT"
> >  		fi
> > -		 ;;
> > +		;;
> >      esac
> >
> >      # mounted?
> > @@ -1167,19 +1180,14 @@ _require_test()
> >  		fi
> >  		;;
> >  	*)
> > -		 if [ -z "$TEST_DEV" -o "`_is_block_dev $TEST_DEV`" = "" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$TEST_DEV"
> > -		 fi
> > -		 if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev
> $TEST_DEV`" ]
> > -		 then
> > -		     _notrun "this test requires a valid \$TEST_DEV"
> > -		 fi
> > +		if ! _is_block_dev "$TEST_DEV" "$SCRATCH_DEV"; then
> > +		    _notrun "this test requires a valid \$TEST_DEV"
> > +		fi
> 
> Same, but this time for $TEST_DEV
> 
> >  		if [ ! -d "$TEST_DIR" ]
> >  		then
> > -		     _notrun "this test requires a valid \$TEST_DIR"
> > +		    _notrun "this test requires a valid \$TEST_DIR"
> >  		fi
> > -		 ;;
> > +		;;
> >      esac
> >
> >      # mounted?
> > @@ -1288,7 +1296,7 @@ _require_block_device()
> >  	echo "Usage: _require_block_device <dev>" 1>&2
> >  	exit 1
> >      fi
> > -    if [ "`_is_block_dev $1`" == "" ]; then
> > +    if ! _is_block_dev "$1"; then
> >  	_notrun "require $1 to be valid block disk"
> 
> Please keep the code using the if [ ]; then syntax.

It is hard to check return value in if [] format, unless we change function to
echo result into stdout and run function in a subshell.
We had discussed it in last mail, using return value is simple and stable than
function stdout, for example, a function will output unwanted content when
it call a failed command or changed-version command.


> >      fi
> >  }
> > @@ -2236,11 +2244,8 @@ _require_scratch_dev_pool()
> >  	esac
> >
> >  	for i in $SCRATCH_DEV_POOL; do
> > -		if [ "`_is_block_dev $i`" = "" ]; then
> > -			_notrun "this test requires valid block disk $i"
> > -		fi
> > -		if [ "`_is_block_dev $i`" = "`_is_block_dev $TEST_DEV`" ]; then
> > -			_notrun "$i is part of TEST_DEV, this test requires unique disks"
> > +		if ! _is_block_dev "$i" "$TEST_DEV"; then
> > +			_notrun "$i is not valid valid block disk, or part of TEST_DEV, this
> test requires unique valid disks"
> 
> And that error message is too long. Seperate tests tell us exactly what the
> error is, not make us guess which of many errors it could have been.

Agree.

Thanks
Zhaolei

> 
> Cheers,
> 
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Lukáš Czerner]

On Thu, 19 Feb 2015, Dave Chinner wrote:

> Date: Thu, 19 Feb 2015 09:02:04 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Andrew Price <andyp@fedoraproject.org>
> Cc: fstests@vger.kernel.org
> Subject: Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()
> 
> On Wed, Feb 18, 2015 at 10:21:16AM +0000, Andrew Price wrote:
> > On 18/02/15 01:21, Dave Chinner wrote:
> > >On Mon, Feb 16, 2015 at 02:50:39PM +0800, Zhaolei wrote:
> > >>From: Zhao Lei <zhaolei@cn.fujitsu.com>
> > >>
> > >>We need to check "$1" instead "$SCRATCH_DEV" in this function.
> > >>And make tabs same with other code.
> > >>
> > >>Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> > >>---
> > >>  common/rc | 14 +++++++-------
> > >>  1 file changed, 7 insertions(+), 7 deletions(-)
> > >>
> > >>diff --git a/common/rc b/common/rc
> > >>index 7449a1d..76522d4 100644
> > >>--- a/common/rc
> > >>+++ b/common/rc
> > >>@@ -1284,13 +1284,13 @@ _require_command()
> > >>  # $1 - device
> > >>  _require_block_device()
> > >>  {
> > >>-	if [ -z "$1" ]; then
> > >>-		echo "Usage: _require_block_device <dev>" 1>&2
> > >>-		exit 1
> > >>-	fi
> > >>-	if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then
> > >>-		_notrun "require $1 to be valid block disk"
> > >>-	fi
> > >>+    if [ -z "$1" ]; then
> > >>+	echo "Usage: _require_block_device <dev>" 1>&2
> > >>+	exit 1
> > >>+    fi
> > >>+    if [ "`_is_block_dev $1`" == "" ]; then
> > >>+	_notrun "require $1 to be valid block disk"
> > >>+    fi
> > >>  }
> > >
> > >The change is fine - the reformating of the code is not. 8 space
> > >tabs
> > 
> > To be clear, do you mean 8 spaces or single tabs?
> 
> Single tab, but tabs are the width of 8 spaces. Same as you'd use
> for writing new kernel code.

Even I was confused by that :) Just use tabs not spaces. Width of the
tab is whatever your editor decides it is so it's irrelevant :)

-Lukas

> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 1/2] xfstests: fix a typo in _require_block_device()

[Andrew Price]

>>>> The change is fine - the reformating of the code is not. 8 space
>>>> tabs
>>>
>>> To be clear, do you mean 8 spaces or single tabs?
>>
>> Single tab, but tabs are the width of 8 spaces. Same as you'd use
>> for writing new kernel code.
>
> Even I was confused by that :) Just use tabs not spaces. Width of the
> tab is whatever your editor decides it is

Exactly :)

> so it's irrelevant :)

Well, unfortunately many use tabs for alignment as well as indentation 
so changing the width of a tab can make code look ugly where there are 
line continuations, aligned comments, etc. Using tabs for indentation 
and spaces for alignment is ideal, but tabs-for-alignment is so common 
that assuming tabs are always displayed 8 characters wide is often the 
pragmatic option.

Andy