Re: [PATCH v2 7/8] python: Add failing tests for long disk path

[Nir Soffer]

On Mon, Jun 17, 2019 at 5:30 PM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com&gt; 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&gt;
> > ---
> >  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=&quot;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=&quot;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=&quot;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=&quot;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=&quot;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=&quot;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=&quot;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
> >
> >

[PATCH v2 7/8] python: Add failing tests for long disk path

[Nir Soffer]

On Mon, Jun 17, 2019 at 6:32 PM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:

>  On Mon, Jun 17, 2019 at 5:30 PM Pavel Bar <pbar(a)redhat.com&gt;
> wrote:
>
> >
> >
> >
> > On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com&gt; 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&gt;
> >> ---
> >>  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=&quot;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.
>  
However I fixed this before I pushed, thanks!


> >
> >> +        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=&quot;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=&quot;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=&quot;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=&quot;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=&quot;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=&quot;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
> >>
> >>