[PATCH 0/3] lvb improvements

[Benny Zlotnik]

The following patchset addresses comment in[1]

- Improve documentation for get/set_lvb
- Add size support to get_lvb
- Add and mprove lvb tests


[1]
https://lists.fedorahosted.org/archives/list/[e-mail]...
Github branch: https://github.com/bennyz/sanlock/tree/lvb-fixes

Benny Zlotnik (3):
  python: improve get/set_lvb documentation
  python: support size in get_lvb
  python: improve lvb tests

 python/sanlock.c     | 23 ++++++----
 tests/python_test.py | 99 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 103 insertions(+), 19 deletions(-)

-- 
2.28.0

[PATCH 1/3] python: improve get/set_lvb documentation

[Benny Zlotnik]

- Specify lvb=True explicitly instead of mentioning the sanlock flag
- Document when data is written and visible

Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
---
 python/sanlock.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index fbcabb1..567d86c 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1607,7 +1607,9 @@ Arguments\n\
 \n\
 Notes\n\
 \n\
-The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+The resource must be acquired with lvb=true.\n\
+The size of data is limited by the sector size (512/4K).\n\
+The new data is visible after the resource is released\n");
 static PyObject *
 py_set_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
@@ -1662,7 +1664,7 @@ Returns\n\
   data              data written with set_lvb\n\
 Notes\n\
 \n\
-The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+The resource must be acquired with lvb=True\n");
 static PyObject *
 py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-- 
2.28.0

[PATCH 2/3] python: support size in get_lvb

[Benny Zlotnik]

Add a size parameter to allow the user choose how much data to get back,
the API will now look like this:

    sanlock.get_lvb(lockspace, resource, disks, size)

Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
---
 python/sanlock.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index 567d86c..41bbb43 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1659,26 +1659,28 @@ Arguments\n\
   lockspace         lockspace name (str)\n\
   resource          resource name (int)\n\
   disks             path and offset (tuple)\n\
+  size              amount of data to read (int)\n\
 \n\
 Returns\n\
   data              data written with set_lvb\n\
 Notes\n\
 \n\
-The resource must be acquired with lvb=True\n");
+The resource must be acquired with lvb=True\n\
+size is required and has to be > 0.\n");
 static PyObject *
 py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    uint32_t flags = 0;
+    uint32_t flags = 0, lvb_size = 0;
     int rv = -1;
     struct sanlk_resource *res = NULL;
     PyObject *lockspace = NULL, *resource = NULL;
     PyObject *disks;
-    char data[512];
+    char *lvb_data = NULL;

-    static char *kwlist[] = {"lockspace", "resource",
"disks", NULL};
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!", kwlist,
+    static char *kwlist[] = {"lockspace", "resource",
"disks", "size", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!K", kwlist,
         convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
-        &PyList_Type, &disks)) {
+        &PyList_Type, &disks, &lvb_size)) {
         goto finally;
     }

@@ -1688,9 +1690,10 @@ py_get_lvb(PyObject *self __unused, PyObject *args, PyObject
*keywds)

     strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
     strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
+    lvb_data = calloc(lvb_size, sizeof(char));

     Py_BEGIN_ALLOW_THREADS
-    rv = sanlock_get_lvb(flags, res, data, sizeof(data));
+    rv = sanlock_get_lvb(flags, res, lvb_data, lvb_size);
     Py_END_ALLOW_THREADS

     if (rv < 0) {
@@ -1705,7 +1708,7 @@ finally:
     if (rv < 0)
         return NULL;

-    return Py_BuildValue("y", data);
+    return Py_BuildValue("y#", lvb_data, lvb_size);
 }

 static PyMethodDef
--
2.28.0

[PATCH 3/3] python: improve lvb tests

[Benny Zlotnik]

- Remove null byte test, it is now tested in the basic test
- Add 4k tests for E2BIG failure
- Add test for writing entire 4k sector
- Add test for reading less than sector size

Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
---
 tests/python_test.py | 99 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 89 insertions(+), 10 deletions(-)

diff --git a/tests/python_test.py b/tests/python_test.py
index 28fd27f..b2e0198 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -780,17 +780,21 @@ def test_lvb(tmpdir, sanlock_daemon):
     sanlock.write_resource(b"ls_name", b"res_name", disks)

     fd = sanlock.register()
+    lvb_data = b"first\0second".ljust(512, b"\0")

     sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
-    sanlock.set_lvb(b"ls_name", b"res_name", disks,
b"{gen:0}")
+    sanlock.set_lvb(b"ls_name", b"res_name", disks, lvb_data)
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+
+    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
+    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
size=512)

-    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
     sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

-    assert result == b"{gen:0}"
+    assert result == lvb_data


-def test_lvb_value_too_long(tmpdir, sanlock_daemon):
+def test_lvb_value_too_long_512(tmpdir, sanlock_daemon):
     ls_path = str(tmpdir.join("ls_name"))
     util.create_file(ls_path, MiB)

@@ -805,15 +809,86 @@ def test_lvb_value_too_long(tmpdir, sanlock_daemon):

     fd = sanlock.register()

-    long_val = b"a" * 513
+    long_val = b"a" * (SECTOR_SIZE_512 + 1)
+    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
+    with raises_sanlock_errno(errno.E2BIG):
+        sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)
+
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+
+
+(a)pytest.mark.parametrize(&quot;align&quot;, sanlock.ALIGN_SIZE)
+def test_lvb_value_too_long_4k(user_4k_path, sanlock_daemon, align):
+
+    disks = [(user_4k_path, 0)]
+
+    # Poison resource area, ensuring that previous tests will not break this
+    # test, and sanlock does not write beyond the lockspace area.
+    with io.open(user_4k_path, "rb+") as f:
+        f.write(align * b"x")
+    util.write_guard(user_4k_path, align)
+    sanlock.write_lockspace(
+        b"ls_name",
+        user_4k_path,
+        iotimeout=1,
+        align=align,
+        sector=SECTOR_SIZE_4K)
+
+    sanlock.add_lockspace(b"ls_name", 1, user_4k_path, offset=0, iotimeout=1)
+
+    sanlock.write_resource(
+        b"ls_name", b"res_name", disks, align=align,
sector=SECTOR_SIZE_4K)
+
+    fd = sanlock.register()
+
+    long_val = b"a" * (SECTOR_SIZE_4K + 1)
     sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
     with raises_sanlock_errno(errno.E2BIG):
         sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)

     sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

+    util.check_guard(user_4k_path, align)
+

-def test_lvb_null_bytes(tmpdir, sanlock_daemon):
+(a)pytest.mark.parametrize(&quot;align&quot;, sanlock.ALIGN_SIZE)
+def test_lvb_use_entire_sector_4k(user_4k_path, sanlock_daemon, align):
+    disks = [(user_4k_path, 0)]
+
+    # Poison resource area, ensuring that previous tests will not break this
+    # test, and sanlock does not write beyond the lockspace area.
+    with io.open(user_4k_path, "rb+") as f:
+        f.write(align * b"x")
+    util.write_guard(user_4k_path, align)
+    sanlock.write_lockspace(
+        b"ls_name",
+        user_4k_path,
+        iotimeout=1,
+        align=align,
+        sector=SECTOR_SIZE_4K)
+
+    sanlock.add_lockspace(b"ls_name", 1, user_4k_path, offset=0, iotimeout=1)
+
+    sanlock.write_resource(
+        b"ls_name", b"res_name", disks, align=align,
sector=SECTOR_SIZE_4K)
+
+    fd = sanlock.register()
+
+    long_val = b"a" * SECTOR_SIZE_4K
+    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
+    sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+
+    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
+    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
+                             size=SECTOR_SIZE_4K)
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+    assert result == long_val
+
+    util.check_guard(user_4k_path, align)
+
+
+def test_lvb_read_less_than_cluster_size(tmpdir, sanlock_daemon):
     ls_path = str(tmpdir.join("ls_name"))
     util.create_file(ls_path, MiB)

@@ -827,12 +902,16 @@ def test_lvb_null_bytes(tmpdir, sanlock_daemon):
     sanlock.write_resource(b"ls_name", b"res_name", disks)

     fd = sanlock.register()
+    lvb_data = b"first\0second"
+
+    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
+    sanlock.set_lvb(b"ls_name", b"res_name", disks,
lvb_data.ljust(512, b"\0"))
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

     sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
lvb=True)
-    sanlock.set_lvb(b"ls_name", b"res_name", disks,
b"{ge\x00:0}")
+    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
+                             size=len(lvb_data))

-    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
     sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

-    # Check that the string we passed is terminated by the null-byte
-    assert result == b"{ge"
+    assert result == lvb_data
--
2.28.0

[PATCH 1/3] python: improve get/set_lvb documentation

[Nir Soffer]

On Wed, Dec 9, 2020 at 7:43 PM Benny Zlotnik <bzlotnik(a)redhat.com&gt; wrote:
>  - Specify lvb=True explicitly instead of mentioning the sanlock flag
>  - Document when data is written and visible
>
>  Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  ---
>   python/sanlock.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index fbcabb1..567d86c 100644
>  --- a/python/sanlock.c
>  +++ b/python/sanlock.c
>  @@ -1607,7 +1607,9 @@ Arguments\n\
>   \n\
>   Notes\n\
>   \n\
>  -The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
>  +The resource must be acquired with lvb=true.\n\
>  +The size of data is limited by the sector size (512/4K).\n\
>  +The new data is visible after the resource is released\n");
>   static PyObject *
>   py_set_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>   {
>  @@ -1662,7 +1664,7 @@ Returns\n\
>     data              data written with set_lvb\n\ 
Missing \n

>   Notes\n\
>   \n\
>  -The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
>  +The resource must be acquired with lvb=True\n"); 
In a later patch you added

    size is required and has to be > 0.

Which should be part of this patch. But we should also mention that the size
must be equal or less then the resource sector size.

>   static PyObject *
>   py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>   {
>  --
>  2.28.0
>

[PATCH 1/3] python: improve get/set_lvb documentation

[Nir Soffer]

On Wed, Dec 9, 2020 at 10:16 PM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
>  On Wed, Dec 9, 2020 at 7:43 PM Benny Zlotnik <bzlotnik(a)redhat.com&gt; wrote:
>  >
>  > - Specify lvb=True explicitly instead of mentioning the sanlock flag
>  > - Document when data is written and visible
>  >
>  > Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  > ---
>  >  python/sanlock.c | 6 ++++--
>  >  1 file changed, 4 insertions(+), 2 deletions(-)
>  >
>  > diff --git a/python/sanlock.c b/python/sanlock.c
>  > index fbcabb1..567d86c 100644
>  > --- a/python/sanlock.c
>  > +++ b/python/sanlock.c
>  > @@ -1607,7 +1607,9 @@ Arguments\n\
>  >  \n\
>  >  Notes\n\
>  >  \n\
>  > -The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
>  > +The resource must be acquired with lvb=true.\n\
>  > +The size of data is limited by the sector size (512/4K).\n\
>  > +The new data is visible after the resource is released\n");
>  >  static PyObject *
>  >  py_set_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>  >  {
>  > @@ -1662,7 +1664,7 @@ Returns\n\
>  >    data              data written with set_lvb\n\
>
>  Missing \n 
You can check that text using:

$ PYTHONPATH=build/lib.linux-x86_64-3.8 python -c 'import
sanlock;help(sanlock.get_lvb)'
Help on built-in function get_lvb in module sanlock:

get_lvb(...)
    get_lvb(lockspace, resource, disks) -> bytes
    Read Lock Value Block for a given resource

    Arguments
      lockspace         lockspace name (str)
      resource          resource name (int)
      disks             path and offset (tuple)
      size              Amount of data to read (int)

    Returns
      data              data written with set_lvb
    Notes

    The resource must be acquired with lvb=True
    size is required and has to be > 0.

>  >  Notes\n\
>  >  \n\ 
Newline not needed here

>  > -The resource must be acquired with the SANLK_ACQUIRE_LVB
> flag\n");
>  > +The resource must be acquired with lvb=True\n");
>
>  In a later patch you added
>
>      size is required and has to be > 0.
>
>  Which should be part of this patch. 
size is added only in the next patch so this is fine.

>  But we should also mention that the size
>  must be equal or less then the resource sector size.
>
>  >  static PyObject *
>  >  py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>  >  {
>  > --
>  > 2.28.0
>  >

[PATCH 2/3] python: support size in get_lvb

[Nir Soffer]

On Wed, Dec 9, 2020 at 7:43 PM Benny Zlotnik <bzlotnik(a)redhat.com&gt; wrote:
>  Add a size parameter to allow the user choose how much data to get back,
>  the API will now look like this:
>
>      sanlock.get_lvb(lockspace, resource, disks, size)
>
>  Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  ---
>   python/sanlock.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index 567d86c..41bbb43 100644
>  --- a/python/sanlock.c
>  +++ b/python/sanlock.c
>  @@ -1659,26 +1659,28 @@ Arguments\n\
>     lockspace         lockspace name (str)\n\
>     resource          resource name (int)\n\
>     disks             path and offset (tuple)\n\
>  +  size              amount of data to read (int)\n\
>   \n\
>   Returns\n\
>     data              data written with set_lvb\n\ 
Missing \n

>   Notes\n\
>   \n\ 
Remove \n

>  -The resource must be acquired with lvb=True\n");
>  +The resource must be acquired with lvb=True\n\
>  +size is required and has to be > 0.\n"); 
Add the upper limit, see comment  I posted by mistake on the previous patch.

>   static PyObject *
>   py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>   {
>  -    uint32_t flags = 0;
>  +    uint32_t flags = 0, lvb_size = 0; 
Lets use one variable per line in new code.

>       int rv = -1;
>       struct sanlk_resource *res = NULL;
>       PyObject *lockspace = NULL, *resource = NULL;
>       PyObject *disks;
>  -    char data[512];
>  +    char *lvb_data = NULL;
>
>  -    static char *kwlist[] = {"lockspace", "resource",
> "disks", NULL};
>  -    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!", kwlist,
>  +    static char *kwlist[] = {"lockspace", "resource",
> "disks", "size", NULL};
>  +    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!K", kwlist,
K is unsigned long, we can use I (capital i) for int.

>           convert_to_pybytes, &lockspace, convert_to_pybytes,
> &resource,
>  -        &PyList_Type, &disks)) {
>  +        &PyList_Type, &disks, &lvb_size)) {
>           goto finally;
>       } 
We need to validate lvb_len. If it is out of range (0 < lvb_len
<4096), we want to
raise a ValueError right here.

>  @@ -1688,9 +1690,10 @@ py_get_lvb(PyObject *self __unused, PyObject *args, PyObject
> *keywds)
>
>       strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
>       strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
>  +    lvb_data = calloc(lvb_size, sizeof(char)); 
calloc(1, lv_len)

(see line 195)

>       Py_BEGIN_ALLOW_THREADS
>  -    rv = sanlock_get_lvb(flags, res, data, sizeof(data));
>  +    rv = sanlock_get_lvb(flags, res, lvb_data, lvb_size);
>       Py_END_ALLOW_THREADS
>
>       if (rv < 0) {
>  @@ -1705,7 +1708,7 @@ finally:
>       if (rv < 0)
>           return NULL; 
You leak lvb_data here...

>  -    return Py_BuildValue("y", data);
>  +    return Py_BuildValue("y#", lvb_data, lvb_size); 
and here.

The nicest way would be to create a bytes object from lvb_data,
so it owns the data so we don't have to copy it twice. Python does
not support this, but we can do:

    PyObject *result = NULL;
    ...
    result = PyBytes_FromStringAndSize(NULL, lvb_len);
    memset(PyBytes_AS_STRING(result), 0, lvb_len);

And pass the internal buffer to sanlock in the same way.

If the call fail we will clear the value, which decref the object and set
the pointer to NULL;

finally:
    Py_XDECREF(lockspace);
    Py_XDECREF(resource);
    free(res);
    if (rv < 0)
        Py_CLEAR(result);

    return result;

>   }
>
>   static PyMethodDef
>  --
>  2.28.0
>

[PATCH 3/3] python: improve lvb tests

[Nir Soffer]

On Wed, Dec 9, 2020 at 7:43 PM Benny Zlotnik <bzlotnik(a)redhat.com&gt; wrote:
>  - Remove null byte test, it is now tested in the basic test
>  - Add 4k tests for E2BIG failure
>  - Add test for writing entire 4k sector
>  - Add test for reading less than sector size
>
>  Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  ---
>   tests/python_test.py | 99 +++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 89 insertions(+), 10 deletions(-)
>
>  diff --git a/tests/python_test.py b/tests/python_test.py
>  index 28fd27f..b2e0198 100644
>  --- a/tests/python_test.py
>  +++ b/tests/python_test.py
>  @@ -780,17 +780,21 @@ def test_lvb(tmpdir, sanlock_daemon):
>       sanlock.write_resource(b"ls_name", b"res_name", disks)
>
>       fd = sanlock.register()
>  +    lvb_data = b"first\0second".ljust(512, b"\0")
>
>       sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  -    sanlock.set_lvb(b"ls_name", b"res_name", disks,
> b"{gen:0}")
>  +    sanlock.set_lvb(b"ls_name", b"res_name", disks, lvb_data)
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +
>  +    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  +    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
> size=512)
>  
Remove this blank line

>  -    result = sanlock.get_lvb(b"ls_name",
> b"res_name", disks)
>       sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>
>  -    assert result == b"{gen:0}"
>  +    assert result == lvb_data 
Nice

>  -def test_lvb_value_too_long(tmpdir, sanlock_daemon):
>  +def test_lvb_value_too_long_512(tmpdir, sanlock_daemon):
>       ls_path = str(tmpdir.join("ls_name"))
>       util.create_file(ls_path, MiB)
>
>  @@ -805,15 +809,86 @@ def test_lvb_value_too_long(tmpdir, sanlock_daemon):
>
>       fd = sanlock.register()
>
>  -    long_val = b"a" * 513
>  +    long_val = b"a" * (SECTOR_SIZE_512 + 1)
>  +    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  +    with raises_sanlock_errno(errno.E2BIG):
>  +        sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)
Looks good

>  +
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +
>  +
>  +(a)pytest.mark.parametrize(&quot;align&quot;, sanlock.ALIGN_SIZE)
>  +def test_lvb_value_too_long_4k(user_4k_path, sanlock_daemon, align):
>  +
>  +    disks = [(user_4k_path, 0)]
>  +
>  +    # Poison resource area, ensuring that previous tests will not break this
>  +    # test, and sanlock does not write beyond the lockspace area.
>  +    with io.open(user_4k_path, "rb+") as f:
>  +        f.write(align * b"x")
>  +    util.write_guard(user_4k_path, align)
>  +    sanlock.write_lockspace(
>  +        b"ls_name",
>  +        user_4k_path,
>  +        iotimeout=1,
>  +        align=align,
>  +        sector=SECTOR_SIZE_4K)
>  +
>  +    sanlock.add_lockspace(b"ls_name", 1, user_4k_path, offset=0, iotimeout=1)
>  +
>  +    sanlock.write_resource(
>  +        b"ls_name", b"res_name", disks, align=align,
> sector=SECTOR_SIZE_4K)
>  +
>  +    fd = sanlock.register()
>  +
>  +    long_val = b"a" * (SECTOR_SIZE_4K + 1)
>       sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>       with raises_sanlock_errno(errno.E2BIG):
>           sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)
This goes to sanlock damon just to fail with invalid value that we can check
in the python side. I think it will be easier to debug issues if we validate the
size in the python binding and raise ValueError with clear error message.

We need another test for size=0. Since we have expensive setup code
for each test,
it will be useful to test all invalid values in the same test.

>       sanlock.release(b"ls_name", b"res_name",
> disks, slkfd=fd)
>
>  +    util.check_guard(user_4k_path, align)
>  +
>
>  -def test_lvb_null_bytes(tmpdir, sanlock_daemon):
>  +(a)pytest.mark.parametrize(&quot;align&quot;, sanlock.ALIGN_SIZE)
>  +def test_lvb_use_entire_sector_4k(user_4k_path, sanlock_daemon, align): 
I think a more consistent name would be test_lvb_4k

>  +    disks = [(user_4k_path, 0)]
>  +
>  +    # Poison resource area, ensuring that previous tests will not break this
>  +    # test, and sanlock does not write beyond the lockspace area.
>  +    with io.open(user_4k_path, "rb+") as f:
>  +        f.write(align * b"x")
>  +    util.write_guard(user_4k_path, align)
>  +    sanlock.write_lockspace(
>  +        b"ls_name",
>  +        user_4k_path,
>  +        iotimeout=1,
>  +        align=align,
>  +        sector=SECTOR_SIZE_4K)
>  +
>  +    sanlock.add_lockspace(b"ls_name", 1, user_4k_path, offset=0, iotimeout=1)
>  +
>  +    sanlock.write_resource(
>  +        b"ls_name", b"res_name", disks, align=align,
> sector=SECTOR_SIZE_4K)
>  +
>  +    fd = sanlock.register()
>  +
>  +    long_val = b"a" * SECTOR_SIZE_4K
>  +    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  +    sanlock.set_lvb(b"ls_name", b"res_name", disks, long_val)
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +
>  +    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  +    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
>  +                             size=SECTOR_SIZE_4K)
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +    assert result == long_val
>  +
>  +    util.check_guard(user_4k_path, align) 
This mostly duplicates the 512 byte tests. We can avoid this using
pytest parameters,
but it require using userstorage module from pip (like ovirt-imageio).
To keep it simple
for now, lets have a check_xxx() helper accepting path and size and
have tiny tests
calling this:

def test_lvb_512():
     path = util.generate_path(...)
     check_lvb(path, 512)

def test_lvb_4k(user_4k_path):
     check_lvb(user_4k_path, 4096)

>  +
>  +
>  +def test_lvb_read_less_than_cluster_size(tmpdir, sanlock_daemon):
>       ls_path = str(tmpdir.join("ls_name"))
>       util.create_file(ls_path, MiB)
>
>  @@ -827,12 +902,16 @@ def test_lvb_null_bytes(tmpdir, sanlock_daemon):
>       sanlock.write_resource(b"ls_name", b"res_name", disks)
>
>       fd = sanlock.register()
>  +    lvb_data = b"first\0second"
>  +
>  +    sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  +    sanlock.set_lvb(b"ls_name", b"res_name", disks,
> lvb_data.ljust(512, b"\0"))
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>
>       sanlock.acquire(b"ls_name", b"res_name", disks, slkfd=fd,
> lvb=True)
>  -    sanlock.set_lvb(b"ls_name", b"res_name", disks,
> b"{ge\x00:0}")
>  +    result = sanlock.get_lvb(b"ls_name", b"res_name", disks,
>  +                             size=len(lvb_data))
>
>  -    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
>       sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>
>  -    # Check that the string we passed is terminated by the null-byte
>  -    assert result == b"{ge"
>  +    assert result == lvb_data
>  --
>  2.28.0

[PATCH 2/3] python: support size in get_lvb

[Benny Zlotnik]

> Remove \n 
ACK

> Add the upper limit, see comment  I posted by mistake on the previous
> patch. 
ACK

> Lets use one variable per line in new code. 
ACK

> K is unsigned long, we can use I (capital i) for int. 
ACK

> We need to validate lvb_len. If it is out of range (0 < lvb_len
> <4096), we want to
> raise a ValueError right here. 
ACK

> calloc(1, lv_len) 
ACK

> The nicest way would be to create a bytes object from lvb_data,
> so it owns the data so we don't have to copy it twice. Python does
> not support this, but we can do:
>
>     PyObject *result = NULL;
>     ...
>     result = PyBytes_FromStringAndSize(NULL, lvb_len);
>     memset(PyBytes_AS_STRING(result), 0, lvb_len);
>
> And pass the internal buffer to sanlock in the same way.
>
> If the call fail we will clear the value, which decref the object and set
> the pointer to NULL;
>
> finally:
>     Py_XDECREF(lockspace);
>     Py_XDECREF(resource);
>     free(res);
>     if (rv < 0)
>         Py_CLEAR(result);
>
>     return result; 
ACK