[PATCH] common: use xfs_io to obtain the xfsprogs version

[PATCH] common: use xfs_io to obtain the xfsprogs version

[David Disseldorp]

xfs_db is currently used for this, but is otherwise only used for XFS
specific tests. This change allows for the generic tests to be run on a
system without the xfs_db binary.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/config | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/config b/common/config
index 6cce7ce..1604aa9 100644
--- a/common/config
+++ b/common/config
@@ -226,11 +226,11 @@ export UDEV_SETTLE_PROG
 # Generate a comparable xfsprogs version number in the form of
 # major * 10000 + minor * 100 + release
 #
-# $ xfs_db -V
-# xfs_db version 2.9.7
+# $ xfs_io -V
+# xfs_io version 2.9.7
 #
 # so, 2.9.7 = 20907
-_version=`$XFS_DB_PROG -V | $AWK_PROG '
+_version=`$XFS_IO_PROG -V | $AWK_PROG '
 	/version/ {
 		if (split($3,ver,".") == 3)
 			print (ver[1] * 10000) + (ver[2] * 100) + ver[3];
-- 
2.10.2

Re: [PATCH] common: use xfs_io to obtain the xfsprogs version

[Eric Sandeen]

On 1/11/17 4:48 AM, David Disseldorp wrote:
> xfs_db is currently used for this, but is otherwise only used for XFS
> specific tests. This change allows for the generic tests to be run on a
> system without the xfs_db binary.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  common/config | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/config b/common/config
> index 6cce7ce..1604aa9 100644
> --- a/common/config
> +++ b/common/config
> @@ -226,11 +226,11 @@ export UDEV_SETTLE_PROG
>  # Generate a comparable xfsprogs version number in the form of
>  # major * 10000 + minor * 100 + release
>  #
> -# $ xfs_db -V
> -# xfs_db version 2.9.7
> +# $ xfs_io -V
> +# xfs_io version 2.9.7
>  #
>  # so, 2.9.7 = 20907
> -_version=`$XFS_DB_PROG -V | $AWK_PROG '
> +_version=`$XFS_IO_PROG -V | $AWK_PROG '
>  	/version/ {
>  		if (split($3,ver,".") == 3)
>  			print (ver[1] * 10000) + (ver[2] * 100) + ver[3];
> 

Re: [PATCH] common: use xfs_io to obtain the xfsprogs version

[Eryu Guan]

On Wed, Jan 11, 2017 at 11:48:20AM +0100, David Disseldorp wrote:
> xfs_db is currently used for this, but is otherwise only used for XFS
> specific tests. This change allows for the generic tests to be run on a
> system without the xfs_db binary.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  common/config | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/common/config b/common/config
> index 6cce7ce..1604aa9 100644
> --- a/common/config
> +++ b/common/config
> @@ -226,11 +226,11 @@ export UDEV_SETTLE_PROG
>  # Generate a comparable xfsprogs version number in the form of
>  # major * 10000 + minor * 100 + release
>  #
> -# $ xfs_db -V
> -# xfs_db version 2.9.7
> +# $ xfs_io -V
> +# xfs_io version 2.9.7
>  #
>  # so, 2.9.7 = 20907
> -_version=`$XFS_DB_PROG -V | $AWK_PROG '
> +_version=`$XFS_IO_PROG -V | $AWK_PROG '

I noticed that this $_version is only assigned to XFSPROGS_VERSION, and
the only user of $XFSPROGS_VERSION is xfs/188.

if [ $XFSPROGS_VERSION -lt 21000 ]; then
    _notrun "this test requires case-insensitive support"
fi

That's a test added in 2008, now we avoid skipping tests by checking on
some version number, we tend to add new _require rules to actually test
for the requirements. e.g. for xfs/188, I think we can have:

in common/xfs:
# this test requires mkfs.xfs have case-insensitive naming support
_require_xfs_mkfs_ciname()
{
	_scratch_mkfs_xfs_supported -n version=ci >/dev/null 2>&1 \
		|| _notrun "need case-insensitive naming support in mkfs.xfs"
}

And remove all the version detection & checking code from common/config
and xfs/188, and call the new _require rule in the test.

Thanks,
Eryu

Re: [PATCH] common: use xfs_io to obtain the xfsprogs version

[David Disseldorp]

On Thu, 12 Jan 2017 11:51:24 +0800, Eryu Guan wrote:
...
> I noticed that this $_version is only assigned to XFSPROGS_VERSION, and
> the only user of $XFSPROGS_VERSION is xfs/188.
> 
> if [ $XFSPROGS_VERSION -lt 21000 ]; then
>     _notrun "this test requires case-insensitive support"
> fi
> 
> That's a test added in 2008, now we avoid skipping tests by checking on
> some version number, we tend to add new _require rules to actually test
> for the requirements. e.g. for xfs/188, I think we can have:
> 
> in common/xfs:
> # this test requires mkfs.xfs have case-insensitive naming support
> _require_xfs_mkfs_ciname()
> {
> 	_scratch_mkfs_xfs_supported -n version=ci >/dev/null 2>&1 \
> 		|| _notrun "need case-insensitive naming support in mkfs.xfs"
> }
> 
> And remove all the version detection & checking code from common/config
> and xfs/188, and call the new _require rule in the test.

This seems much nicer, Eryu. I'm giving it a spin now, and will send a
patch once tested - thanks for the feedback.

Cheers, David

[PATCH 1/3] xfs/188: add and use _require_xfs_mkfs_ciname

[David Disseldorp]

Use an explicit mkfs -n version=ci test to check whether the test should
run, instead of checking the xfsprogs version.

Suggested-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/xfs    | 7 +++++++
 tests/xfs/188 | 6 +-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/common/xfs b/common/xfs
index fbd139a..767a481 100644
--- a/common/xfs
+++ b/common/xfs
@@ -587,3 +587,10 @@ _require_meta_uuid()
 	   || _notrun "Kernel doesn't support meta_uuid feature"
 	_scratch_unmount
 }
+
+# this test requires mkfs.xfs have case-insensitive naming support
+_require_xfs_mkfs_ciname()
+{
+	_scratch_mkfs_xfs_supported -n version=ci >/dev/null 2>&1 \
+		|| _notrun "need case-insensitive naming support in mkfs.xfs"
+}
diff --git a/tests/xfs/188 b/tests/xfs/188
index 6332085..8798ecf 100755
--- a/tests/xfs/188
+++ b/tests/xfs/188
@@ -52,12 +52,8 @@ _cleanup()
 # real QA test starts here
 _supported_fs xfs
 _supported_os IRIX Linux
-
-if [ $XFSPROGS_VERSION -lt 21000 ]; then
-    _notrun "this test requires case-insensitive support"
-fi
-
 _require_scratch
+_require_xfs_mkfs_ciname
 rm -f $seqres.full
 
 _scratch_mkfs -n version=ci >/dev/null 2>&1
-- 
2.10.2

[PATCH 2/3] common: remove unused XFSPROGS_VERSION check

[David Disseldorp]

This was only used to check for mkfs.xfs -n version=ci support, which is
carried in xfsprogs >= 2.10. _require_xfs_mkfs_ciname() is now used to
explicitly check for the mkfs parameter.

Suggested-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/config | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/common/config b/common/config
index 6cce7ce..0706aca 100644
--- a/common/config
+++ b/common/config
@@ -223,21 +223,6 @@ if [ "$UDEV_SETTLE_PROG" == "" ]; then
 fi
 export UDEV_SETTLE_PROG
 
-# Generate a comparable xfsprogs version number in the form of
-# major * 10000 + minor * 100 + release
-#
-# $ xfs_db -V
-# xfs_db version 2.9.7
-#
-# so, 2.9.7 = 20907
-_version=`$XFS_DB_PROG -V | $AWK_PROG '
-	/version/ {
-		if (split($3,ver,".") == 3)
-			print (ver[1] * 10000) + (ver[2] * 100) + ver[3];
-	}'`
-[ -z "$_version" ] && _fatal "xfsprogs version cannot be found"
-export XFSPROGS_VERSION="$_version"
-
 case "$HOSTOS" in
     IRIX*)
         export MKFS_XFS_PROG="`set_prog_path mkfs_xfs`"
-- 
2.10.2

[PATCH 3/3] common: remove tmp.mkfs[err/std] files by name

[David Disseldorp]

Without any $tmp suffix, users silly enough to run xfstests without
mktemp present will unintentionally "rm -f *".

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/rc  | 2 +-
 common/xfs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 37d0fc8..4cbe00f 100644
--- a/common/rc
+++ b/common/rc
@@ -464,7 +464,7 @@ _scratch_do_mkfs()
 	cat $tmp.mkfsstd
 	eval "cat $tmp.mkfserr | $mkfs_filter" >&2
 
-	rm -f $tmp*
+	rm -f $tmp.mkfsstd $tmp.mkfserr
 	return $mkfs_status
 }
 
diff --git a/common/xfs b/common/xfs
index 767a481..582fcbe 100644
--- a/common/xfs
+++ b/common/xfs
@@ -104,7 +104,7 @@ _scratch_mkfs_xfs()
 	# output mkfs stdout and stderr
 	cat $tmp.mkfsstd
 	cat $tmp.mkfserr >&2
-	rm -f $tmp*
+	rm -f $tmp.mkfsstd $tmp.mkfserr
 
 	return $mkfs_status
 }
-- 
2.10.2

Re: [PATCH 3/3] common: remove tmp.mkfs[err/std] files by name

[Eryu Guan]

On Thu, Jan 12, 2017 at 06:18:49PM +0100, David Disseldorp wrote:
> Without any $tmp suffix, users silly enough to run xfstests without
> mktemp present will unintentionally "rm -f *".

mktemp creates a file too, removing $tmp.xxx files won't remove $tmp
file itself. So along with removing them by name, I'm thinking about
initializing tmp with /tmp/$$._mkfs (or something like that) as well.

I have another tmp file cleanup patch[1] pending in the list, would you
like to do all the cleanup fix in your patch? Or I can fold your patch
to mine and send out v3.

Thanks,
Eryu

[1] http://www.spinics.net/lists/fstests/msg04953.html

Re: [PATCH 3/3] common: remove tmp.mkfs[err/std] files by name

[David Disseldorp]

On Fri, 13 Jan 2017 12:24:04 +0800, Eryu Guan wrote:

> On Thu, Jan 12, 2017 at 06:18:49PM +0100, David Disseldorp wrote:
> > Without any $tmp suffix, users silly enough to run xfstests without
> > mktemp present will unintentionally "rm -f *".  
> 
> mktemp creates a file too, removing $tmp.xxx files won't remove $tmp
> file itself. So along with removing them by name, I'm thinking about
> initializing tmp with /tmp/$$._mkfs (or something like that) as well.

Oh, yes I missed the extra $tmp rm parameter.

> I have another tmp file cleanup patch[1] pending in the list, would you
> like to do all the cleanup fix in your patch? Or I can fold your patch
> to mine and send out v3.

Please fold it into your patchset.

Cheers, David