On Sun, Aug 24, 2014 at 11:54:50AM +0400, Pavel Shilovsky wrote:
> 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:
....
> >>
> >> + 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@xxxxxxxxxxxxx
--
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
