[PATCH 0/2] CIFS support for XFS test suite

[PATCH 0/2] CIFS support for XFS test suite

[Pavel Shilovsky]

These are two patches that adds CIFS support to XFS tests.

The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.

With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.

Pavel Shilovsky (2):
  common: add cifs support
  common: add a directory tree for cifs tests

 README              |  5 +++--
 check               |  2 ++
 common/config       | 17 +++++++++++++----
 common/rc           | 33 +++++++++++++++++++++++++++++++++
 tests/cifs/Makefile | 14 ++++++++++++++
 tests/cifs/group    |  5 +++++
 tests/generic/013   |  7 ++++++-
 7 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 tests/cifs/Makefile
 create mode 100644 tests/cifs/group

-- 
1.9.1

[PATCH 0/2] CIFS support for XFS test suite

[Pavel Shilovsky]

These are two patches that adds CIFS support to XFS tests.

The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.

With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.

Pavel Shilovsky (2):
  common: add cifs support
  common: add a directory tree for cifs tests

 README              |  5 +++--
 check               |  2 ++
 common/config       | 17 +++++++++++++----
 common/rc           | 33 +++++++++++++++++++++++++++++++++
 tests/cifs/Makefile | 14 ++++++++++++++
 tests/cifs/group    |  5 +++++
 tests/generic/013   |  7 ++++++-
 7 files changed, 76 insertions(+), 7 deletions(-)
 create mode 100644 tests/cifs/Makefile
 create mode 100644 tests/cifs/group

-- 
1.9.1

[PATCH 1/2] common: add cifs support

[Pavel Shilovsky]

Pass -cifs argument from command line to enable cifs testing.

Reviewed-by: David Disseldorp <ddiss@suse.de>
Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 README            |  5 +++--
 check             |  2 ++
 common/config     | 17 +++++++++++++----
 common/rc         | 33 +++++++++++++++++++++++++++++++++
 tests/generic/013 |  7 ++++++-
 5 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/README b/README
index b299c8f..51d0a03 100644
--- a/README
+++ b/README
@@ -91,14 +91,15 @@ Running tests:
     - By default the tests suite will run xfs tests:
     - ./check '*/001' '*/002' '*/003'
     - ./check '*/06?'
-    - You can explicitly specify NFS, otherwise the filesystem type will be
-      autodetected from $TEST_DEV:
+    - You can explicitly specify NFS or CIFS, otherwise the filesystem type will
+      be autodetected from $TEST_DEV:
       ./check -nfs [test(s)]
     - Groups of tests maybe ran by: ./check -g [group(s)]
       See the 'group' file for details on groups
     - for udf tests: ./check -udf [test(s)]
       Running all the udf tests: ./check -udf -g udf
     - for running nfs tests: ./check -nfs [test(s)]
+    - for running cifs tests: ./check -cifs [test(s)]
     - To randomize test order: ./check -r [test(s)]
 
     
diff --git a/check b/check
index 77c6559..42a1ac2 100755
--- a/check
+++ b/check
@@ -68,6 +68,7 @@ usage()
 
 check options
     -nfs                test NFS
+    -cifs               test CIFS
     -tmpfs              test TMPFS
     -l			line mode diff
     -udiff		show unified diff (default)
@@ -205,6 +206,7 @@ while [ $# -gt 0 ]; do
 	-\? | -h | --help) usage ;;
 
 	-nfs)	FSTYP=nfs ;;
+	-cifs)	FSTYP=cifs ;;
 	-tmpfs)	FSTYP=tmpfs ;;
 
 	-g)	group=$2 ; shift ;
diff --git a/common/config b/common/config
index 10cc6fe..045a3e4 100644
--- a/common/config
+++ b/common/config
@@ -206,6 +206,7 @@ case "$HOSTOS" in
         export MKFS_UDF_PROG="`set_prog_path mkfs_udf`"
         export XFS_FSR_PROG="`set_prog_path /usr/etc/fsr_xfs`"
         export MKFS_NFS_PROG="false"
+        export MKFS_CIFS_PROG="false"
         ;;
     Linux)
         export MKFS_XFS_PROG="`set_prog_path mkfs.xfs`"
@@ -215,6 +216,7 @@ case "$HOSTOS" in
         export BTRFS_SHOW_SUPER_PROG="`set_prog_path btrfs-show-super`"
         export XFS_FSR_PROG="`set_prog_path xfs_fsr`"
         export MKFS_NFS_PROG="false"
+        export MKFS_CIFS_PROG="false"
         ;;
 esac
 
@@ -228,6 +230,7 @@ fi
 
 _mount_opts()
 {
+
 	case $FSTYP in
 	xfs)
 		export MOUNT_OPTIONS=$XFS_MOUNT_OPTIONS
@@ -238,6 +241,9 @@ _mount_opts()
 	nfs)
 		export MOUNT_OPTIONS=$NFS_MOUNT_OPTIONS
 		;;
+	cifs)
+		export MOUNT_OPTIONS=$CIFS_MOUNT_OPTIONS
+		;;
 	ext2|ext3|ext4|ext4dev)
 		# acls & xattrs aren't turned on by default on ext$FOO
 		export MOUNT_OPTIONS="-o acl,user_xattr $EXT_MOUNT_OPTIONS"
@@ -273,6 +279,9 @@ _mkfs_opts()
 	nfs)
 		export MKFS_OPTIONS=$NFS_MKFS_OPTIONS
 		;;
+	cifs)
+		export MKFS_OPTIONS=$CIFS_MKFS_OPTIONS
+		;;
 	reiserfs)
 		export MKFS_OPTIONS="$REISERFS_MKFS_OPTIONS -q"
 		;;
@@ -408,9 +417,9 @@ get_next_config() {
 		exit 1
 	fi
 
-	echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
+	echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
 	if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
-		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
+		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
 		exit 1
 	fi
 
@@ -431,9 +440,9 @@ get_next_config() {
 		export SCRATCH_DEV_NOT_SET=true
 	fi
 
-	echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
+	echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
 	if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
-		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
+		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
 		exit 1
 	fi
 
diff --git a/common/rc b/common/rc
index 16da898..dbc99ee 100644
--- a/common/rc
+++ b/common/rc
@@ -107,6 +107,8 @@ case "$FSTYP" in
 	 ;;
     nfs)
 	 ;;
+    cifs)
+	 ;;
 esac
 
 # make sure we have a standard umask
@@ -148,6 +150,11 @@ _test_options()
     type=$1
     TEST_OPTIONS=""
 
+    if [ "$FSTYP" = "cifs" ]; then
+        TEST_OPTIONS="$MOUNT_OPTIONS"
+        return
+    fi
+
     if [ "$FSTYP" != "xfs" ]; then
         return
     fi
@@ -497,6 +504,9 @@ _test_mkfs()
     nfs*)
 	# do nothing for nfs
 	;;
+    cifs)
+	# do nothing for cifs
+	;;
     udf)
         $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
 	;;
@@ -518,6 +528,9 @@ _scratch_mkfs()
     nfs*)
 	# do nothing for nfs
 	;;
+    cifs)
+	# do nothing for cifs
+	;;
     udf)
         $MKFS_UDF_PROG $MKFS_OPTIONS $* $SCRATCH_DEV > /dev/null
 	;;
@@ -967,6 +980,9 @@ _require_scratch()
 	nfs*)
                  _notrun "requires a scratch device"
 		 ;;
+	cifs)
+		_notrun "requires a scratch device"
+		;;
 	tmpfs)
 		if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ];
 		then
@@ -1016,6 +1032,17 @@ _require_test()
 	nfs*)
                  _notrun "requires a test device"
 		 ;;
+	cifs)
+		echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
+		if [ -z "$TEST_DEV" -o "$?" != "0" ];
+		then
+			_notrun "this test requires a valid \$TEST_DEV"
+		fi
+		if [ ! -d "$TEST_DIR" ];
+		then
+		     _notrun "this test requires a valid \$TEST_DIR"
+		fi
+		;;
 	tmpfs)
 		if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ];
 		then
@@ -1806,6 +1833,9 @@ _check_test_fs()
     nfs)
 	# no way to check consistency for nfs
 	;;
+    cifs)
+	# no way to check consistency for cifs
+	;;
     udf)
 	# do nothing for now
 	;;
@@ -1844,6 +1874,9 @@ _check_scratch_fs()
     nfs*)
 	# Don't know how to check an NFS filesystem, yet.
 	;;
+    cifs)
+	# Don't know how to check a CIFS filesystem, yet.
+	;;
     btrfs)
 	_check_btrfs_filesystem $device
 	;;
diff --git a/tests/generic/013 b/tests/generic/013
index 93d9904..ae57c67 100755
--- a/tests/generic/013
+++ b/tests/generic/013
@@ -35,7 +35,12 @@ _cleanup()
 {
     cd /
     # we might get here with a RO FS
-    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
+    REMOUNT_OPTIONS="remount,rw"
+    if [ "$FSTYP" = "cifs" ];
+    then
+        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
+    fi
+    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
     # now remove fsstress directory.
     # N.B. rm(1) on IRIX can find problems when building up a long pathname
     # such that what it has is greater the 1024 chars and will
-- 
1.9.1

[PATCH 1/2] common: add cifs support

[Pavel Shilovsky]

Pass -cifs argument from command line to enable cifs testing.

Reviewed-by: David Disseldorp <ddiss-l3A5Bk7waGM@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 README            |  5 +++--
 check             |  2 ++
 common/config     | 17 +++++++++++++----
 common/rc         | 33 +++++++++++++++++++++++++++++++++
 tests/generic/013 |  7 ++++++-
 5 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/README b/README
index b299c8f..51d0a03 100644
--- a/README
+++ b/README
@@ -91,14 +91,15 @@ Running tests:
     - By default the tests suite will run xfs tests:
     - ./check '*/001' '*/002' '*/003'
     - ./check '*/06?'
-    - You can explicitly specify NFS, otherwise the filesystem type will be
-      autodetected from $TEST_DEV:
+    - You can explicitly specify NFS or CIFS, otherwise the filesystem type will
+      be autodetected from $TEST_DEV:
       ./check -nfs [test(s)]
     - Groups of tests maybe ran by: ./check -g [group(s)]
       See the 'group' file for details on groups
     - for udf tests: ./check -udf [test(s)]
       Running all the udf tests: ./check -udf -g udf
     - for running nfs tests: ./check -nfs [test(s)]
+    - for running cifs tests: ./check -cifs [test(s)]
     - To randomize test order: ./check -r [test(s)]
 
     
diff --git a/check b/check
index 77c6559..42a1ac2 100755
--- a/check
+++ b/check
@@ -68,6 +68,7 @@ usage()
 
 check options
     -nfs                test NFS
+    -cifs               test CIFS
     -tmpfs              test TMPFS
     -l			line mode diff
     -udiff		show unified diff (default)
@@ -205,6 +206,7 @@ while [ $# -gt 0 ]; do
 	-\? | -h | --help) usage ;;
 
 	-nfs)	FSTYP=nfs ;;
+	-cifs)	FSTYP=cifs ;;
 	-tmpfs)	FSTYP=tmpfs ;;
 
 	-g)	group=$2 ; shift ;
diff --git a/common/config b/common/config
index 10cc6fe..045a3e4 100644
--- a/common/config
+++ b/common/config
@@ -206,6 +206,7 @@ case "$HOSTOS" in
         export MKFS_UDF_PROG="`set_prog_path mkfs_udf`"
         export XFS_FSR_PROG="`set_prog_path /usr/etc/fsr_xfs`"
         export MKFS_NFS_PROG="false"
+        export MKFS_CIFS_PROG="false"
         ;;
     Linux)
         export MKFS_XFS_PROG="`set_prog_path mkfs.xfs`"
@@ -215,6 +216,7 @@ case "$HOSTOS" in
         export BTRFS_SHOW_SUPER_PROG="`set_prog_path btrfs-show-super`"
         export XFS_FSR_PROG="`set_prog_path xfs_fsr`"
         export MKFS_NFS_PROG="false"
+        export MKFS_CIFS_PROG="false"
         ;;
 esac
 
@@ -228,6 +230,7 @@ fi
 
 _mount_opts()
 {
+
 	case $FSTYP in
 	xfs)
 		export MOUNT_OPTIONS=$XFS_MOUNT_OPTIONS
@@ -238,6 +241,9 @@ _mount_opts()
 	nfs)
 		export MOUNT_OPTIONS=$NFS_MOUNT_OPTIONS
 		;;
+	cifs)
+		export MOUNT_OPTIONS=$CIFS_MOUNT_OPTIONS
+		;;
 	ext2|ext3|ext4|ext4dev)
 		# acls & xattrs aren't turned on by default on ext$FOO
 		export MOUNT_OPTIONS="-o acl,user_xattr $EXT_MOUNT_OPTIONS"
@@ -273,6 +279,9 @@ _mkfs_opts()
 	nfs)
 		export MKFS_OPTIONS=$NFS_MKFS_OPTIONS
 		;;
+	cifs)
+		export MKFS_OPTIONS=$CIFS_MKFS_OPTIONS
+		;;
 	reiserfs)
 		export MKFS_OPTIONS="$REISERFS_MKFS_OPTIONS -q"
 		;;
@@ -408,9 +417,9 @@ get_next_config() {
 		exit 1
 	fi
 
-	echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
+	echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
 	if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
-		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
+		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
 		exit 1
 	fi
 
@@ -431,9 +440,9 @@ get_next_config() {
 		export SCRATCH_DEV_NOT_SET=true
 	fi
 
-	echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
+	echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
 	if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
-		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
+		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
 		exit 1
 	fi
 
diff --git a/common/rc b/common/rc
index 16da898..dbc99ee 100644
--- a/common/rc
+++ b/common/rc
@@ -107,6 +107,8 @@ case "$FSTYP" in
 	 ;;
     nfs)
 	 ;;
+    cifs)
+	 ;;
 esac
 
 # make sure we have a standard umask
@@ -148,6 +150,11 @@ _test_options()
     type=$1
     TEST_OPTIONS=""
 
+    if [ "$FSTYP" = "cifs" ]; then
+        TEST_OPTIONS="$MOUNT_OPTIONS"
+        return
+    fi
+
     if [ "$FSTYP" != "xfs" ]; then
         return
     fi
@@ -497,6 +504,9 @@ _test_mkfs()
     nfs*)
 	# do nothing for nfs
 	;;
+    cifs)
+	# do nothing for cifs
+	;;
     udf)
         $MKFS_UDF_PROG $MKFS_OPTIONS $* $TEST_DEV > /dev/null
 	;;
@@ -518,6 +528,9 @@ _scratch_mkfs()
     nfs*)
 	# do nothing for nfs
 	;;
+    cifs)
+	# do nothing for cifs
+	;;
     udf)
         $MKFS_UDF_PROG $MKFS_OPTIONS $* $SCRATCH_DEV > /dev/null
 	;;
@@ -967,6 +980,9 @@ _require_scratch()
 	nfs*)
                  _notrun "requires a scratch device"
 		 ;;
+	cifs)
+		_notrun "requires a scratch device"
+		;;
 	tmpfs)
 		if [ -z "$SCRATCH_DEV" -o ! -d "$SCRATCH_MNT" ];
 		then
@@ -1016,6 +1032,17 @@ _require_test()
 	nfs*)
                  _notrun "requires a test device"
 		 ;;
+	cifs)
+		echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
+		if [ -z "$TEST_DEV" -o "$?" != "0" ];
+		then
+			_notrun "this test requires a valid \$TEST_DEV"
+		fi
+		if [ ! -d "$TEST_DIR" ];
+		then
+		     _notrun "this test requires a valid \$TEST_DIR"
+		fi
+		;;
 	tmpfs)
 		if [ -z "$TEST_DEV" -o ! -d "$TEST_DIR" ];
 		then
@@ -1806,6 +1833,9 @@ _check_test_fs()
     nfs)
 	# no way to check consistency for nfs
 	;;
+    cifs)
+	# no way to check consistency for cifs
+	;;
     udf)
 	# do nothing for now
 	;;
@@ -1844,6 +1874,9 @@ _check_scratch_fs()
     nfs*)
 	# Don't know how to check an NFS filesystem, yet.
 	;;
+    cifs)
+	# Don't know how to check a CIFS filesystem, yet.
+	;;
     btrfs)
 	_check_btrfs_filesystem $device
 	;;
diff --git a/tests/generic/013 b/tests/generic/013
index 93d9904..ae57c67 100755
--- a/tests/generic/013
+++ b/tests/generic/013
@@ -35,7 +35,12 @@ _cleanup()
 {
     cd /
     # we might get here with a RO FS
-    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
+    REMOUNT_OPTIONS="remount,rw"
+    if [ "$FSTYP" = "cifs" ];
+    then
+        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
+    fi
+    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
     # now remove fsstress directory.
     # N.B. rm(1) on IRIX can find problems when building up a long pathname
     # such that what it has is greater the 1024 chars and will
-- 
1.9.1

Re: [PATCH 1/2] common: add cifs support

[Christoph Hellwig]

On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
> Pass -cifs argument from command line to enable cifs testing.

Looks mostly fine, but a few nitpicks below:

>  _mount_opts()
>  {
> +
>  	case $FSTYP in

Remove this spurious new empty line, please.

> -	echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
> +	echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
>  	if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
> -		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
> +		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
>  		exit 1
>  	fi

I'd just generalize this to ".. is not a block device or network
filesystem"

> -	echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
> +	echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
>  	if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
> -		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
> +		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
>  		exit 1
>  	fi

Same here.

>  
>  # make sure we have a standard umask
> @@ -148,6 +150,11 @@ _test_options()
>      type=$1
>      TEST_OPTIONS=""
>  
> +    if [ "$FSTYP" = "cifs" ]; then
> +        TEST_OPTIONS="$MOUNT_OPTIONS"
> +        return
> +    fi

What's this for?  This doesn't really make sense to me as this function adds
mkfs/mount options to the already normally specified ones.

> +	cifs)
> +		echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
> +		if [ -z "$TEST_DEV" -o "$?" != "0" ];
> +		then
> +			_notrun "this test requires a valid \$TEST_DEV"
> +		fi
> +		if [ ! -d "$TEST_DIR" ];
> +		then
> +		     _notrun "this test requires a valid \$TEST_DIR"
> +		fi
> +		;;

Please put the then on the same line as the if for new code.

> diff --git a/tests/generic/013 b/tests/generic/013
> index 93d9904..ae57c67 100755
> --- a/tests/generic/013
> +++ b/tests/generic/013
> @@ -35,7 +35,12 @@ _cleanup()
>  {
>      cd /
>      # we might get here with a RO FS
> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
> +    REMOUNT_OPTIONS="remount,rw"
> +    if [ "$FSTYP" = "cifs" ];
> +    then
> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
> +    fi
> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1

This looks wrong and will need an explanation.

Re: [PATCH 1/2] common: add cifs support

[Christoph Hellwig]

On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
> Pass -cifs argument from command line to enable cifs testing.

Looks mostly fine, but a few nitpicks below:

>  _mount_opts()
>  {
> +
>  	case $FSTYP in

Remove this spurious new empty line, please.

> -	echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
> +	echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
>  	if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
> -		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
> +		echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
>  		exit 1
>  	fi

I'd just generalize this to ".. is not a block device or network
filesystem"

> -	echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
> +	echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
>  	if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
> -		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
> +		echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
>  		exit 1
>  	fi

Same here.

>  
>  # make sure we have a standard umask
> @@ -148,6 +150,11 @@ _test_options()
>      type=$1
>      TEST_OPTIONS=""
>  
> +    if [ "$FSTYP" = "cifs" ]; then
> +        TEST_OPTIONS="$MOUNT_OPTIONS"
> +        return
> +    fi

What's this for?  This doesn't really make sense to me as this function adds
mkfs/mount options to the already normally specified ones.

> +	cifs)
> +		echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
> +		if [ -z "$TEST_DEV" -o "$?" != "0" ];
> +		then
> +			_notrun "this test requires a valid \$TEST_DEV"
> +		fi
> +		if [ ! -d "$TEST_DIR" ];
> +		then
> +		     _notrun "this test requires a valid \$TEST_DIR"
> +		fi
> +		;;

Please put the then on the same line as the if for new code.

> diff --git a/tests/generic/013 b/tests/generic/013
> index 93d9904..ae57c67 100755
> --- a/tests/generic/013
> +++ b/tests/generic/013
> @@ -35,7 +35,12 @@ _cleanup()
>  {
>      cd /
>      # we might get here with a RO FS
> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
> +    REMOUNT_OPTIONS="remount,rw"
> +    if [ "$FSTYP" = "cifs" ];
> +    then
> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
> +    fi
> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1

This looks wrong and will need an explanation.

Re: [PATCH 1/2] common: add cifs support

[Pavel Shilovsky]

2014-08-23 15:56 GMT+04:00 Christoph Hellwig <hch@infradead.org>:
> On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
>> Pass -cifs argument from command line to enable cifs testing.
>
> Looks mostly fine, but a few nitpicks below:

Thank you for reviewing this.

>
>>  _mount_opts()
>>  {
>> +
>>       case $FSTYP in
>
> Remove this spurious new empty line, please.

This was added by mistake - will remove.

>
>> -     echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
>> +     echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
>>       if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
>> -             echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
>> +             echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
>>               exit 1
>>       fi
>
> I'd just generalize this to ".. is not a block device or network
> filesystem"

Agree.

>
>> -     echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
>> +     echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
>>       if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
>> -             echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
>> +             echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
>>               exit 1
>>       fi
>
> Same here.
>
>>
>>  # make sure we have a standard umask
>> @@ -148,6 +150,11 @@ _test_options()
>>      type=$1
>>      TEST_OPTIONS=""
>>
>> +    if [ "$FSTYP" = "cifs" ]; then
>> +        TEST_OPTIONS="$MOUNT_OPTIONS"
>> +        return
>> +    fi
>
> What's this for?  This doesn't really make sense to me as this function adds
> mkfs/mount options to the already normally specified ones.

1) We included common/rc -- init_rc() is called.
2) init_rc() checks if $TEST_DEV is mounted and if not -- calls _test_mount().
3) _test_mount() calls _test_options() that:
  a) initializes TEST_OPTIONS=''
  b) adds rtdev,logdev options to TEST_OPTIONS for XFS; for others
filesystems it simply returns leaving TEST_OPTIONS as empty string.

That's why we need to initialize TEST_OPTIONS as MOUN_OPTIONS for CIFS
because we can't mount a remote share without specifying a username,
password, etc.

>> +     cifs)
>> +             echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
>> +             if [ -z "$TEST_DEV" -o "$?" != "0" ];
>> +             then
>> +                     _notrun "this test requires a valid \$TEST_DEV"
>> +             fi
>> +             if [ ! -d "$TEST_DIR" ];
>> +             then
>> +                  _notrun "this test requires a valid \$TEST_DIR"
>> +             fi
>> +             ;;
>
> Please put the then on the same line as the if for new code.

Ok.

>
>> diff --git a/tests/generic/013 b/tests/generic/013
>> index 93d9904..ae57c67 100755
>> --- a/tests/generic/013
>> +++ b/tests/generic/013
>> @@ -35,7 +35,12 @@ _cleanup()
>>  {
>>      cd /
>>      # we might get here with a RO FS
>> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
>> +    REMOUNT_OPTIONS="remount,rw"
>> +    if [ "$FSTYP" = "cifs" ];
>> +    then
>> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
>> +    fi
>> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
>
> This looks wrong and will need an explanation.

We can't remount the existing CIFS mount without specifying username
and password. Also we need to keep others options as well since they
are user-defined (e.g. nounix, noserverino, etc) while during
remounting mount options are changed to the specified ones.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/2] common: add cifs support

[Pavel Shilovsky]

2014-08-23 15:56 GMT+04:00 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
> On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
>> Pass -cifs argument from command line to enable cifs testing.
>
> Looks mostly fine, but a few nitpicks below:

Thank you for reviewing this.

>
>>  _mount_opts()
>>  {
>> +
>>       case $FSTYP in
>
> Remove this spurious new empty line, please.

This was added by mistake - will remove.

>
>> -     echo $TEST_DEV | grep -q ":" > /dev/null 2>&1
>> +     echo $TEST_DEV | grep -qE ":|//" > /dev/null 2>&1
>>       if [ ! -b "$TEST_DEV" -a "$?" != "0" ]; then
>> -             echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS filesystem"
>> +             echo "common/config: Error: \$TEST_DEV ($TEST_DEV) is not a block device or a NFS or CIFS filesystem"
>>               exit 1
>>       fi
>
> I'd just generalize this to ".. is not a block device or network
> filesystem"

Agree.

>
>> -     echo $SCRATCH_DEV | grep -q ":" > /dev/null 2>&1
>> +     echo $SCRATCH_DEV | grep -qE ":|//" > /dev/null 2>&1
>>       if [ ! -z "$SCRATCH_DEV" -a ! -b "$SCRATCH_DEV" -a "$?" != "0" ]; then
>> -             echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS filesystem"
>> +             echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) is not a block device or a NFS or CIFS filesystem"
>>               exit 1
>>       fi
>
> Same here.
>
>>
>>  # make sure we have a standard umask
>> @@ -148,6 +150,11 @@ _test_options()
>>      type=$1
>>      TEST_OPTIONS=""
>>
>> +    if [ "$FSTYP" = "cifs" ]; then
>> +        TEST_OPTIONS="$MOUNT_OPTIONS"
>> +        return
>> +    fi
>
> What's this for?  This doesn't really make sense to me as this function adds
> mkfs/mount options to the already normally specified ones.

1) We included common/rc -- init_rc() is called.
2) init_rc() checks if $TEST_DEV is mounted and if not -- calls _test_mount().
3) _test_mount() calls _test_options() that:
  a) initializes TEST_OPTIONS=''
  b) adds rtdev,logdev options to TEST_OPTIONS for XFS; for others
filesystems it simply returns leaving TEST_OPTIONS as empty string.

That's why we need to initialize TEST_OPTIONS as MOUN_OPTIONS for CIFS
because we can't mount a remote share without specifying a username,
password, etc.

>> +     cifs)
>> +             echo $TEST_DEV | grep -q "//" > /dev/null 2>&1
>> +             if [ -z "$TEST_DEV" -o "$?" != "0" ];
>> +             then
>> +                     _notrun "this test requires a valid \$TEST_DEV"
>> +             fi
>> +             if [ ! -d "$TEST_DIR" ];
>> +             then
>> +                  _notrun "this test requires a valid \$TEST_DIR"
>> +             fi
>> +             ;;
>
> Please put the then on the same line as the if for new code.

Ok.

>
>> diff --git a/tests/generic/013 b/tests/generic/013
>> index 93d9904..ae57c67 100755
>> --- a/tests/generic/013
>> +++ b/tests/generic/013
>> @@ -35,7 +35,12 @@ _cleanup()
>>  {
>>      cd /
>>      # we might get here with a RO FS
>> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
>> +    REMOUNT_OPTIONS="remount,rw"
>> +    if [ "$FSTYP" = "cifs" ];
>> +    then
>> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
>> +    fi
>> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
>
> This looks wrong and will need an explanation.

We can't remount the existing CIFS mount without specifying username
and password. Also we need to keep others options as well since they
are user-defined (e.g. nounix, noserverino, etc) while during
remounting mount options are changed to the specified ones.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 1/2] common: add cifs support

[Dave Chinner]

On Sun, Aug 24, 2014 at 11:54:50AM +0400, Pavel Shilovsky wrote:
> 2014-08-23 15:56 GMT+04:00 Christoph Hellwig <hch@infradead.org>:
> > On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
> >> Pass -cifs argument from command line to enable cifs testing.
> >
> > Looks mostly fine, but a few nitpicks below:
....
> >>
> >> +    if [ "$FSTYP" = "cifs" ]; then
> >> +        TEST_OPTIONS="$MOUNT_OPTIONS"
> >> +        return
> >> +    fi
> >
> > What's this for?  This doesn't really make sense to me as this function adds
> > mkfs/mount options to the already normally specified ones.
> 
> 1) We included common/rc -- init_rc() is called.
> 2) init_rc() checks if $TEST_DEV is mounted and if not -- calls _test_mount().
> 3) _test_mount() calls _test_options() that:
>   a) initializes TEST_OPTIONS=''
>   b) adds rtdev,logdev options to TEST_OPTIONS for XFS; for others
> filesystems it simply returns leaving TEST_OPTIONS as empty string.
> 
> That's why we need to initialize TEST_OPTIONS as MOUN_OPTIONS for CIFS
> because we can't mount a remote share without specifying a username,
> password, etc.

That is what $TEST_FS_MOUNT_OPTS is supposed to be for. Make that
work properly when specified on the CLI or via the config file
just like MOUNT_OPTIONS does for the scratch device.

> >>      cd /
> >>      # we might get here with a RO FS
> >> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
> >> +    REMOUNT_OPTIONS="remount,rw"
> >> +    if [ "$FSTYP" = "cifs" ];
> >> +    then
> >> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
> >> +    fi
> >> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
> >
> > This looks wrong and will need an explanation.
> 
> We can't remount the existing CIFS mount without specifying username
> and password. Also we need to keep others options as well since they
> are user-defined (e.g. nounix, noserverino, etc) while during
> remounting mount options are changed to the specified ones.

filesystem configuration details like this should not pollute the
test code. Write a helper similar to _scratch_remount():

_test_remount()
{
	$UNMOUNT_PROG $TEST_DIR
	_test_mount
}

and use that in the test instead.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] common: add cifs support

[Dave Chinner]

On Sun, Aug 24, 2014 at 11:54:50AM +0400, Pavel Shilovsky wrote:
> 2014-08-23 15:56 GMT+04:00 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
> > On Sat, Aug 23, 2014 at 12:16:02PM +0400, Pavel Shilovsky wrote:
> >> Pass -cifs argument from command line to enable cifs testing.
> >
> > Looks mostly fine, but a few nitpicks below:
....
> >>
> >> +    if [ "$FSTYP" = "cifs" ]; then
> >> +        TEST_OPTIONS="$MOUNT_OPTIONS"
> >> +        return
> >> +    fi
> >
> > What's this for?  This doesn't really make sense to me as this function adds
> > mkfs/mount options to the already normally specified ones.
> 
> 1) We included common/rc -- init_rc() is called.
> 2) init_rc() checks if $TEST_DEV is mounted and if not -- calls _test_mount().
> 3) _test_mount() calls _test_options() that:
>   a) initializes TEST_OPTIONS=''
>   b) adds rtdev,logdev options to TEST_OPTIONS for XFS; for others
> filesystems it simply returns leaving TEST_OPTIONS as empty string.
> 
> That's why we need to initialize TEST_OPTIONS as MOUN_OPTIONS for CIFS
> because we can't mount a remote share without specifying a username,
> password, etc.

That is what $TEST_FS_MOUNT_OPTS is supposed to be for. Make that
work properly when specified on the CLI or via the config file
just like MOUNT_OPTIONS does for the scratch device.

> >>      cd /
> >>      # we might get here with a RO FS
> >> -    mount -o remount,rw $TEST_DEV >/dev/null 2>&1
> >> +    REMOUNT_OPTIONS="remount,rw"
> >> +    if [ "$FSTYP" = "cifs" ];
> >> +    then
> >> +        REMOUNT_OPTIONS="$REMOUNT_OPTIONS,$MOUNT_OPTIONS"
> >> +    fi
> >> +    mount -o $REMOUNT_OPTIONS $TEST_DEV >/dev/null 2>&1
> >
> > This looks wrong and will need an explanation.
> 
> We can't remount the existing CIFS mount without specifying username
> and password. Also we need to keep others options as well since they
> are user-defined (e.g. nounix, noserverino, etc) while during
> remounting mount options are changed to the specified ones.

filesystem configuration details like this should not pollute the
test code. Write a helper similar to _scratch_remount():

_test_remount()
{
	$UNMOUNT_PROG $TEST_DIR
	_test_mount
}

and use that in the test instead.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

[PATCH 2/2] common: add a directory tree for cifs tests

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 tests/cifs/Makefile | 14 ++++++++++++++
 tests/cifs/group    |  5 +++++
 2 files changed, 19 insertions(+)
 create mode 100644 tests/cifs/Makefile
 create mode 100644 tests/cifs/group

diff --git a/tests/cifs/Makefile b/tests/cifs/Makefile
new file mode 100644
index 0000000..37e868b
--- /dev/null
+++ b/tests/cifs/Makefile
@@ -0,0 +1,14 @@
+TOPDIR = ../..
+include $(TOPDIR)/include/builddefs
+
+CIFS_DIR = cifs
+TARGET_DIR = $(PKG_LIB_DIR)/$(TESTS_DIR)/$(CIFS_DIR)
+
+include $(BUILDRULES)
+
+install:
+	$(INSTALL) -m 755 -d $(TARGET_DIR)
+	$(INSTALL) -m 644 group $(TARGET_DIR)
+
+# Nothing.
+install-dev install-lib:
diff --git a/tests/cifs/group b/tests/cifs/group
new file mode 100644
index 0000000..4e01f0c
--- /dev/null
+++ b/tests/cifs/group
@@ -0,0 +1,5 @@
+# QA groups control file
+# Defines test groups and nominal group owners
+# - do not start group names with a digit
+# - comment line before each group is "new" description
+#
-- 
1.9.1

[PATCH 2/2] common: add a directory tree for cifs tests

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 tests/cifs/Makefile | 14 ++++++++++++++
 tests/cifs/group    |  5 +++++
 2 files changed, 19 insertions(+)
 create mode 100644 tests/cifs/Makefile
 create mode 100644 tests/cifs/group

diff --git a/tests/cifs/Makefile b/tests/cifs/Makefile
new file mode 100644
index 0000000..37e868b
--- /dev/null
+++ b/tests/cifs/Makefile
@@ -0,0 +1,14 @@
+TOPDIR = ../..
+include $(TOPDIR)/include/builddefs
+
+CIFS_DIR = cifs
+TARGET_DIR = $(PKG_LIB_DIR)/$(TESTS_DIR)/$(CIFS_DIR)
+
+include $(BUILDRULES)
+
+install:
+	$(INSTALL) -m 755 -d $(TARGET_DIR)
+	$(INSTALL) -m 644 group $(TARGET_DIR)
+
+# Nothing.
+install-dev install-lib:
diff --git a/tests/cifs/group b/tests/cifs/group
new file mode 100644
index 0000000..4e01f0c
--- /dev/null
+++ b/tests/cifs/group
@@ -0,0 +1,5 @@
+# QA groups control file
+# Defines test groups and nominal group owners
+# - do not start group names with a digit
+# - comment line before each group is "new" description
+#
-- 
1.9.1

Re: [PATCH 2/2] common: add a directory tree for cifs tests

[Dave Chinner]

On Sat, Aug 23, 2014 at 12:16:03PM +0400, Pavel Shilovsky wrote:
> Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
> ---
>  tests/cifs/Makefile | 14 ++++++++++++++
>  tests/cifs/group    |  5 +++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 tests/cifs/Makefile
>  create mode 100644 tests/cifs/group

Add these when you add the first CIFS specific test.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] common: add a directory tree for cifs tests

[Dave Chinner]

On Sat, Aug 23, 2014 at 12:16:03PM +0400, Pavel Shilovsky wrote:
> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  tests/cifs/Makefile | 14 ++++++++++++++
>  tests/cifs/group    |  5 +++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 tests/cifs/Makefile
>  create mode 100644 tests/cifs/group

Add these when you add the first CIFS specific test.

Cheers,

Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org

Re: [PATCH 0/2] CIFS support for XFS test suite

[Christoph Hellwig]

On Sat, Aug 23, 2014 at 12:16:01PM +0400, Pavel Shilovsky wrote:
> These are two patches that adds CIFS support to XFS tests.
> 
> The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.
> 
> With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.

That's against an smb1 server with posix extentions?  don't you need
various new _require_foo checks for things not support on normal
SMB1/2/3?

Re: [PATCH 0/2] CIFS support for XFS test suite

[Christoph Hellwig]

On Sat, Aug 23, 2014 at 12:16:01PM +0400, Pavel Shilovsky wrote:
> These are two patches that adds CIFS support to XFS tests.
> 
> The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.
> 
> With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.

That's against an smb1 server with posix extentions?  don't you need
various new _require_foo checks for things not support on normal
SMB1/2/3?

Re: [PATCH 0/2] CIFS support for XFS test suite

[Pavel Shilovsky]

2014-08-23 15:49 GMT+04:00 Christoph Hellwig <hch@infradead.org>:
> On Sat, Aug 23, 2014 at 12:16:01PM +0400, Pavel Shilovsky wrote:
>> These are two patches that adds CIFS support to XFS tests.
>>
>> The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.
>>
>> With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.
>
> That's against an smb1 server with posix extentions?  don't you need
> various new _require_foo checks for things not support on normal
> SMB1/2/3?

It was tested in posix and non-posix modes.The difference is in tests
005, 023, 024, 131 that are fail on non-posix.

We can add _require_posix() that checks if the mounted filesystem is
cifs and nounix (for example through cat /proc/mounts | grep $TEST_DEV
| grep cifs | grep nounix) and disable the above tests for this case.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 0/2] CIFS support for XFS test suite

[Pavel Shilovsky]

2014-08-23 15:49 GMT+04:00 Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>:
> On Sat, Aug 23, 2014 at 12:16:01PM +0400, Pavel Shilovsky wrote:
>> These are two patches that adds CIFS support to XFS tests.
>>
>> The first patch adds "-cifs" command line argument and CIFS specific variables. The second patch setups a directory tree for possible CIFS specific tests.
>>
>> With these patches applied, most of generic/shared $TEST_DEV tests pass for the recent kernel cifs module.
>
> That's against an smb1 server with posix extentions?  don't you need
> various new _require_foo checks for things not support on normal
> SMB1/2/3?

It was tested in posix and non-posix modes.The difference is in tests
005, 023, 024, 131 that are fail on non-posix.

We can add _require_posix() that checks if the mounted filesystem is
cifs and nounix (for example through cat /proc/mounts | grep $TEST_DEV
| grep cifs | grep nounix) and disable the above tests for this case.

-- 
Best regards,
Pavel Shilovsky.