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  wrote:
> On Sat, Jun 15, 2019 at 1:02 AM Nir Soffer  wrote:
> > On Sat, Jun 15, 2019 at 12:55 AM Vojtech Juranek 
> > 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.