Discussion:
[PATCH 0/2] CIFS support for XFS test suite
Pavel Shilovsky
2014-08-23 08:16:01 UTC
Permalink
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
Pavel Shilovsky
2014-08-23 08:16:02 UTC
Permalink
Pass -cifs argument from command line to enable cifs testing.

Reviewed-by: David Disseldorp <ddiss-***@public.gmane.org>
Signed-off-by: Pavel Shilovsky <pshilovsky-***@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
Christoph Hellwig
2014-08-23 11:56:06 UTC
Permalink
Post by Pavel Shilovsky
Pass -cifs argument from command line to enable cifs testing.
_mount_opts()
{
+
case $FSTYP in
Remove this spurious new empty line, please.
Post by Pavel Shilovsky
- 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"
Post by Pavel Shilovsky
- 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.
Post by Pavel Shilovsky
# 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.
Post by Pavel Shilovsky
+ 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.
Post by Pavel Shilovsky
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.
Pavel Shilovsky
2014-08-24 07:54:50 UTC
Permalink
Post by Christoph Hellwig
Post by Pavel Shilovsky
Pass -cifs argument from command line to enable cifs testing.
Thank you for reviewing this.
Post by Christoph Hellwig
Post by Pavel Shilovsky
_mount_opts()
{
+
case $FSTYP in
Remove this spurious new empty line, please.
This was added by mistake - will remove.
Post by Christoph Hellwig
Post by Pavel Shilovsky
- 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.
Post by Christoph Hellwig
Post by Pavel Shilovsky
- 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.
Post by Pavel Shilovsky
# 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.
Post by Christoph Hellwig
Post by Pavel Shilovsky
+ 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.
Post by Christoph Hellwig
Post by Pavel Shilovsky
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.
Dave Chinner
2014-08-25 00:56:18 UTC
Permalink
Post by Pavel Shilovsky
Post by Christoph Hellwig
Post by Pavel Shilovsky
Pass -cifs argument from command line to enable cifs testing.
....
Post by Pavel Shilovsky
Post by Christoph Hellwig
Post by Pavel Shilovsky
+ 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().
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.
Post by Pavel Shilovsky
Post by Christoph Hellwig
Post by Pavel Shilovsky
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-***@public.gmane.org
Pavel Shilovsky
2014-08-23 08:16:03 UTC
Permalink
Signed-off-by: Pavel Shilovsky <pshilovsky-***@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
Dave Chinner
2014-08-25 00:56:59 UTC
Permalink
Post by Pavel Shilovsky
---
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-***@public.gmane.org
Christoph Hellwig
2014-08-23 11:49:50 UTC
Permalink
Post by 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.
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?
Pavel Shilovsky
2014-08-24 10:41:27 UTC
Permalink
Post by Christoph Hellwig
Post by 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.
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.
Loading...