On Mon, Jun 17, 2019 at 5:30 PM Pavel Bar <pbar@redhat.com> wrote:



On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof@gmail.com> wrote:
Sanlock allow up to 1023 characters in a disk path. We used to truncate
invalid long path silently instead of failing. Add failing tests for the
expected behaviour.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/constants.py   |  4 +++
 tests/python_test.py | 79 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/tests/constants.py b/tests/constants.py
index 4f45d13..2371fed 100644
--- a/tests/constants.py
+++ b/tests/constants.py
@@ -21,5 +21,9 @@ RINDEX_DISK_MAGIC = 0x01042018
 # src/rindex_disk.h
 # Copied from the docs module comment.

 RINDEX_ENTRY_SIZE = 64
 RINDEX_ENTRIES_SECTORS = 2000
+
+# src/sanlock.h
+
+SANLK_PATH_LEN = 1024
diff --git a/tests/python_test.py b/tests/python_test.py
index cd8a82f..5624f60 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -550,10 +550,21 @@ def test_write_resource_parse_args(no_sanlock_daemon, name, filename, encoding):

     with raises_sanlock_errno():
         sanlock.write_resource(b"ls_name", name, disks)


+@pytest.mark.xfail(reason="path truncated silently")
+def test_write_resource_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN

What about moving the "path" initialization line 2 lines up?
Here and in many more places where "path" local variable is initialized and then passed to a method call.
I know that from the Java best practices perspective it's better to wrap only code that can throw an exception with the exception handling block.

It is indeed better to move path out of the pytest.raises() block. However it is 
unlikely that we will fail with ValueError in that line.

 
+        sanlock.write_resource(b"ls_name", b"res_name", [(path, 0)])
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.write_resource(b"ls_name", b"res_name", [(path, 0)])
+
+
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_release_resource_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     disks = [(path, 0)]
@@ -562,10 +573,21 @@ def test_release_resource_parse_args(no_sanlock_daemon, name, filename, encoding

     with raises_sanlock_errno():
         sanlock.release(b"ls_name", name, disks)


+@pytest.mark.xfail(reason="path truncated silently")
+def test_release_resource_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.release(b"ls_name", b"res_name", [(path, 0)])
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.release(b"ls_name", b"res_name", [(path, 0)])
+
+
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_read_resource_owners_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     disks = [(path, 0)]
@@ -574,10 +596,21 @@ def test_read_resource_owners_parse_args(no_sanlock_daemon, name, filename, enco

     with raises_sanlock_errno():
         sanlock.read_resource_owners(b"ls_name", name, disks)


+@pytest.mark.xfail(reason="path truncated silently")
+def test_read_resource_owners_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.read_resource_owners(b"ls_name", b"res_name", [(path, 0)])
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.read_resource_owners(b"ls_name", b"res_name", [(path, 0)])
+
+
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 def test_get_hosts_parse_args(no_sanlock_daemon, name):
     with raises_sanlock_errno():
         sanlock.get_hosts(name, 1)

@@ -624,10 +657,23 @@ def test_init_resource_parse_args(no_sanlock_daemon, name, filename, encoding):
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.init_resource(b"ls_name", name, disks)
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.init_resource(name, b"res_name", disks)

+
+@pytest.mark.xfail(reason="path truncated silently")
+def test_init_resource_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.init_resource(b"ls_name", b"res_name", [(path, 0)])
+
+    # init_resource access storage directly.
+    with raises_sanlock_errno(errno.ENAMETOOLONG):
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.init_resource(b"ls_name", b"res_name", [(path, 0)])
+
+
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_get_alignment_parse_args(no_sanlock_daemon, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.get_alignment(path)
@@ -643,10 +689,21 @@ def test_read_resource_parse_args(no_sanlock_daemon, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
         sanlock.read_resource(path)


+@pytest.mark.xfail(reason="path truncated silently")
+def test_read_resource_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.read_resource(path)
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.read_resource(path)
+
+
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_request_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     disks = [(path, 0)]
@@ -656,10 +713,21 @@ def test_request_parse_args(no_sanlock_daemon, name, filename, encoding):

     with raises_sanlock_errno():
         sanlock.request(name, b"res_name", disks)


+@pytest.mark.xfail(reason="path truncated silently")
+def test_request_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.request(b"ls_name", b"res_name", [(path, 0)])
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.request(b"ls_name", b"res_name", [(path, 0)])
+
+
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_acquire_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     disks = [(path, 0)]
@@ -667,5 +735,16 @@ def test_acquire_parse_args(no_sanlock_daemon, name, filename, encoding):
     with raises_sanlock_errno():
         sanlock.acquire(b"ls_name", name, disks, pid=os.getpid())

     with raises_sanlock_errno():
         sanlock.acquire(name, b"res_name", disks, pid=os.getpid())
+
+
+@pytest.mark.xfail(reason="path truncated silently")
+def test_acquire_path_length(no_sanlock_daemon):
+    with pytest.raises(ValueError):
+        path = "x" * constants.SANLK_PATH_LEN
+        sanlock.acquire(b"ls_name", b"res_name", [(path, 0)], pid=os.getpid())
+
+    with raises_sanlock_errno():
+        path = "x" * (constants.SANLK_PATH_LEN - 1)
+        sanlock.acquire(b"ls_name", b"res_name", [(path, 0)], pid=os.getpid())
--
2.17.2