| Message ID | 1409084918-17764-4-git-send-email-pshilovsky@samba.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 27, 2014 at 12:28:38AM +0400, Pavel Shilovsky wrote: > CIFS/SMB protocol without POSIX extensions doesn't support operations > with symbolic links and advisory byte-range locks from the same process. > Add a check for nounix mounts and use it in generic tests that > require such operations. +_require_test_posix_ext seems very cifs specific. Can you take a look at the tests and see what posix feature they require and add features based on that? Let's have a quick discussion here on the requirements of the tests before even writing the code. > diff --git a/tests/generic/005 b/tests/generic/005 > index d78e43f..0c2b51f 100755 > --- a/tests/generic/005 > +++ b/tests/generic/005 > @@ -67,6 +67,7 @@ _touch() > # real QA test starts here > _supported_fs generic > _require_test > +_require_test_posix_ext > > # IRIX UDF does not support symlinks > if [ $FSTYP == 'udf' ]; then this suggest 005 needs symlinks and plain cifs doesn't support them. We should also fold this test for IRIX udf into the _requires_symlink tests. > diff --git a/tests/generic/023 b/tests/generic/023 > index 114485c..91b8a37 100755 > --- a/tests/generic/023 > +++ b/tests/generic/023 > @@ -45,6 +45,7 @@ _supported_os Linux > > _require_test > _requires_renameat2 > +_require_test_posix_ext 023-025 just require a working renameat2, and nothing in Posix. What's the problem for cifs here? > diff --git a/tests/generic/131 b/tests/generic/131 > index b4e3ff0..9736963 100755 > --- a/tests/generic/131 > +++ b/tests/generic/131 > @@ -45,6 +45,7 @@ _cleanup() > _supported_fs generic > _supported_os Linux > _require_test > +_require_test_posix_ext > > TESTFILE=$TEST_DIR/lock_file 131 tests fcntl style file locking, so we should test for that. -- 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
On Wed, Aug 27, 2014 at 10:58 AM, Christoph Hellwig <hch@infradead.org> wrote: > On Wed, Aug 27, 2014 at 12:28:38AM +0400, Pavel Shilovsky wrote: >> CIFS/SMB protocol without POSIX extensions doesn't support operations >> with symbolic links and advisory byte-range locks from the same process. >> Add a check for nounix mounts and use it in generic tests that >> require such operations. > > +_require_test_posix_ext seems very cifs specific. Can you take > a look at the tests and see what posix feature they require and > add features based on that? Let's have a quick discussion here on the > requirements of the tests before even writing the code. Do you know a more standardized way to test if symlink support is available? We don't really have the same thing as the FS and Device capability ioctls that Microsoft offers, but even for them, they don't export whether symlink support is enabled as a volume or export property. Presumably we could try to create one and look for the rc EOPNOTSUPP), but it seems simple enough for now to check on mount looking for fs-specific features as Pavel has done, or to always enable the symlink tests for cifs (and smb3 after we have added it). > > this suggest 005 needs symlinks and plain cifs doesn't support them. > We should also fold this test for IRIX udf into the _requires_symlink > tests. Plain cifs does support emulated symlinks two ways but they aren't enabled by default in mount. At least one of these also needs to be added to SMB3, and when I looked at this in the past it looked fairly easy. When support for "mfsymlinks" (apple-style emulated symlinks) or for creating "NFS-reparse point symlinks" (Windows style NFS server symlinks) is added for SMB3 we could change the way the cifs check is done. In the long run I would prefer that we run the cifs and smb3 xfstest runs with symlink dependent tests enabled. >> diff --git a/tests/generic/023 b/tests/generic/023 >> index 114485c..91b8a37 100755 >> --- a/tests/generic/023 >> +++ b/tests/generic/023 >> @@ -45,6 +45,7 @@ _supported_os Linux >> >> _require_test >> _requires_renameat2 >> +_require_test_posix_ext > > > 023-025 just require a working renameat2, and nothing in Posix. What's > the problem for cifs here?
On Wed, Aug 27, 2014 at 11:17:25AM -0500, Steve French wrote: > Do you know a more standardized way to test if symlink support is available? > We don't really have the same thing as the FS and Device capability ioctls > that Microsoft offers, but even for them, they don't export whether symlink > support is enabled as a volume or export property. > Presumably we could try to create one and look for the rc EOPNOTSUPP), but > it seems simple enough for now to check on mount looking for > fs-specific features > as Pavel has done, or to always enable the symlink tests for cifs (and > smb3 after > we have added it). There's not good way to check for it except for trying. So a simple routine that creates a symlink on $TEST_DIR and checks if that works would be the way to go. -- 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
> 27 ???. 2014 ?., ? 19:58, Christoph Hellwig <hch@infradead.org> ???????(?): > >> On Wed, Aug 27, 2014 at 12:28:38AM +0400, Pavel Shilovsky wrote: >> CIFS/SMB protocol without POSIX extensions doesn't support operations >> with symbolic links and advisory byte-range locks from the same process. >> Add a check for nounix mounts and use it in generic tests that >> require such operations. > > +_require_test_posix_ext seems very cifs specific. Can you take > a look at the tests and see what posix feature they require and > add features based on that? Let's have a quick discussion here on the > requirements of the tests before even writing the code. > Agree. >> diff --git a/tests/generic/005 b/tests/generic/005 >> index d78e43f..0c2b51f 100755 >> --- a/tests/generic/005 >> +++ b/tests/generic/005 >> @@ -67,6 +67,7 @@ _touch() >> # real QA test starts here >> _supported_fs generic >> _require_test >> +_require_test_posix_ext >> >> # IRIX UDF does not support symlinks >> if [ $FSTYP == 'udf' ]; then > > this suggest 005 needs symlinks and plain cifs doesn't support them. > We should also fold this test for IRIX udf into the _requires_symlink > tests. > >> diff --git a/tests/generic/023 b/tests/generic/023 >> index 114485c..91b8a37 100755 >> --- a/tests/generic/023 >> +++ b/tests/generic/023 >> @@ -45,6 +45,7 @@ _supported_os Linux >> >> _require_test >> _requires_renameat2 >> +_require_test_posix_ext > > > 023-025 just require a working renameat2, and nothing in Posix. What's > the problem for cifs here? These tests try to create symlinks and then rename them. > >> diff --git a/tests/generic/131 b/tests/generic/131 >> index b4e3ff0..9736963 100755 >> --- a/tests/generic/131 >> +++ b/tests/generic/131 >> @@ -45,6 +45,7 @@ _cleanup() >> _supported_fs generic >> _supported_os Linux >> _require_test >> +_require_test_posix_ext >> >> TESTFILE=$TEST_DIR/lock_file > > 131 tests fcntl style file locking, so we should test for that. > > -- > 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 So for these tests we need two check functions: _require_symlink for 005, 023, 024, 025 and _require_fcntl for 131. Right? -- Best regards, Pavel Shilovsky.-- 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
On Wed, Aug 27, 2014 at 11:49:56PM +0400, Pavel Shilovsky wrote: > So for these tests we need two check functions: _require_symlink for 005, > 023, 024, 025 and _require_fcntl for 131. Right? More or less. I'd call the second one _require_fcntl_locks as fcntl is a multiplexer for tons of unrelated functionality. -- 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-28 2:50 GMT+04:00 Christoph Hellwig <hch@infradead.org>: > On Wed, Aug 27, 2014 at 11:49:56PM +0400, Pavel Shilovsky wrote: >> So for these tests we need two check functions: _require_symlink for 005, >> 023, 024, 025 and _require_fcntl for 131. Right? > > More or less. I'd call the second one _require_fcntl_locks as fcntl > is a multiplexer for tons of unrelated functionality. Ok, let's name it this way. What's about the 1st and 2nd patches? I am going to repost the series with a new version of the 3rd patch (according to our discussion) and it'll be better to incorporate their possible changes as well.
diff --git a/common/rc b/common/rc index 8b427fc..191f7ff 100644 --- a/common/rc +++ b/common/rc @@ -2374,6 +2374,13 @@ _require_btrfs_fs_feature() _notrun "Feature $feat not supported by the available btrfs version" } +_require_test_posix_ext() +{ + [ "$FSTYP" != "cifs" ] && return 0 + cat /proc/mounts | grep $TEST_DEV | grep cifs | grep -q nounix && \ + _notrun "Require POSIX extensions enabled" +} + _get_total_inode() { if [ -z "$1" ]; then diff --git a/tests/generic/005 b/tests/generic/005 index d78e43f..0c2b51f 100755 --- a/tests/generic/005 +++ b/tests/generic/005 @@ -67,6 +67,7 @@ _touch() # real QA test starts here _supported_fs generic _require_test +_require_test_posix_ext # IRIX UDF does not support symlinks if [ $FSTYP == 'udf' ]; then diff --git a/tests/generic/023 b/tests/generic/023 index 114485c..91b8a37 100755 --- a/tests/generic/023 +++ b/tests/generic/023 @@ -45,6 +45,7 @@ _supported_os Linux _require_test _requires_renameat2 +_require_test_posix_ext # real QA test starts here diff --git a/tests/generic/024 b/tests/generic/024 index 8945191..1248e78 100755 --- a/tests/generic/024 +++ b/tests/generic/024 @@ -45,6 +45,7 @@ _supported_os Linux _require_test _requires_renameat2 +_require_test_posix_ext rename_dir=$TEST_DIR/$$ mkdir $rename_dir diff --git a/tests/generic/025 b/tests/generic/025 index 6b6c8ab..d06136c 100755 --- a/tests/generic/025 +++ b/tests/generic/025 @@ -45,6 +45,7 @@ _supported_os Linux _require_test _requires_renameat2 +_require_test_posix_ext rename_dir=$TEST_DIR/$$ mkdir $rename_dir diff --git a/tests/generic/131 b/tests/generic/131 index b4e3ff0..9736963 100755 --- a/tests/generic/131 +++ b/tests/generic/131 @@ -45,6 +45,7 @@ _cleanup() _supported_fs generic _supported_os Linux _require_test +_require_test_posix_ext TESTFILE=$TEST_DIR/lock_file