FS/XFS testing framework
 help / color / mirror / Atom feed
* [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand
@ 2016-12-08  2:04 Qu Wenruo
  2016-12-08  2:04 ` [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code Qu Wenruo
  2016-12-08  4:00 ` [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Eryu Guan
  0 siblings, 2 replies; 6+ messages in thread
From: Qu Wenruo @ 2016-12-08  2:04 UTC (permalink / raw)
  To: linux-btrfs, fstests

Rename _require_btrfs() to _require_btrfs_subcommand() to avoid
confusion, as all other _require_btrfs_* has a quite clear suffix, like
_require_btrfs_mkfs_feature() or _require_btrfs_fs_feature().

Also enhance _require_btrfs_subcommand() to accept 2nd level commands or
options.
Options will be determined by the first "-" char.
This is quite useful for case like "btrfs inspect-internal dump-tree"
and "btrfs check --qgroup-report".

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 common/rc       | 29 ++++++++++++++++++++++++-----
 tests/btrfs/004 |  3 ++-
 tests/btrfs/048 |  2 +-
 tests/btrfs/059 |  2 +-
 tests/btrfs/131 |  2 +-
 5 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/common/rc b/common/rc
index 8c99306..1703232 100644
--- a/common/rc
+++ b/common/rc
@@ -3019,15 +3019,34 @@ _require_deletable_scratch_dev_pool()
 }
 
 # We check for btrfs and (optionally) features of the btrfs command
-_require_btrfs()
+# _require_btrfs_subcommand <subcommand> [<subfunction>|<option>]
+# It can handle both subfunction like "inspect-internal dump-tree"
+# and options like "check --qgroup-report"
+_require_btrfs_subcommand()
 {
-	cmd=$1
-	_require_command "$BTRFS_UTIL_PROG" btrfs
 	if [ -z "$1" ]; then
-		return 1;
+		echo "Usage: _require_btrfs_subcommand command [subcommand]" 1>&2
+		exit 1
 	fi
-	$BTRFS_UTIL_PROG $cmd --help >/dev/null 2>&1
+	cmd=$1
+	param=$2
+
+	_require_command "$BTRFS_UTIL_PROG" btrfs
+	$BTRFS_UTIL_PROG $cmd --help &>/dev/null
 	[ $? -eq 0 ] || _notrun "$BTRFS_UTIL_PROG too old (must support $cmd)"
+
+	test -z "$param" && return
+
+	# if $param is an option, replace leading "-"s for grep
+	if [ ${param:0:1} == "-" ]; then
+		param=$(echo $param | sed 's/^-*//')
+		$BTRFS_UTIL_PROG $cmd --help | grep $param > /dev/null || \
+			_not_run "$BTRFS_UTIL_PROG too old (must support $cmd $param)"
+		return
+	fi
+
+	$BTRFS_UTIL_PROG $cmd $param --help &>/dev/null
+	[ $? -eq 0 ] || _notrun "$BTRFS_UTIL_PROG too old (must support $cmd $param)"
 }
 
 # Check that fio is present, and it is able to execute given jobfile
diff --git a/tests/btrfs/004 b/tests/btrfs/004
index 905770a..e60a034 100755
--- a/tests/btrfs/004
+++ b/tests/btrfs/004
@@ -51,7 +51,8 @@ _supported_fs btrfs
 _supported_os Linux
 _require_scratch
 _require_no_large_scratch_dev
-_require_btrfs inspect-internal
+_require_btrfs_subcommand inspect-internal logical-resolve
+_require_btrfs_subcommand inspect-internal inode-resolve
 _require_command "/usr/sbin/filefrag" filefrag
 
 rm -f $seqres.full
diff --git a/tests/btrfs/048 b/tests/btrfs/048
index 0b907b0..ac731d1 100755
--- a/tests/btrfs/048
+++ b/tests/btrfs/048
@@ -48,7 +48,7 @@ _supported_fs btrfs
 _supported_os Linux
 _require_test
 _require_scratch
-_require_btrfs "property"
+_require_btrfs_subcommand "property"
 
 send_files_dir=$TEST_DIR/btrfs-test-$seq
 
diff --git a/tests/btrfs/059 b/tests/btrfs/059
index 8f106d2..fd67ebb 100755
--- a/tests/btrfs/059
+++ b/tests/btrfs/059
@@ -51,7 +51,7 @@ _supported_fs btrfs
 _supported_os Linux
 _require_test
 _require_scratch
-_require_btrfs "property"
+_require_btrfs_subcommand "property"
 
 rm -f $seqres.full
 
diff --git a/tests/btrfs/131 b/tests/btrfs/131
index d1a11d2..d7c7f12 100755
--- a/tests/btrfs/131
+++ b/tests/btrfs/131
@@ -48,7 +48,7 @@ rm -f $seqres.full
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
-_require_btrfs inspect-internal
+_require_btrfs_subcommand inspect-internal dump-super
 
 mkfs_v1()
 {
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code
  2016-12-08  2:04 [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Qu Wenruo
@ 2016-12-08  2:04 ` Qu Wenruo
  2016-12-08  5:27   ` Eryu Guan
  2016-12-08  4:00 ` [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Eryu Guan
  1 sibling, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2016-12-08  2:04 UTC (permalink / raw)
  To: linux-btrfs, fstests

Introduce new _require_btrfs_qgroup_report function, which will check
the accessibility to "btrfs check --qgroup-report", then set a global
flag to info _check_scratch_fs() to do extra qgroup check.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 common/rc       | 22 ++++++++++++++++++++++
 tests/btrfs/022 |  5 +++++
 tests/btrfs/028 |  5 ++---
 tests/btrfs/042 |  6 ++----
 tests/btrfs/099 |  1 +
 tests/btrfs/104 | 20 +++++---------------
 tests/btrfs/122 | 10 +++-------
 tests/btrfs/123 |  5 ++---
 8 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/common/rc b/common/rc
index 1703232..bce3a09 100644
--- a/common/rc
+++ b/common/rc
@@ -2624,6 +2624,20 @@ _check_btrfs_filesystem()
         mountpoint=`_umount_or_remount_ro $device`
     fi
 
+    # Check qgroup numbers
+    if [ "$BTRFS_NEED_QGROUP_REPORT" == "yes" ];then
+	    btrfsck $device --qgroup-report > $tmp.qgroup_report 2>&1
+	    if grep -qE "Counts for qgroup.*are different" $tmp.qgroup_report ; then
+	        echo "_check_btrfs_filesystem: filesystem on $device has wrong qgroup numbers (see $seqres.full)"
+	        echo "_check_btrfs_filesystem: filesystem on $device has wrong qgroup numbers" \
+			>> $seqres.full
+		echo "*** qgroup_report.$FSTYP output ***"	>>$seqres.full
+		cat $tmp.qgroup_report				>>$seqres.full
+		echo "*** qgroup_report.$FSTYP output ***"	>>$seqres.full
+            fi
+    fi
+    rm -f $tmp.qgroup_report
+
     btrfsck $device >$tmp.fsck 2>&1
     if [ $? -ne 0 ]
     then
@@ -3049,6 +3063,14 @@ _require_btrfs_subcommand()
 	[ $? -eq 0 ] || _notrun "$BTRFS_UTIL_PROG too old (must support $cmd $param)"
 }
 
+# Require "btrfs check --qgroup-report" fucntion and will check the qgroup
+# numbers at _check_scratch_fs()
+_require_btrfs_qgroup_report()
+{
+	_require_btrfs_subcommand check --qgroup-report
+	export BTRFS_NEED_QGROUP_REPORT="yes"
+}
+
 # Check that fio is present, and it is able to execute given jobfile
 _require_fio()
 {
diff --git a/tests/btrfs/022 b/tests/btrfs/022
index 56d4f3d..2f21a78 100755
--- a/tests/btrfs/022
+++ b/tests/btrfs/022
@@ -43,6 +43,7 @@ _cleanup()
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 rm -f $seqres.full
 
@@ -125,20 +126,24 @@ _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _basic_test
 _scratch_unmount
+_check_scratch_fs
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _rescan_test
 _scratch_unmount
+_check_scratch_fs
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _limit_test_exceed
 _scratch_unmount
+_check_scratch_fs
 
 _scratch_mkfs > /dev/null 2>&1
 _scratch_mount
 _limit_test_noexceed
+_check_scratch_fs
 
 # success, all done
 echo "Silence is golden"
diff --git a/tests/btrfs/028 b/tests/btrfs/028
index 1425609..a3d9a27 100755
--- a/tests/btrfs/028
+++ b/tests/btrfs/028
@@ -51,6 +51,7 @@ rm -f $seqres.full
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 _scratch_mkfs
 _scratch_mount
@@ -86,9 +87,7 @@ _run_btrfs_util_prog filesystem sync $SCRATCH_MNT
 
 _scratch_unmount
 
-# generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-	grep -E "Counts for qgroup.*are different"
+# qgroup will be checked at _check_scratch_fs() by fstest.
 echo "Silence is golden"
 status=0
 
diff --git a/tests/btrfs/042 b/tests/btrfs/042
index 498ccc9..dc9b762 100755
--- a/tests/btrfs/042
+++ b/tests/btrfs/042
@@ -43,6 +43,7 @@ _cleanup()
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 rm -f $seqres.full
 
@@ -84,10 +85,7 @@ for i in `seq 10 -1 1`; do
 	total_written=$(($total_written+$filesize))
 done
 
-#check if total written exceeds limit
-if [ $total_written -gt $LIMIT_SIZE ];then
-	_fail "total written should be less than $LIMIT_SIZE"
-fi
+# qgroup will be checked automatically at _check_scratch_fs() by fstest
 
 # success, all done
 echo "Silence is golden"
diff --git a/tests/btrfs/099 b/tests/btrfs/099
index 70f07b5..65ea79b 100755
--- a/tests/btrfs/099
+++ b/tests/btrfs/099
@@ -46,6 +46,7 @@ _cleanup()
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 # Use big blocksize to ensure there is still enough space left for metadata
 # space reserve.
diff --git a/tests/btrfs/104 b/tests/btrfs/104
index 6afaa02..e6a6d3b 100755
--- a/tests/btrfs/104
+++ b/tests/btrfs/104
@@ -58,6 +58,7 @@ rm -f $seqres.full
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 rm -f $seqres.full
 
@@ -145,21 +146,10 @@ _scratch_cycle_mount
 # referenced above.
 _run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap1
 
-# There is no way from userspace to force btrfs_drop_snapshot to run
-# at a given time (even via mount/unmount). We must wait for it to
-# start and complete. This is the shortest time on my tests systems I
-# have found which always allows drop_snapshot to run to completion.
-sleep 45
+# "btrfs filesystem sync" will trigger subvolume deletion
+_run_btrfs_util_prog filesystem sync $SCRATCH_MNT
 
-_scratch_unmount
-
-# generate a qgroup report and look for inconsistent groups
-#  - don't use _run_btrfs_util_prog here as it captures the output and
-#    we need to grep it.
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-			grep -E -q "Counts for qgroup.*are different"
-if [ $? -ne 0 ]; then
-	status=0
-fi
+# Qgroup will be checked by fstest at _check_scratch_fs()
 
+status=0
 exit
diff --git a/tests/btrfs/122 b/tests/btrfs/122
index 82252ab..212571f 100755
--- a/tests/btrfs/122
+++ b/tests/btrfs/122
@@ -49,6 +49,7 @@ rm -f $seqres.full
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 rm -f $seqres.full
 
@@ -78,11 +79,6 @@ _run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT "$SCRATCH_MNT/snaps/snap2"
 
 _scratch_unmount
 
-# generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-			grep -q -E "Counts for qgroup.*are different"
-if [ $? -ne 0 ]; then
-	status=0
-fi
-
+# qgroup will be checked by fstest at _check_scratch_fs()
+status=0
 exit
diff --git a/tests/btrfs/123 b/tests/btrfs/123
index e89d541..52d87fb 100755
--- a/tests/btrfs/123
+++ b/tests/btrfs/123
@@ -53,6 +53,7 @@ rm -f $seqres.full
 _supported_fs btrfs
 _supported_os Linux
 _require_scratch
+_require_btrfs_qgroup_report
 
 _scratch_mkfs
 # Need to use inline extents to fill metadata rapidly
@@ -76,9 +77,7 @@ _run_btrfs_util_prog quota rescan -w $SCRATCH_MNT
 _run_btrfs_util_prog balance start -d $SCRATCH_MNT
 
 _scratch_unmount
-# generate a qgroup report and look for inconsistent groups
-$BTRFS_UTIL_PROG check --qgroup-report $SCRATCH_DEV 2>&1 | \
-		grep -E "Counts for qgroup.*are different"
+# qgroup will be check at _check_scratch_fs() by fstest
 
 # success, all done
 status=0
-- 
2.7.4




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand
  2016-12-08  2:04 [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Qu Wenruo
  2016-12-08  2:04 ` [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code Qu Wenruo
@ 2016-12-08  4:00 ` Eryu Guan
  2016-12-08  5:44   ` Qu Wenruo
  1 sibling, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2016-12-08  4:00 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs, fstests

On Thu, Dec 08, 2016 at 10:04:55AM +0800, Qu Wenruo wrote:
> Rename _require_btrfs() to _require_btrfs_subcommand() to avoid
> confusion, as all other _require_btrfs_* has a quite clear suffix, like
> _require_btrfs_mkfs_feature() or _require_btrfs_fs_feature().
> 
> Also enhance _require_btrfs_subcommand() to accept 2nd level commands or
> options.
> Options will be determined by the first "-" char.
> This is quite useful for case like "btrfs inspect-internal dump-tree"
> and "btrfs check --qgroup-report".
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  common/rc       | 29 ++++++++++++++++++++++++-----

Can you rebase on top of current master please? We've moved
btrfs-specific functions to common/btrfs

>  tests/btrfs/004 |  3 ++-
>  tests/btrfs/048 |  2 +-
>  tests/btrfs/059 |  2 +-
>  tests/btrfs/131 |  2 +-
>  5 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 8c99306..1703232 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3019,15 +3019,34 @@ _require_deletable_scratch_dev_pool()
>  }
>  
>  # We check for btrfs and (optionally) features of the btrfs command
> -_require_btrfs()
> +# _require_btrfs_subcommand <subcommand> [<subfunction>|<option>]
> +# It can handle both subfunction like "inspect-internal dump-tree"
> +# and options like "check --qgroup-report"
> +_require_btrfs_subcommand()

I'd prefer a name similar to _require_xfs_io_command, e.g.
_require_btrfs_command, "subcommand" seems not necessary to me.

>  {
> -	cmd=$1
> -	_require_command "$BTRFS_UTIL_PROG" btrfs
>  	if [ -z "$1" ]; then
> -		return 1;
> +		echo "Usage: _require_btrfs_subcommand command [subcommand]" 1>&2
> +		exit 1
>  	fi
> -	$BTRFS_UTIL_PROG $cmd --help >/dev/null 2>&1
> +	cmd=$1
> +	param=$2
> +
> +	_require_command "$BTRFS_UTIL_PROG" btrfs
> +	$BTRFS_UTIL_PROG $cmd --help &>/dev/null
>  	[ $? -eq 0 ] || _notrun "$BTRFS_UTIL_PROG too old (must support $cmd)"
> +
> +	test -z "$param" && return
> +
> +	# if $param is an option, replace leading "-"s for grep
> +	if [ ${param:0:1} == "-" ]; then
> +		param=$(echo $param | sed 's/^-*//')
> +		$BTRFS_UTIL_PROG $cmd --help | grep $param > /dev/null || \

Use "grep -w" to be safer? And "-q" instead of "> /dev/null"

> +			_not_run "$BTRFS_UTIL_PROG too old (must support $cmd $param)"

$param here is without leading "-", so the _notrun message is kind of
misleading. And _notrun not _not_run :)

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code
  2016-12-08  2:04 ` [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code Qu Wenruo
@ 2016-12-08  5:27   ` Eryu Guan
  2016-12-08  5:49     ` Qu Wenruo
  0 siblings, 1 reply; 6+ messages in thread
From: Eryu Guan @ 2016-12-08  5:27 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs, fstests

On Thu, Dec 08, 2016 at 10:04:56AM +0800, Qu Wenruo wrote:
> Introduce new _require_btrfs_qgroup_report function, which will check
> the accessibility to "btrfs check --qgroup-report", then set a global
> flag to info _check_scratch_fs() to do extra qgroup check.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  common/rc       | 22 ++++++++++++++++++++++

This needs rebase too.

>  tests/btrfs/022 |  5 +++++
>  tests/btrfs/028 |  5 ++---
>  tests/btrfs/042 |  6 ++----
>  tests/btrfs/099 |  1 +
>  tests/btrfs/104 | 20 +++++---------------
>  tests/btrfs/122 | 10 +++-------
>  tests/btrfs/123 |  5 ++---
>  8 files changed, 42 insertions(+), 32 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 1703232..bce3a09 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2624,6 +2624,20 @@ _check_btrfs_filesystem()
>          mountpoint=`_umount_or_remount_ro $device`
>      fi
>  
> +    # Check qgroup numbers
> +    if [ "$BTRFS_NEED_QGROUP_REPORT" == "yes" ];then

So we can bypass the _require_btrfs_qgroup_report check if we set
BTRFS_NEED_QGROUP_REPORT to "yes" directly, right? How about doing
something like _require_scratch do, e.g. touching some signal file in
$RESULT_DIR and only do qgroup check if that file exists?

> +	    btrfsck $device --qgroup-report > $tmp.qgroup_report 2>&1

Shouldn't "$BTRFS_UTIL_PROG check $device ..." be used for new code? I
might be wrong on this, I think btrfsck is deprecated.

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand
  2016-12-08  4:00 ` [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Eryu Guan
@ 2016-12-08  5:44   ` Qu Wenruo
  0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2016-12-08  5:44 UTC (permalink / raw)
  To: Eryu Guan; +Cc: linux-btrfs, fstests



At 12/08/2016 12:00 PM, Eryu Guan wrote:
> On Thu, Dec 08, 2016 at 10:04:55AM +0800, Qu Wenruo wrote:
>> Rename _require_btrfs() to _require_btrfs_subcommand() to avoid
>> confusion, as all other _require_btrfs_* has a quite clear suffix, like
>> _require_btrfs_mkfs_feature() or _require_btrfs_fs_feature().
>>
>> Also enhance _require_btrfs_subcommand() to accept 2nd level commands or
>> options.
>> Options will be determined by the first "-" char.
>> This is quite useful for case like "btrfs inspect-internal dump-tree"
>> and "btrfs check --qgroup-report".
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  common/rc       | 29 ++++++++++++++++++++++++-----
>
> Can you rebase on top of current master please? We've moved
> btrfs-specific functions to common/btrfs

Finally!
Good news.

>
>>  tests/btrfs/004 |  3 ++-
>>  tests/btrfs/048 |  2 +-
>>  tests/btrfs/059 |  2 +-
>>  tests/btrfs/131 |  2 +-
>>  5 files changed, 29 insertions(+), 9 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 8c99306..1703232 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -3019,15 +3019,34 @@ _require_deletable_scratch_dev_pool()
>>  }
>>
>>  # We check for btrfs and (optionally) features of the btrfs command
>> -_require_btrfs()
>> +# _require_btrfs_subcommand <subcommand> [<subfunction>|<option>]
>> +# It can handle both subfunction like "inspect-internal dump-tree"
>> +# and options like "check --qgroup-report"
>> +_require_btrfs_subcommand()
>
> I'd prefer a name similar to _require_xfs_io_command, e.g.
> _require_btrfs_command, "subcommand" seems not necessary to me.
>

Right, the subcommand seems not that handy compared to other _require_*.


>>  {
>> -	cmd=$1
>> -	_require_command "$BTRFS_UTIL_PROG" btrfs
>>  	if [ -z "$1" ]; then
>> -		return 1;
>> +		echo "Usage: _require_btrfs_subcommand command [subcommand]" 1>&2
>> +		exit 1
>>  	fi
>> -	$BTRFS_UTIL_PROG $cmd --help >/dev/null 2>&1
>> +	cmd=$1
>> +	param=$2
>> +
>> +	_require_command "$BTRFS_UTIL_PROG" btrfs
>> +	$BTRFS_UTIL_PROG $cmd --help &>/dev/null
>>  	[ $? -eq 0 ] || _notrun "$BTRFS_UTIL_PROG too old (must support $cmd)"
>> +
>> +	test -z "$param" && return
>> +
>> +	# if $param is an option, replace leading "-"s for grep
>> +	if [ ${param:0:1} == "-" ]; then
>> +		param=$(echo $param | sed 's/^-*//')
>> +		$BTRFS_UTIL_PROG $cmd --help | grep $param > /dev/null || \
>
> Use "grep -w" to be safer? And "-q" instead of "> /dev/null"

Right, -w is much safer. I'll use "-q" in next version.

>
>> +			_not_run "$BTRFS_UTIL_PROG too old (must support $cmd $param)"
>
> $param here is without leading "-", so the _notrun message is kind of
> misleading. And _notrun not _not_run :)

Right, I'll using safe_param.

Thanks,
Qu

>
> Thanks,
> Eryu
>
>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code
  2016-12-08  5:27   ` Eryu Guan
@ 2016-12-08  5:49     ` Qu Wenruo
  0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2016-12-08  5:49 UTC (permalink / raw)
  To: Eryu Guan; +Cc: linux-btrfs, fstests



At 12/08/2016 01:27 PM, Eryu Guan wrote:
> On Thu, Dec 08, 2016 at 10:04:56AM +0800, Qu Wenruo wrote:
>> Introduce new _require_btrfs_qgroup_report function, which will check
>> the accessibility to "btrfs check --qgroup-report", then set a global
>> flag to info _check_scratch_fs() to do extra qgroup check.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  common/rc       | 22 ++++++++++++++++++++++
>
> This needs rebase too.
>
>>  tests/btrfs/022 |  5 +++++
>>  tests/btrfs/028 |  5 ++---
>>  tests/btrfs/042 |  6 ++----
>>  tests/btrfs/099 |  1 +
>>  tests/btrfs/104 | 20 +++++---------------
>>  tests/btrfs/122 | 10 +++-------
>>  tests/btrfs/123 |  5 ++---
>>  8 files changed, 42 insertions(+), 32 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index 1703232..bce3a09 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2624,6 +2624,20 @@ _check_btrfs_filesystem()
>>          mountpoint=`_umount_or_remount_ro $device`
>>      fi
>>
>> +    # Check qgroup numbers
>> +    if [ "$BTRFS_NEED_QGROUP_REPORT" == "yes" ];then
>
> So we can bypass the _require_btrfs_qgroup_report check if we set
> BTRFS_NEED_QGROUP_REPORT to "yes" directly, right? How about doing
> something like _require_scratch do, e.g. touching some signal file in
> $RESULT_DIR and only do qgroup check if that file exists?

Nice idea.

>
>> +	    btrfsck $device --qgroup-report > $tmp.qgroup_report 2>&1
>
> Shouldn't "$BTRFS_UTIL_PROG check $device ..." be used for new code? I
> might be wrong on this, I think btrfsck is deprecated.

Oh, the code in common/btrfs is just too old.
I'll update them together in next version.

Thanks,
Qu

>
> Thanks,
> Eryu
>
>



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-12-08  5:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-08  2:04 [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Qu Wenruo
2016-12-08  2:04 ` [PATCH 2/2] fstests: btrfs: Use _require_btrfs_qgroup_report to replace open code Qu Wenruo
2016-12-08  5:27   ` Eryu Guan
2016-12-08  5:49     ` Qu Wenruo
2016-12-08  4:00 ` [PATCH 1/2] fstests: common: rename and enhance _require_btrfs to _require_btrfs_subcommand Eryu Guan
2016-12-08  5:44   ` Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox