[PATCH 0/4] Add python bindings to set_lvb and get_lvb

[Benny Zlotnik]

This patch series adds python bindings to writing (set_lvb) and reading (get_lvb) data to
and from the Lock
Value Block of a sanlock resource.

Benny Zlotnik (4):
  python: add lvb flag to sanlock.acquire
  python: expose set_lvb
  python: expose get_lvb
  tests: add a test for LVB operation

 python/sanlock.c     | 140 ++++++++++++++++++++++++++++++++++++++++---
 tests/python_test.py |  23 +++++++
 2 files changed, 156 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/4] python: add lvb flag to sanlock.acquire

[Benny Zlotnik]

The flag will allow to acquire a resource with the SANLK_ACQUIRE_LVB
flag, allowing the user to write or read data from LVB space.

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

diff --git a/python/sanlock.c b/python/sanlock.c
index d616008..c1c3e8b 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -981,28 +981,31 @@ finally:
 /* acquire */
 PyDoc_STRVAR(pydoc_acquire, "\
 acquire(lockspace, resource, disks \
-[, slkfd=fd, pid=owner, shared=False, version=None])\n\
+[, slkfd=fd, pid=owner, shared=False, version=None, lvb=False])\n\
 Acquire a resource lease for the current process (using the slkfd argument\n\
 to specify the sanlock file descriptor) or for an other process (using the\n\
 pid argument). If shared is True the resource will be acquired in the shared\n\
 mode. The version is the version of the lease that must be acquired or fail.\n\
-The disks must be in the format: [(path, offset), ... ]\n");
+The disks must be in the format: [(path, offset), ... ]\n\
+If lvb is True the resource will be acquired with the LVB flag enabled\n\
+to allow access to LVB data.\n");
 
 static PyObject *
 py_acquire(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv = -1, sanlockfd = -1, pid = -1, shared = 0;
+    int rv = -1, sanlockfd = -1, pid = -1, shared = 0, lvb = 0;
+    uint32_t flags = 0;
     PyObject *lockspace = NULL, *resource = NULL;
     struct sanlk_resource *res = NULL;
     PyObject *disks, *version = Py_None;
 
     static char *kwlist[] = {"lockspace", "resource",
"disks", "slkfd",
-                                "pid", "shared", "version",
NULL};
+                                "pid", "shared", "lvb",
"version", NULL};
 
     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|iiiO",
kwlist,
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|iiiiO",
kwlist,
         convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
-        &PyList_Type, &disks, &sanlockfd, &pid, &shared,
&version)) {
+        &PyList_Type, &disks, &sanlockfd, &pid, &shared, &lvb,
&version)) {
         goto finally;
     }
 
@@ -1026,6 +1029,10 @@ py_acquire(PyObject *self __unused, PyObject *args, PyObject
*keywds)
         res->flags |= SANLK_RES_SHARED;
     }
 
+    if (lvb) {
+        flags |= SANLK_ACQUIRE_LVB;
+    }
+
     /* prepare the resource version */
     if (version != Py_None) {
         res->flags |= SANLK_RES_LVER;
@@ -1038,7 +1045,7 @@ py_acquire(PyObject *self __unused, PyObject *args, PyObject
*keywds)
 
     /* acquire sanlock resource (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
-    rv = sanlock_acquire(sanlockfd, pid, 0, 1, &res, 0);
+    rv = sanlock_acquire(sanlockfd, pid, flags, 1, &res, 0);
     Py_END_ALLOW_THREADS
 
     if (rv != 0) {
-- 
2.26.2

[PATCH 2/4] python: expose set_lvb

[Benny Zlotnik]

Add a binding to the set_lvb function to allow writing data to LVB.

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

diff --git a/python/sanlock.c b/python/sanlock.c
index c1c3e8b..b2dc624 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1594,6 +1594,63 @@ finally:
     Py_RETURN_NONE;
 }
 
+/* set_lvb */
+PyDoc_STRVAR(pydoc_set_lvb, "\
+set_lvb(lockspace, resource, disks, event, data)\n\
+Set Lock Value Block for a given resource\n\
+\n\
+Arguments\n\
+  lockspace         lockspace name (str)\n\
+  resource          resource name (int)\n\
+  disks             path and offset (tuple)\n\
+  data              data to write (str)\n\
+\n\
+Notes\n\
+\n\
+The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+static PyObject *
+py_set_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
+{
+    uint32_t flags = 0;
+    int rv = -1;
+    struct sanlk_resource *res;
+    PyObject *lockspace = NULL, *resource = NULL, *data = NULL;
+    PyObject *disks;
+
+    static char *kwlist[] = {"lockspace", "resource",
"disks", "data", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!O&",
kwlist,
+                                     convert_to_pybytes, &lockspace,
convert_to_pybytes, &resource, &PyList_Type, &disks, convert_to_pybytes,
&data))
+    {
+        goto finally;
+    }
+
+    if (parse_disks(disks, &res )< 0)
+    {
+        goto finally;
+    }
+
+    strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
+    strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
+
+    Py_BEGIN_ALLOW_THREADS
+        rv = sanlock_set_lvb(flags, res, PyBytes_AS_STRING(data),
PyBytes_GET_SIZE(data));
+    Py_END_ALLOW_THREADS
+
+    if (rv < 0)
+    {
+        set_sanlock_error(rv, "Unable to set lvb");
+        goto finally;
+    }
+
+finally:
+    Py_XDECREF(lockspace);
+    Py_XDECREF(resource);
+    free(res);
+    if (rv < 0)
+        return NULL;
+    Py_RETURN_NONE;
+}
+
 static PyMethodDef
 sanlock_methods[] = {
     {"register", py_register, METH_NOARGS, pydoc_register},
@@ -1631,6 +1688,9 @@ sanlock_methods[] = {
     {"end_event", (PyCFunction) py_end_event, METH_VARARGS, pydoc_end_event},
     {"set_event", (PyCFunction) py_set_event,
                 METH_VARARGS|METH_KEYWORDS, pydoc_set_event},
+    {"set_lvb", (PyCFunction) py_set_lvb,
+         METH_VARARGS|METH_KEYWORDS, pydoc_set_lvb},
+
     {NULL, NULL, 0, NULL}
 };
 
-- 
2.26.2

[PATCH 3/4] python: expose get_lvb

[Benny Zlotnik]

Add a binding to the get_lvb function to allow reading data from LVB.

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

diff --git a/python/sanlock.c b/python/sanlock.c
index b2dc624..4803b1f 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1651,6 +1651,63 @@ finally:
     Py_RETURN_NONE;
 }
 
+PyDoc_STRVAR(pydoc_get_lvb, "\
+get_lvb(lockspace, resource, disks, event, data)\n\
+Read Lock Value Block for a given resource\n\
+\n\
+Arguments\n\
+  lockspace         lockspace name (str)\n\
+  resource          resource name (int)\n\
+  disks             path and offset (tuple)\n\
+\n\
+Notes\n\
+\n\
+The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+static PyObject *
+py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
+{
+    uint32_t flags = 0;
+    int rv = -1;
+    struct sanlk_resource *res;
+    PyObject *lockspace = NULL, *resource = NULL;
+    PyObject *disks;
+    char data[512];
+
+    static char *kwlist[] = {"lockspace", "resource",
"disks", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!", kwlist,
+                                     convert_to_pybytes, &lockspace,
convert_to_pybytes, &resource, &PyList_Type, &disks))
+    {
+        goto finally;
+    }
+
+    if (parse_disks(disks, &res )< 0)
+    {
+        goto finally;
+    }
+
+    strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
+    strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
+
+    Py_BEGIN_ALLOW_THREADS
+        rv = sanlock_get_lvb(flags, res, &data, sizeof(data));
+    Py_END_ALLOW_THREADS
+
+    if (rv < 0)
+    {
+        set_sanlock_error(rv, "Unable to get lvb");
+        goto finally;
+    }
+
+finally:
+    Py_XDECREF(lockspace);
+    Py_XDECREF(resource);
+    free(res);
+    if (rv < 0)
+        return NULL;
+
+    return Py_BuildValue("s", data);
+}
+
 static PyMethodDef
 sanlock_methods[] = {
     {"register", py_register, METH_NOARGS, pydoc_register},
@@ -1690,6 +1747,8 @@ sanlock_methods[] = {
                 METH_VARARGS|METH_KEYWORDS, pydoc_set_event},
     {"set_lvb", (PyCFunction) py_set_lvb,
          METH_VARARGS|METH_KEYWORDS, pydoc_set_lvb},
+    {"get_lvb", (PyCFunction) py_get_lvb,
+         METH_VARARGS|METH_KEYWORDS, pydoc_get_lvb},
 
     {NULL, NULL, 0, NULL}
 };
-- 
2.26.2

[PATCH v2 1/2] python: expose get_lvb

[Benny Zlotnik]

Add a binding to the get_lvb function to allow reading data from LVB.

Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
---
Changes in v2:
  - use 'y' instead of 's' in Py_BuildValue

 python/sanlock.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/python/sanlock.c b/python/sanlock.c
index b2dc624..ee9e500 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1651,6 +1651,63 @@ finally:
     Py_RETURN_NONE;
 }
 
+PyDoc_STRVAR(pydoc_get_lvb, "\
+get_lvb(lockspace, resource, disks, event, data)\n\
+Read Lock Value Block for a given resource\n\
+\n\
+Arguments\n\
+  lockspace         lockspace name (str)\n\
+  resource          resource name (int)\n\
+  disks             path and offset (tuple)\n\
+\n\
+Notes\n\
+\n\
+The resource must be acquired with the SANLK_ACQUIRE_LVB flag\n");
+static PyObject *
+py_get_lvb(PyObject *self __unused, PyObject *args, PyObject *keywds)
+{
+    uint32_t flags = 0;
+    int rv = -1;
+    struct sanlk_resource *res;
+    PyObject *lockspace = NULL, *resource = NULL;
+    PyObject *disks;
+    char data[512];
+
+    static char *kwlist[] = {"lockspace", "resource",
"disks", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!", kwlist,
+                                     convert_to_pybytes, &lockspace,
convert_to_pybytes, &resource, &PyList_Type, &disks))
+    {
+        goto finally;
+    }
+
+    if (parse_disks(disks, &res )< 0)
+    {
+        goto finally;
+    }
+
+    strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
+    strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);
+
+    Py_BEGIN_ALLOW_THREADS
+        rv = sanlock_get_lvb(flags, res, &data, sizeof(data));
+    Py_END_ALLOW_THREADS
+
+    if (rv < 0)
+    {
+        set_sanlock_error(rv, "Unable to get lvb");
+        goto finally;
+    }
+
+finally:
+    Py_XDECREF(lockspace);
+    Py_XDECREF(resource);
+    free(res);
+    if (rv < 0)
+        return NULL;
+
+    return Py_BuildValue("y", data);
+}
+
 static PyMethodDef
 sanlock_methods[] = {
     {"register", py_register, METH_NOARGS, pydoc_register},
@@ -1690,6 +1747,8 @@ sanlock_methods[] = {
                 METH_VARARGS|METH_KEYWORDS, pydoc_set_event},
     {"set_lvb", (PyCFunction) py_set_lvb,
          METH_VARARGS|METH_KEYWORDS, pydoc_set_lvb},
+    {"get_lvb", (PyCFunction) py_get_lvb,
+         METH_VARARGS|METH_KEYWORDS, pydoc_get_lvb},
 
     {NULL, NULL, 0, NULL}
 };
-- 
2.26.2

[PATCH 4/4] tests: add a test for LVB operation

[Benny Zlotnik]

This patch tests writing and reading to LVB via python bindings.

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

diff --git a/tests/python_test.py b/tests/python_test.py
index abd459e..6316790 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -764,3 +764,26 @@ def test_acquire_path_length(no_sanlock_daemon):
     path = "x" * (constants.SANLK_PATH_LEN - 1)
     with raises_sanlock_errno():
         sanlock.acquire(b"ls_name", b"res_name", [(path, 0)],
pid=os.getpid())
+
+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}")
+
+    result = sanlock.get_lvb(b"ls_name", b"res_name", disks)
+    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
+
+    assert result == "{gen:0}"
-- 
2.26.2

[PATCH 4/4] tests: add a test for LVB operation

[Nir Soffer]

On Mon, Oct 5, 2020 at 1:45 PM Benny Zlotnik <bzlotnik(a)redhat.com&gt; wrote:
>  This patch tests writing and reading to LVB via python bindings.
>
>  Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
>  ---
>   tests/python_test.py | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
>  diff --git a/tests/python_test.py b/tests/python_test.py
>  index abd459e..6316790 100644
>  --- a/tests/python_test.py
>  +++ b/tests/python_test.py
>  @@ -764,3 +764,26 @@ def test_acquire_path_length(no_sanlock_daemon):
>       path = "x" * (constants.SANLK_PATH_LEN - 1)
>       with raises_sanlock_errno():
>           sanlock.acquire(b"ls_name", b"res_name", [(path, 0)],
> pid=os.getpid())
>  +
>  +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}") 
Accessing lvm works only when the resource is acquired?

>  +    result = sanlock.get_lvb(b"ls_name",
> b"res_name", disks)
>  +    sanlock.release(b"ls_name", b"res_name", disks, slkfd=fd)
>  +
>  +    assert result == "{gen:0}" 
Nice test, but it shows that get_lvb is *decoding* the data on output,
which is wrong. The API should accept and return the same type.

In the current form, if I try to set_lvb() with unicode value encoded
in another encoding, like:

> >> "\u05d0".encode("cp1255")
b'\xe0'

When reading the python bindings will try to decode using "utf-8"
and:

> >> b'\xe0'.decode("utf-8") 
Traceback
(most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe0 in position
0: unexpected end of data

The issue is probably using the wrong format string when
building the return value.

Other issues that should be tested:
- Handling of too long input - the input should not be truncated silently but
  but fail the call.
- Handling of null bytes in the lvm block - is this supported by sanlock or
  it treats lvm as a null terminated string?

I did not have time yet to review the other patches. The user facing API
makes sense except the decoding issue.

Nir

[PATCH 4/4] tests: add a test for LVB operation

[Benny Zlotnik]

>  Accessing lvm works only when the resource is acquired? 
Yes,
needs to be acquired with SANLK_ACQUIRE_LVB flag (added lvb=True
for this in patch 1)

>  Nice test, but it shows that get_lvb is *decoding* the data on
> output,
>  which is wrong. The API should accept and return the same type.
>
>  In the current form, if I try to set_lvb() with unicode value encoded
>  in another encoding, like:
>
>  >>> "\u05d0".encode("cp1255")
>  b'\xe0'
>
>  When reading the python bindings will try to decode using "utf-8"
>  and:
>
>  >>> b'\xe0'.decode("utf-8")
>  Traceback (most recent call last):
>    File "<stdin>", line 1, in <module>
>  UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe0 in position
>  0: unexpected end of data
>
>  The issue is probably using the wrong format string when
>  building the return value. 
Thanks, it was indeed wrong, used 's' instead of
'y'

>  Other issues that should be tested:
>  - Handling of too long input - the input should not be truncated silently but
>    but fail the call. 
ack, sanlock returns E2BIG when it's larger than the
sector size, will
add a test

>  - Handling of null bytes in the lvm block - is this supported by
> sanlock or
>    it treats lvm as a null terminated string? 
ack, it terminates the string, will
add a test


>  I did not have time yet to review the other patches. The user facing
> API
>  makes sense except the decoding issue.
>
>  Nir
>

[PATCH v2] tests: add a test for LVB operation

[Benny Zlotnik]

This patch add tests for:
- Writing and reading the value to and from LVB
- Proper handling of a value that is too long
- Null-byte string termination

Signed-off-by: Benny Zlotnik <bzlotnik(a)redhat.com&gt;
---
Changes in v2:
- Adjusted test_lvb to receive bytes in get_lvb as were sent in set_lvb
- Added test_lvb_value_too_long
- Added test_lvb_null_bytes

 tests/python_test.py | 72 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)


diff --git a/tests/python_test.py b/tests/python_test.py
index abd459e..28fd27f 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -764,3 +764,75 @@ def test_acquire_path_length(no_sanlock_daemon):
     path = "x" * (constants.SANLK_PATH_LEN - 1)
     with raises_sanlock_errno():
         sanlock.acquire(b"ls_name", b"res_name", [(path, 0)],
pid=os.getpid())
+
+
+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}")
+
+    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}"
+
+
+def test_lvb_value_too_long(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()
+
+    long_val = b"a" * 513
+    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)
+
+    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)
+
+    # Check that the string we passed is terminated by the null-byte
+    assert result == b"{ge"
-- 
2.26.2