diff mbox

[1/2] common: add cifs support

Message ID 1408781763-30127-2-git-send-email-pshilovsky@samba.org (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky Aug. 23, 2014, 8:16 a.m. UTC 
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(-)

Comments

Christoph Hellwig Aug. 23, 2014, 11:56 a.m. UTC | #1 
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Shilovsky Aug. 24, 2014, 7:54 a.m. UTC | #2 
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.
Dave Chinner Aug. 25, 2014, 12:56 a.m. UTC | #3 
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.
diff mbox

Patch

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