diff mbox

[v2,3/3] cifs: skip tests that need POSIX support for nounix mounts

Message ID 1409084918-17764-4-git-send-email-pshilovsky@samba.org (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Shilovsky Aug. 26, 2014, 8:28 p.m. UTC 
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.

Reviewed-by: Steve French <smfrench@gmail.com>
Signed-off-by: Pavel Shilovsky <pshilovsky@samba.org>
---
 common/rc         | 7 +++++++
 tests/generic/005 | 1 +
 tests/generic/023 | 1 +
 tests/generic/024 | 1 +
 tests/generic/025 | 1 +
 tests/generic/131 | 1 +
 6 files changed, 12 insertions(+)

Comments

Christoph Hellwig Aug. 27, 2014, 3:58 p.m. UTC | #1 
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
Steve French Aug. 27, 2014, 4:17 p.m. UTC | #2 
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?
Christoph Hellwig Aug. 27, 2014, 5:13 p.m. UTC | #3 
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
Pavel Shilovsky Aug. 27, 2014, 7:49 p.m. UTC | #4 
> 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
Christoph Hellwig Aug. 27, 2014, 10:50 p.m. UTC | #5 
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
Pavel Shilovsky Aug. 28, 2014, 7:25 a.m. UTC | #6 
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 mbox

Patch

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