[PATCH v2 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;
---
 Changes in v2:
  Remove newline and missing newline

 python/sanlock.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index fbcabb1..8a45bff 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)
 {
@@ -1660,9 +1662,9 @@ Arguments\n\
 \n\
 Returns\n\
   data              data written with set_lvb\n\
-Notes\n\
 \n\
-The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+Notes\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 v2 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;
---
Changes in v2:
  - Add upper limit to doc
  - Use I instead of K
  - Add lvb_len validation
  - Use python object instead of char*

 python/sanlock.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index 8a45bff..0c6a80c 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1606,7 +1606,6 @@ Arguments\n\
   data              data to write (bytes)\n\
 \n\
 Notes\n\
-\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");
@@ -1659,26 +1658,34 @@ 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\
 \n\
 Notes\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 in range 0 < n <= 4096.\n");
 static PyObject *
 py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
     uint32_t flags = 0;
+    uint32_t lvb_len = 0;
     int rv = -1;
     struct sanlk_resource *res = NULL;
     PyObject *lockspace = NULL, *resource = NULL;
-    PyObject *disks;
-    char data[512];
+    PyObject *disks = NULL;
+    PyObject *result = 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!I", kwlist,
         convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
-        &PyList_Type, &disks)) {
+        &PyList_Type, &disks, &lvb_len)) {
+        goto finally;
+    }
+
+    if (lvb_len < 1 || lvb_len > 4096) {
+        PyErr_Format(PyExc_ValueError, "Invalid size %d, must be in range: 0 <
size <= 4096", lvb_len);
         goto finally;
     }

@@ -1689,8 +1696,15 @@ 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);

+    result = PyBytes_FromStringAndSize(NULL, lvb_len);
+    if (result == NULL) {
+        goto finally;
+    }
+
+    memset(PyBytes_AS_STRING(result), 0, lvb_len);
+
     Py_BEGIN_ALLOW_THREADS
-    rv = sanlock_get_lvb(flags, res, data, sizeof(data));
+    rv = sanlock_get_lvb(flags, res, PyBytes_AS_STRING(result), lvb_len);
     Py_END_ALLOW_THREADS

     if (rv < 0) {
@@ -1703,9 +1717,9 @@ finally:
     Py_XDECREF(resource);
     free(res);
     if (rv < 0)
-        return NULL;
+        Py_CLEAR(result);

-    return Py_BuildValue("y", data);
+    return result;
 }

 static PyMethodDef
-- 
2.28.0

[PATCH v2 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;
---
Changes in v2:
- Consolidate all writing tests to test_lvb
- Improve invalid values test by not using daemon and only testing
  binding failure
 tests/python_test.py | 63 +++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 42 deletions(-)

diff --git a/tests/python_test.py b/tests/python_test.py
index 28fd27f..2cc7d25 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -769,70 +769,49 @@ def test_acquire_path_length(no_sanlock_daemon):
 def test_lvb(tmpdir, sanlock_daemon):
     ls_path = str(tmpdir.join("ls_name"))
     util.create_file(ls_path, MiB)
-
     res_path = str(tmpdir.join("res_name"))
     util.create_file(res_path, MiB)
-
     sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
     sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
-
     disks = [(res_path, 0)]
     sanlock.write_resource(b"ls_name", b"res_name", disks)

     fd = sanlock.register()

-    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}")
+    # Test full cluster write.
+    lvb_data = b"first\0second".ljust(512, b"\0")

-    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
+    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)
     sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

-    assert result == b"{gen:0}"
-
-
-def test_lvb_value_too_long(tmpdir, sanlock_daemon):
-    ls_path = str(tmpdir.join("ls_name"))
-    util.create_file(ls_path, MiB)
+    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)
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

-    res_path = str(tmpdir.join("res_name"))
-    util.create_file(res_path, MiB)
+    assert result == lvb_data

-    sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
-    sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
+    # Test read sess than cluster size.
+    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=len(lvb_data))
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)

-    disks = [(res_path, 0)]
-    sanlock.write_resource(b"ls_name", b"res_name", disks)
+    assert result == lvb_data

-    fd = sanlock.register()
-
-    long_val = b"a" * 513
+    # Test failure to write data larger than cluster size.
+    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)
-
-
-def test_lvb_null_bytes(tmpdir, sanlock_daemon):
-    ls_path = str(tmpdir.join("ls_name"))
-    util.create_file(ls_path, MiB)

+def test_lvb_invalid_value_512(tmpdir):
     res_path = str(tmpdir.join("res_name"))
-    util.create_file(res_path, MiB)
-
-    sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
-    sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
-
     disks = [(res_path, 0)]
-    sanlock.write_resource(b"ls_name", b"res_name", disks)

-    fd = sanlock.register()
-
-    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)
-    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+    with pytest.raises(ValueError):
+        sanlock.get_lvb(b"ls_name", b"res_name", disks, size=0)

-    # Check that the string we passed is terminated by the null-byte
-    assert result == b"{ge"
+    with pytest.raises(ValueError):
+        sanlock.get_lvb(b"ls_name", b"res_name", disks, size=4097)
-- 
2.28.0

[Nir Soffer]

On Thu, Dec 10, 2020 at 7:23 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;
>  ---
>   Changes in v2:
>    Remove newline and missing newline
>
>   python/sanlock.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index fbcabb1..8a45bff 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)
>   {
>  @@ -1660,9 +1662,9 @@ Arguments\n\
>   \n\
>   Returns\n\
>     data              data written with set_lvb\n\
>  -Notes\n\
>   \n\
>  -The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
>  +Notes\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
>  
Looks good.

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

[Nir Soffer]

On Thu, Dec 10, 2020 at 7:23 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;
>  ---
>  Changes in v2:
>    - Add upper limit to doc
>    - Use I instead of K
>    - Add lvb_len validation
>    - Use python object instead of char*
>
>   python/sanlock.c | 34 ++++++++++++++++++++++++----------
>   1 file changed, 24 insertions(+), 10 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index 8a45bff..0c6a80c 100644
>  --- a/python/sanlock.c
>  +++ b/python/sanlock.c
>  @@ -1606,7 +1606,6 @@ Arguments\n\ 
Need to add size to to signature:

$ PYTHONPATH=python/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 in range 0 < n <= 4096.

I will add it before merging.

>     data              data to write (bytes)\n\
>   \n\
>   Notes\n\
>  -\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");
>  @@ -1659,26 +1658,34 @@ 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\
>   \n\
>   Notes\n\
>  -The resource must be acquired with lvb=True\n");
>  +The resource must be acquired with lvb=True\n\ 
Missing trailing . - I will add it before merging.

>  +size is required and has to be in range 0 < n <=
> 4096.\n"); 
size is defined as a required argument, so we don't need to discuss
this in the notes.
I will fix this before merging.

>   static PyObject *
>   py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
>   {
>       uint32_t flags = 0;
>  +    uint32_t lvb_len = 0;
>       int rv = -1;
>       struct sanlk_resource *res = NULL;
>       PyObject *lockspace = NULL, *resource = NULL;
>  -    PyObject *disks;
>  -    char data[512];
>  +    PyObject *disks = NULL;
>  +    PyObject *result = 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!I", kwlist,
>           convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
>  -        &PyList_Type, &disks)) {
>  +        &PyList_Type, &disks, &lvb_len)) {
>  +        goto finally;
>  +    }
>  +
>  +    if (lvb_len < 1 || lvb_len > 4096) {
>  +        PyErr_Format(PyExc_ValueError, "Invalid size %d, must be in range: 0 <
> size <= 4096", lvb_len); 
unit32_t requires %u format, I will update it.

>           goto finally;
>       }
>
>  @@ -1689,8 +1696,15 @@ 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);
>
>  +    result = PyBytes_FromStringAndSize(NULL, lvb_len);
>  +    if (result == NULL) {
>  +        goto finally;
>  +    }
>  +
>  +    memset(PyBytes_AS_STRING(result), 0, lvb_len);
>  +
>       Py_BEGIN_ALLOW_THREADS
>  -    rv = sanlock_get_lvb(flags, res, data, sizeof(data));
>  +    rv = sanlock_get_lvb(flags, res, PyBytes_AS_STRING(result), lvb_len);
>       Py_END_ALLOW_THREADS
>
>       if (rv < 0) {
>  @@ -1703,9 +1717,9 @@ finally:
>       Py_XDECREF(resource);
>       free(res);
>       if (rv < 0)
>  -        return NULL;
>  +        Py_CLEAR(result);
>
>  -    return Py_BuildValue("y", data);
>  +    return result;
>   }
>
>   static PyMethodDef
>  --
>  2.28.0
>  
Look good otherwise, thanks!

[PATCH v2 3/3] python: improve lvb tests

[Nir Soffer]

On Thu, Dec 10, 2020 at 7:23 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 
We don't have 4k tests yet, I'll update the commit message.

>  - Add test for reading less than sector size
>
>  Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  ---
>  Changes in v2:
>  - Consolidate all writing tests to test_lvb
>  - Improve invalid values test by not using daemon and only testing
>    binding failure
>   tests/python_test.py | 63 +++++++++++++++-----------------------------
>   1 file changed, 21 insertions(+), 42 deletions(-)
>
>  diff --git a/tests/python_test.py b/tests/python_test.py
>  index 28fd27f..2cc7d25 100644
>  --- a/tests/python_test.py
>  +++ b/tests/python_test.py
>  @@ -769,70 +769,49 @@ def test_acquire_path_length(no_sanlock_daemon):
>   def test_lvb(tmpdir, sanlock_daemon):
>       ls_path = str(tmpdir.join("ls_name"))
>       util.create_file(ls_path, MiB)
>  -
>       res_path = str(tmpdir.join("res_name"))
>       util.create_file(res_path, MiB)
>  -
>       sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
>       sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
>  -
>       disks = [(res_path, 0)]
>       sanlock.write_resource(b"ls_name", b"res_name", disks)
>
>       fd = sanlock.register()
>
>  -    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}")
>  +    # Test full cluster write.
>  +    lvb_data = b"first\0second".ljust(512, b"\0")
>
>  -    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
>  +    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)
>       sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>
>  -    assert result == b"{gen:0}"
>  -
>  -
>  -def test_lvb_value_too_long(tmpdir, sanlock_daemon):
>  -    ls_path = str(tmpdir.join("ls_name"))
>  -    util.create_file(ls_path, MiB)
>  +    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)
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>
>  -    res_path = str(tmpdir.join("res_name"))
>  -    util.create_file(res_path, MiB)
>  +    assert result == lvb_data
>
>  -    sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
>  -    sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
>  +    # Test read sess than cluster size.
>  +    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=len(lvb_data)) 
This tests the same data, I'll fix this before merging.

>  +    sanlock.release(b"ls_name", b"res_name",
> disks, slkfd=fd)
>
>  -    disks = [(res_path, 0)]
>  -    sanlock.write_resource(b"ls_name", b"res_name", disks)
>  +    assert result == lvb_data
>
>  -    fd = sanlock.register()
>  -
>  -    long_val = b"a" * 513
>  +    # Test failure to write data larger than cluster size.
>  +    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)
>  -
>  -
>  -def test_lvb_null_bytes(tmpdir, sanlock_daemon):
>  -    ls_path = str(tmpdir.join("ls_name"))
>  -    util.create_file(ls_path, MiB)
>
>  +def test_lvb_invalid_value_512(tmpdir):
>       res_path = str(tmpdir.join("res_name")) 
We don't need tmpdir, since we don't reach storage in this test. Also
tmpdir.join() does not create the file, so we can just use "/no/such/path".
I'll fix it before merge.

>  -    util.create_file(res_path, MiB)
>  -
>  -    sanlock.write_lockspace(b"ls_name", ls_path, offset=0, iotimeout=1)
>  -    sanlock.add_lockspace(b"ls_name", 1, ls_path, offset=0, iotimeout=1)
>  -
>       disks = [(res_path, 0)]
>  -    sanlock.write_resource(b"ls_name", b"res_name", disks)
>
>  -    fd = sanlock.register()
>  -
>  -    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)
>  -    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +    with pytest.raises(ValueError):
>  +        sanlock.get_lvb(b"ls_name", b"res_name", disks, size=0)
>
>  -    # Check that the string we passed is terminated by the null-byte
>  -    assert result == b"{ge"
>  +    with pytest.raises(ValueError):
>  +        sanlock.get_lvb(b"ls_name", b"res_name", disks, size=4097)
>  --
>  2.28.0
>