Re: [PATCH v2 1/2] tests: replace reading of magic number by helper function

[Nir Soffer]

On Sun, Jun 16, 2019 at 4:36 PM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  On Sat, Jun 15, 2019 at 1:02 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
>
> > On Sat, Jun 15, 2019 at 12:55 AM Vojtech Juranek <vjuranek(a)redhat.com&gt;
> > wrote:
> >
> >> Introduce helper function for reading sanlock magic numbers. This
> >> helper function seeks to specified offset in given file and reads
> >> unsigned int from this position.
> >> ---
> >>  tests/python_test.py | 25 +++++++++----------------
> >>  tests/util.py        | 11 +++++++++++
> >>  2 files changed, 20 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/tests/python_test.py b/tests/python_test.py
> >> index 3242f57..623a13c 100644
> >> --- a/tests/python_test.py
> >> +++ b/tests/python_test.py
> >> @@ -86,12 +86,9 @@ def test_write_lockspace(tmpdir, sanlock_daemon,
> >> filename, encoding, size, offse
> >>          b"ls_name", 1, path, offset=offset, wait=False)
> >>      assert acquired is False
> >>
> >> -    with io.open(path, "rb") as f:
> >> -        f.seek(offset)
> >> -        magic, = struct.unpack("< I", f.read(4))
> >> -        assert magic == constants.DELTA_DISK_MAGIC
> >> -
> >> -        # TODO: check more stuff here...
> >> +    magic = util.read_uint(path, offset)
> >> +    assert magic == constants.DELTA_DISK_MAGIC
> >> +    # TODO: check more stuff here...
> >>
> >>      util.check_guard(path, size)
> >>
> >> @@ -117,9 +114,8 @@ def test_write_lockspace_4k(user_4k_path,
> >> sanlock_daemon, align):
> >>      assert acquired is False
> >>
> >>      # Verify that lockspace was written.
> >> -    with io.open(user_4k_path, "rb") as f:
> >> -        magic, = struct.unpack("< I", f.read(4))
> >> -        assert magic == constants.DELTA_DISK_MAGIC
> >> +    magic = util.read_uint(user_4k_path)
> >> +    assert magic == constants.DELTA_DISK_MAGIC
> >>
> >>      # Check that sanlock did not write beyond the lockspace area.
> >>      util.check_guard(user_4k_path, align)
> >> @@ -179,10 +175,8 @@ def test_write_resource(tmpdir, sanlock_daemon,
> >> filename, encoding, size, offset
> >>      owners = sanlock.read_resource_owners(b"ls_name",
> b"res_name",
> >> disks)
> >>      assert owners == []
> >>
> >> -    with io.open(path, "rb") as f:
> >> -        f.seek(offset)
> >> -        magic, = struct.unpack("< I", f.read(4))
> >> -        assert magic == constants.PAXOS_DISK_MAGIC
> >> +    magic = util.read_uint(path, offset)
> >> +    assert magic == constants.PAXOS_DISK_MAGIC
> >>
> >>          # TODO: check more stuff here...
> >>
> >> @@ -216,9 +210,8 @@ def test_write_resource_4k(sanlock_daemon,
> >> user_4k_path, align):
> >>      assert owners == []
> >>
> >>      # Verify that resource was written.
> >> -    with io.open(user_4k_path, "rb") as f:
> >> -        magic, = struct.unpack("< I", f.read(4))
> >> -        assert magic == constants.PAXOS_DISK_MAGIC
> >> +    magic = util.read_uint(user_4k_path)
> >> +    assert magic == constants.PAXOS_DISK_MAGIC
> >>
> >>      # Check that sanlock did not write beyond the lockspace area.
> >>      util.check_guard(user_4k_path, align)
> >> diff --git a/tests/util.py b/tests/util.py
> >> index 08db236..32b34ec 100644
> >> --- a/tests/util.py
> >> +++ b/tests/util.py
> >> @@ -177,3 +177,14 @@ def sanlock_is_running():
> >>          if e.errno != errno.ENOENT:
> >>              raise
> >>      return False
> >> +
> >> +
> >> +def read_uint(path, offset=0):
> >> +    """
> >> +    Read and return unsigned int from specified file at given offset.
> >> +    Typical usage is to read sanlock magic number.
> >> +    """
> >> +    with io.open(path, "rb") as f:
> >> +        f.seek(offset)
> >> +        uint, = struct.unpack("< I", f.read(4))
> >> +        return uint
> >> --
> >> 2.20.1
> >>
> >>
> > Looks good, but maybe call it read_magic(path, offset)?
> >
>
>  Looks good.
>  Regarding the Nir's comment, suggest "read_magic_int".
>  
read_magic_init does not not add anything useful. We will  never have
read_magic_float
so there is no reason to include the type in the name.