On Mon, Jun 17, 2019 at 5:30 PM Pavel Bar <pbar(a)redhat.com> wrote:
On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)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(a)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)
>
>
> +(a)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)
>
>
> +(a)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)
>
>
> +(a)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)
>
> +
> +(a)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)
>
>
> +(a)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)
>
>
> +(a)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())
> +
> +
> +(a)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
>
>