2014-08-23 15:56 GMT+04:00 Christoph Hellwig <hch@xxxxxxxxxxxxx>:
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
