| Message ID | 1408781763-30127-2-git-send-email-pshilovsky@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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 --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