[PATCH v4 0/2] Values instead of flags in python API.

[Vojtech Juranek]

To have more consisten python API, value should be used for secor
size and alignment instead of flags. This patch set replaces flags
with values in public API, expose tuples with supported sector size
and alignment values and add some related tests.

The fourth version of this patch set addresses Nir's comments (besides
others squashes the changes into two patches).

Vojtech Juranek (2):
  Accept values instead of flags for sector and aling parameters.
  Expose tuples with supported align and sector values

 python/example.py    |   4 +-
 python/sanlock.c     | 141 ++++++++++++++++++++++++++++++++-----------
 tests/python_test.py |  59 ++++++++++++++++++
 3 files changed, 166 insertions(+), 38 deletions(-)

-- 
2.20.1

[PATCH v4 1/2] Accept values instead of flags for sector and aling parameters.

[Vojtech Juranek]

Currently appropriate sanlock flags are accepted for sector size and
alignment parameters. This can be confusing for users, which may try
to pass values instead of flags and this approach also exposes sanlock
internal implementation details.

In order to have consistent and more intuitive API, values should be
accepted for sector size and alignment parameters. This patch replaces
flags by values in calls which accept sector size and alignment.

This patch also adds validation of provided values. If the value is
not the supported one, error with error code EINVAL is thrown.

The supported values are:
sector: 512, 4096
align: 1048576, 2097152, 4194304, 8388608

The patch also adds sector and align parameters to read and write
lockspace and resource calls to test_write_lockspace and
test_write_resource tests to test that these parameters are accepted.
Other tests still use calls without these parameters to test the
defaults. Tests for invalid align and sector values are also added.
---
 python/example.py    |   4 +-
 python/sanlock.c     | 120 ++++++++++++++++++++++++++++++++-----------
 tests/python_test.py |  59 +++++++++++++++++++++
 3 files changed, 152 insertions(+), 31 deletions(-)

diff --git a/python/example.py b/python/example.py
index 4fe845d..cb8f3e9 100644
--- a/python/example.py
+++ b/python/example.py
@@ -29,10 +29,10 @@ def main():
     fd = sanlock.register()
 
     print "Initializing '%s'" % (LOCKSPACE_NAME,)
-    sanlock.write_lockspace(LOCKSPACE_NAME, disk, max_hosts=0, iotimeout=0,
align=sanlock.ALIGN1M, sector=sanlock.SECTOR512)
+    sanlock.write_lockspace(LOCKSPACE_NAME, disk, max_hosts=0, iotimeout=0,
align=1048576, sector=512)
 
     print "Initializing '%s' on '%s'" % (RESOURCE_NAME,
LOCKSPACE_NAME)
-    sanlock.write_resource(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,
align=sanlock.ALIGN1M, sector=sanlock.SECTOR512)
+    sanlock.write_resource(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, align=1048576,
sector=512)
 
     print "Acquiring the id '%i' on '%s'" % (HOST_ID,
LOCKSPACE_NAME)
     sanlock.add_lockspace(LOCKSPACE_NAME, HOST_ID, disk)
diff --git a/python/sanlock.c b/python/sanlock.c
index 2ad4497..d5bff8e 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -135,6 +135,49 @@ exit_fail:
     return -1;
 }
 
+enum {SECTOR_SIZE_512 = 512, SECTOR_SIZE_4K = 4096};
+
+static uint32_t
+sector_to_flag(int sector, uint32_t *flags)
+{
+    switch (sector) {
+    case SECTOR_SIZE_512:
+        *flags |= SANLK_LSF_SECTOR512;
+	break;
+    case SECTOR_SIZE_4K:
+        *flags |= SANLK_LSF_SECTOR4K;
+	break;
+    default:
+	PyErr_Format(PyExc_ValueError, "Invalid sector value: %d", sector);
+	return -1;
+    }
+    return 0;
+}
+
+enum {ALIGNMENT_1M = 1048576, ALIGNMENT_2M = 2097152, ALIGNMENT_4M = 4194304,
ALIGNMENT_8M = 8388608};
+
+static uint32_t
+align_to_flag(long align, uint32_t *flags)
+{
+    switch (align) {
+    case ALIGNMENT_1M:
+        *flags |= SANLK_RES_ALIGN1M;
+	break;
+    case ALIGNMENT_2M:
+        *flags |= SANLK_RES_ALIGN2M;
+	break;
+    case ALIGNMENT_4M:
+        *flags |= SANLK_RES_ALIGN4M;
+	break;
+    case ALIGNMENT_8M:
+        *flags |= SANLK_RES_ALIGN8M;
+    default:
+	PyErr_Format(PyExc_ValueError, "Invalid align value: %ld", align);
+	return -1;
+    }
+    return 0;
+}
+
 static PyObject *
 __hosts_to_list(struct sanlk_host *hss, int hss_count)
 {
@@ -361,15 +404,17 @@ exit_fail:
 /* write_lockspace */
 PyDoc_STRVAR(pydoc_write_lockspace, "\
 write_lockspace(lockspace, path, offset=0, max_hosts=0, iotimeout=0, \
-align=ALIGN1M, sector=SECTOR512)\n\
-Initialize or update a device to be used as sanlock lockspace.");
+align=1048576, sector=512)\n\
+Initialize or update a device to be used as sanlock lockspace.\n\
+Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
+Sector can be one of (512, 4096).");
 
 static PyObject *
 py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv, max_hosts = 0;
+    int rv, max_hosts = 0, sector = SECTOR_SIZE_512;
+    long align = ALIGNMENT_1M;
     uint32_t io_timeout = 0;
-    uint32_t  align=SANLK_LSF_ALIGN1M, sector=SANLK_LSF_SECTOR512;
     const char *lockspace, *path;
     struct sanlk_lockspace ls;
 
@@ -380,7 +425,7 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     memset(&ls, 0, sizeof(struct sanlk_lockspace));
 
     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ss|kiIII", kwlist,
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ss|kiIli", kwlist,
         &lockspace, &path, &ls.host_id_disk.offset, &max_hosts,
         &io_timeout, &align, &sector)) {
         return NULL;
@@ -391,9 +436,12 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     strncpy(ls.host_id_disk.path, path, SANLK_PATH_LEN - 1);
 
     /* set alignment/sector flags */
-    ls.flags |= align;
-    ls.flags |= sector;
+    if (align_to_flag(align, &ls.flags) == -1)
+        return NULL;
 
+    if (sector_to_flag(sector, &ls.flags) == -1)
+        return NULL;
+    
     /* write sanlock lockspace (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
     rv = sanlock_write_lockspace(&ls, max_hosts, 0, io_timeout);
@@ -409,15 +457,17 @@ py_write_lockspace(PyObject *self __unused, PyObject *args, PyObject
*keywds)
 
 /* read_lockspace */
 PyDoc_STRVAR(pydoc_read_lockspace, "\
-read_lockspace(path, offset=0, align=ALIGN1M, sector=SECTOR512)\n -> dict\n\
-Read the lockspace information from a device at a specific offset.");
+read_lockspace(path, offset=0, align=1048576, sector=512)\n -> dict\n\
+Read the lockspace information from a device at a specific offset.\n\
+Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
+Sector can be one of (512, 4096).");
 
 static PyObject *
 py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv;
+    int rv, sector = SECTOR_SIZE_512;
+    long align = ALIGNMENT_1M;
     uint32_t io_timeout = 0;
-    uint32_t  align=SANLK_LSF_ALIGN1M, sector=SANLK_LSF_SECTOR512;
     const char *path;
     struct sanlk_lockspace ls;
     PyObject *ls_info = NULL, *ls_entry = NULL;
@@ -428,7 +478,7 @@ py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     memset(&ls, 0, sizeof(struct sanlk_lockspace));
 
     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kII", kwlist,
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kli", kwlist,
         &path, &ls.host_id_disk.offset, &align, &sector)) {
         return NULL;
     }
@@ -437,8 +487,11 @@ py_read_lockspace(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     strncpy(ls.host_id_disk.path, path, SANLK_PATH_LEN - 1);
 
     /* set alignment/sector flags */
-    ls.flags |= align;
-    ls.flags |= sector;
+    if (align_to_flag(align, &ls.flags) == -1)
+        return NULL;
+
+    if (sector_to_flag(sector, &ls.flags) == -1)
+        return NULL;
 
     /* read sanlock lockspace (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
@@ -481,14 +534,16 @@ exit_fail:
 
 /* read_resource */
 PyDoc_STRVAR(pydoc_read_resource, "\
-read_resource(path, offset=0, align=ALIGN1M, sector=SECTOR512) -> dict\n\
-Read the resource information from a device at a specific offset.");
+read_resource(path, offset=0, align=1048576, sector=512) -> dict\n\
+Read the resource information from a device at a specific offset.\n\
+Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
+Sector can be one of (512, 4096).");
 
 static PyObject *
 py_read_resource(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv, rs_len;
-    uint32_t  align=SANLK_RES_ALIGN1M, sector=SANLK_RES_SECTOR512;
+    int rv, rs_len, sector = SECTOR_SIZE_512;
+    long align = ALIGNMENT_1M;
     const char *path;
     struct sanlk_resource *rs;
     PyObject *rs_info = NULL, *rs_entry = NULL;
@@ -509,7 +564,7 @@ py_read_resource(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     rs->num_disks = 1;
 
     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kII", kwlist,
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "s|kli", kwlist,
         &path, &(rs->disks[0].offset), &align, &sector)) {
         goto exit_fail;
     }
@@ -518,8 +573,11 @@ py_read_resource(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     strncpy(rs->disks[0].path, path, SANLK_PATH_LEN - 1);
 
     /* set alignment/sector flags */
-    rs->flags |= align;
-    rs->flags |= sector;
+    if (align_to_flag(align, &rs->flags) == -1)
+        goto exit_fail;
+
+    if (sector_to_flag(sector, &rs->flags) == -1)
+        goto exit_fail;
 
     /* read sanlock resource (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
@@ -573,17 +631,19 @@ exit_fail:
 /* write_resource */
 PyDoc_STRVAR(pydoc_write_resource, "\
 write_resource(lockspace, resource, disks, max_hosts=0, num_hosts=0, \
-clear=False, align=ALIGN1M, sector=SECTOR512)\n\
+clear=False, align=1048576, sector=512)\n\
 Initialize a device to be used as sanlock resource.\n\
 The disks must be in the format: [(path, offset), ... ].\n\
 If clear is True, the resource is cleared so subsequent read will\n\
-return an error.");
+return an error.\n\
+Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
+Sector can be one of (512, 4096).");
 
 static PyObject *
 py_write_resource(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv, max_hosts = 0, num_hosts = 0, clear = 0;
-    uint32_t  align=SANLK_RES_ALIGN1M, sector=SANLK_RES_SECTOR512;
+    int rv, max_hosts = 0, num_hosts = 0, clear = 0, sector = SECTOR_SIZE_512;
+    long align = ALIGNMENT_1M;
     const char *lockspace, *resource;
     struct sanlk_resource *rs;
     PyObject *disks;
@@ -593,7 +653,7 @@ py_write_resource(PyObject *self __unused, PyObject *args, PyObject
*keywds)
                                 "num_hosts", "clear",
"align", "sector", NULL};
 
     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiiII",
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiili",
         kwlist, &lockspace, &resource, &PyList_Type, &disks,
&max_hosts,
         &num_hosts, &clear, &align, &sector)) {
         return NULL;
@@ -609,8 +669,11 @@ py_write_resource(PyObject *self __unused, PyObject *args, PyObject
*keywds)
     strncpy(rs->name, resource, SANLK_NAME_LEN);
 
     /* set alignment/sector flags */
-    rs->flags |= align;
-    rs->flags |= sector;
+    if (align_to_flag(align, &rs->flags) == -1)
+        goto exit_fail;
+
+    if (sector_to_flag(sector, &rs->flags) == -1)
+        goto exit_fail;
 
     if (clear) {
         flags |= SANLK_WRITE_CLEAR;
@@ -1661,6 +1724,5 @@ initsanlock(void)
     PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
     PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
     PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
-
 #undef PYSNLK_INIT_ADD_CONSTANT
 }
diff --git a/tests/python_test.py b/tests/python_test.py
index 5029924..2e5963f 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -23,6 +23,9 @@ LARGE_FILE_SIZE = 1024**4
 LOCKSPACE_SIZE = 1024**2
 MIN_RES_SIZE = 1024**2
 
+ALIGNMENT_1M = 1024**2
+SECTOR_SIZE_512 = 512
+
 
 @pytest.mark.parametrize("size,offset", [
     # Smallest offset.
@@ -34,11 +37,21 @@ def test_write_lockspace(tmpdir, sanlock_daemon, size, offset):
     path = str(tmpdir.join("lockspace"))
     util.create_file(path, size)
 
+    # test read and write with default alignment nad sector size values
     sanlock.write_lockspace("name", path, offset=offset, iotimeout=1)
 
     ls = sanlock.read_lockspace(path, offset=offset)
     assert ls == {"iotimeout": 1, "lockspace": "name"}
 
+    # test read and write with explicit alignment nad sector size values
+    sanlock.write_lockspace(
+        "name", path, offset=offset, iotimeout=1, align=ALIGNMENT_1M,
+        sector=SECTOR_SIZE_512)
+
+    ls = sanlock.read_lockspace(
+        path, offset=offset, align=ALIGNMENT_1M, sector=SECTOR_SIZE_512)
+    assert ls == {"iotimeout": 1, "lockspace": "name"}
+
     acquired = sanlock.inq_lockspace(
         "name", 1, path, offset=offset, wait=False)
     assert acquired is False
@@ -64,6 +77,7 @@ def test_write_resource(tmpdir, sanlock_daemon, size, offset):
     util.create_file(path, size)
     disks = [(path, offset)]
 
+    # test read and write with default alignment nad sector size values
     sanlock.write_resource("ls_name", "res_name", disks)
 
     res = sanlock.read_resource(path, offset=offset)
@@ -73,6 +87,19 @@ def test_write_resource(tmpdir, sanlock_daemon, size, offset):
         "version": 0
     }
 
+    # test read and write with explicit alignment nad sector size values
+    sanlock.write_resource(
+        "ls_name", "res_name", disks, align=ALIGNMENT_1M,
+        sector=SECTOR_SIZE_512)
+
+    res = sanlock.read_resource(
+        path, offset=offset, align=ALIGNMENT_1M, sector=SECTOR_SIZE_512)
+    assert res == {
+        "lockspace": "ls_name",
+        "resource": "res_name",
+        "version": 0
+    }
+
     owners = sanlock.read_resource_owners("ls_name", "res_name",
disks)
     assert owners == []
 
@@ -225,3 +252,35 @@ def test_acquire_release_resource(tmpdir, sanlock_daemon, size,
offset):
 
     owners = sanlock.read_resource_owners("ls_name", "res_name",
disks)
     assert owners == []
+
+
+(a)pytest.mark.parametrize("align, sector", [
+    # Invalid alignment
+    (1024, 512),
+    # Invalid sector size
+    (1048576, 8192),
+])
+def test_write_lockspace_invalid_align_sector(
+        tmpdir, sanlock_daemon, align, sector):
+    path = str(tmpdir.join("lockspace"))
+    util.create_file(path, LOCKSPACE_SIZE)
+
+    with pytest.raises(ValueError):
+        sanlock.write_lockspace("name", path, align=align, sector=sector)
+
+
+(a)pytest.mark.parametrize("align, sector", [
+    # Invalid alignment
+    (1024, 512),
+    # Invalid sector size
+    (1048576, 8192),
+])
+def test_write_resource_invalid_align_sector(
+        tmpdir, sanlock_daemon, align, sector):
+    path = str(tmpdir.join("resources"))
+    util.create_file(path, MIN_RES_SIZE)
+    disks = [(path, 0)]
+
+    with pytest.raises(ValueError):
+        sanlock.write_resource(
+            "ls_name", "res_name", disks, align=align,
sector=sector)
-- 
2.20.1

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Vojtech Juranek]

Previous patch switched from accepting align and sector flags to values.
Exposing internal sanlock flags doesn't make sense now. Tuples with
supported sector size and alignment values should be exposed instead.
---
 python/sanlock.c     | 21 ++++++++++++++-------
 tests/python_test.py |  8 ++++----
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index d5bff8e..7c4502b 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1717,12 +1717,19 @@ initsanlock(void)
     PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT, 
"SETEV_REPLACE_EVENT");
     PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,      "SETEV_ALL_HOSTS");
 
-    /* Sector and align size flags */
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512");
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K");
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
-    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
 #undef PYSNLK_INIT_ADD_CONSTANT
+
+    /* Tuples with supported sector size and alignment values */
+    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512, SECTOR_SIZE_4K);
+    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
+        Py_DECREF(sector);
+        return;
+    }
+      
+    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M, ALIGNMENT_2M,
ALIGNMENT_4M, ALIGNMENT_8M);
+    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align)) {
+        Py_DECREF(align);
+	return;
+    }
+
 }
diff --git a/tests/python_test.py b/tests/python_test.py
index 2e5963f..5ca7a96 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir, sanlock_daemon, size,
offset):
 
 @pytest.mark.parametrize("align, sector", [
     # Invalid alignment
-    (1024, 512),
+    (1024, sanlock.SECTOR_SIZE[0]),
     # Invalid sector size
-    (1048576, 8192),
+    (sanlock.ALIGN_SIZE[0], 8192),
 ])
 def test_write_lockspace_invalid_align_sector(
         tmpdir, sanlock_daemon, align, sector):
@@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
 
 @pytest.mark.parametrize("align, sector", [
     # Invalid alignment
-    (1024, 512),
+    (1024, sanlock.SECTOR_SIZE[0]),
     # Invalid sector size
-    (1048576, 8192),
+    (sanlock.ALIGN_SIZE[0], 8192),
 ])
 def test_write_resource_invalid_align_sector(
         tmpdir, sanlock_daemon, align, sector):
-- 
2.20.1

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Nir Soffer]

On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote:

>  Previous patch switched from accepting align and sector flags to
> values.
>  Exposing internal sanlock flags doesn't make sense now. Tuples with
>  supported sector size and alignment values should be exposed instead.
>  ---
>   python/sanlock.c     | 21 ++++++++++++++-------
>   tests/python_test.py |  8 ++++----
>   2 files changed, 18 insertions(+), 11 deletions(-)
>
>  diff --git a/python/sanlock.c b/python/sanlock.c
>  index d5bff8e..7c4502b 100644
>  --- a/python/sanlock.c
>  +++ b/python/sanlock.c
>  @@ -1717,12 +1717,19 @@ initsanlock(void)
>       PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
>  "SETEV_REPLACE_EVENT");
>       PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
>  "SETEV_ALL_HOSTS");
>
>  -    /* Sector and align size flags */
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512");
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K");
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
>  -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
>   #undef PYSNLK_INIT_ADD_CONSTANT
>  +
>  +    /* Tuples with supported sector size and alignment values */
>  +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
>  SECTOR_SIZE_4K);
>  
Py_BuildValue can fail and return NULL. The import will probably fail with
MemoryError. This is unlikely
but if this happen we will probably segfault in the next line instead of
failing with clear exception.


>  +    if (PyModule_AddObject(py_module, "SECTOR_SIZE",
> sector)) {
>  +        Py_DECREF(sector);
>  +        return;
>  +    }
>  +
>  +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M, ALIGNMENT_2M,
>  ALIGNMENT_4M, ALIGNMENT_8M);
>  +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align)) {
>  +        Py_DECREF(align);
>  +       return;
>  +    }
>  +
>   }
>  diff --git a/tests/python_test.py b/tests/python_test.py
>  index 2e5963f..5ca7a96 100644
>  --- a/tests/python_test.py
>  +++ b/tests/python_test.py
>  @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
>  sanlock_daemon, size, offset):
>
>   @pytest.mark.parametrize("align, sector", [
>       # Invalid alignment
>  -    (1024, 512),
>  +    (1024, sanlock.SECTOR_SIZE[0]),
>       # Invalid sector size
>  -    (1048576, 8192),
>  +    (sanlock.ALIGN_SIZE[0], 8192),
>   ])
>   def test_write_lockspace_invalid_align_sector(
>           tmpdir, sanlock_daemon, align, sector):
>  @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
>
>   @pytest.mark.parametrize("align, sector", [
>       # Invalid alignment
>  -    (1024, 512),
>  +    (1024, sanlock.SECTOR_SIZE[0]),
>       # Invalid sector size
>  -    (1048576, 8192),
>  +    (sanlock.ALIGN_SIZE[0], 8192),
>   ])
>   def test_write_resource_invalid_align_sector(
>           tmpdir, sanlock_daemon, align, sector):
>  --
>  2.20.1
>
>  
Otherwise looks fine.

Nir

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Amit Bawer]

On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:

>  On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek
> <vjuranek(a)redhat.com&gt;
>  wrote:
>
> > Previous patch switched from accepting align and sector flags to values.
> > Exposing internal sanlock flags doesn't make sense now. Tuples with
> > supported sector size and alignment values should be exposed instead.
> > ---
> >  python/sanlock.c     | 21 ++++++++++++++-------
> >  tests/python_test.py |  8 ++++----
> >  2 files changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/python/sanlock.c b/python/sanlock.c
> > index d5bff8e..7c4502b 100644
> > --- a/python/sanlock.c
> > +++ b/python/sanlock.c
> > @@ -1717,12 +1717,19 @@ initsanlock(void)
> >      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
> > "SETEV_REPLACE_EVENT");
> >      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
> > "SETEV_ALL_HOSTS");
> >
> > -    /* Sector and align size flags */
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512");
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K");
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
> > -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
> >  #undef PYSNLK_INIT_ADD_CONSTANT
> > +
> > +    /* Tuples with supported sector size and alignment values */
> > +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
> > SECTOR_SIZE_4K);
> >
>
>  Py_BuildValue can fail and return NULL. The import will probably fail with
>  MemoryError. This is unlikely
>  but if this happen we will probably segfault in the next line instead of
>  failing with clear exception.
>  
Not sure it will segfault since the last arg in PyModule_AddObject is
checked for validity and PyError is set:
https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799...
However IMHO i would use  Py_XDECREF because i like it better and it checks
for a NULL pointer first.

> > +    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
> > +        Py_DECREF(sector);
> > +        return;
> > +    }
> > +
> > +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M, ALIGNMENT_2M,
> > ALIGNMENT_4M, ALIGNMENT_8M);
> > +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align)) {
> > +        Py_DECREF(align);
> > +       return;
> > +    }
> > +
> >  }
> > diff --git a/tests/python_test.py b/tests/python_test.py
> > index 2e5963f..5ca7a96 100644
> > --- a/tests/python_test.py
> > +++ b/tests/python_test.py
> > @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
> > sanlock_daemon, size, offset):
> >
> >  @pytest.mark.parametrize("align, sector", [
> >      # Invalid alignment
> > -    (1024, 512),
> > +    (1024, sanlock.SECTOR_SIZE[0]),
> >      # Invalid sector size
> > -    (1048576, 8192),
> > +    (sanlock.ALIGN_SIZE[0], 8192),
> >  ])
> >  def test_write_lockspace_invalid_align_sector(
> >          tmpdir, sanlock_daemon, align, sector):
> > @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
> >
> >  @pytest.mark.parametrize("align, sector", [
> >      # Invalid alignment
> > -    (1024, 512),
> > +    (1024, sanlock.SECTOR_SIZE[0]),
> >      # Invalid sector size
> > -    (1048576, 8192),
> > +    (sanlock.ALIGN_SIZE[0], 8192),
> >  ])
> >  def test_write_resource_invalid_align_sector(
> >          tmpdir, sanlock_daemon, align, sector):
> > --
> > 2.20.1
> >
> >
>  Otherwise looks fine.
>
>  Nir
>

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Nir Soffer]

On Friday, May 3, 2019, Amit Bawer <abawer(a)redhat.com&gt; wrote:
>  On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
> >
> > On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt;
wrote:
> >>
> >> Previous patch switched from accepting align and sector flags to values.
> >> Exposing internal sanlock flags doesn't make sense now. Tuples with
> >> supported sector size and alignment values should be exposed instead.
> >> ---
> >>  python/sanlock.c     | 21 ++++++++++++++-------
> >>  tests/python_test.py |  8 ++++----
> >>  2 files changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/python/sanlock.c b/python/sanlock.c
> >> index d5bff8e..7c4502b 100644
> >> --- a/python/sanlock.c
> >> +++ b/python/sanlock.c
> >> @@ -1717,12 +1717,19 @@ initsanlock(void)
> >>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
"SETEV_REPLACE_EVENT");
> >>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
"SETEV_ALL_HOSTS");
> >>
> >> -    /* Sector and align size flags */
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512");
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K");
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
> >> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
> >>  #undef PYSNLK_INIT_ADD_CONSTANT
> >> +
> >> +    /* Tuples with supported sector size and alignment values */
> >> +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
SECTOR_SIZE_4K);
> >
> > Py_BuildValue can fail and return NULL. The import will probably fail 
with
MemoryError. This is unlikely
> > but if this happen we will probably segfault in the next line
> instead of 
failing with clear exception.
>  Not sure it will segfault since the last arg in PyModule_AddObject is 
checked for
validity and PyError is set:
https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799...

Is this documented?

Even if it is documented, there is only one thing you should do after a
failure - retun.

Calling another function based on current implementation detail and
handling the next failure is sloppy and will make it harder to debug
issues, as the original MemoryError will be hidden by a later error.

>  However IMHO i would use  Py_XDECREF because i like it better and it
checks for a NULL pointer first.

This is another sloppy usage. This macro is good for the case when object
is expected to be NULL during cleanup. Here the object is not expected to
be NULL if we check the return value properly.

> >
> >
> >>
> >> +    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
> >> +        Py_DECREF(sector);
> >> +        return;
> >> +    }
> >> +
> >> +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M,
ALIGNMENT_2M, ALIGNMENT_4M, ALIGNMENT_8M);
> >> +    if (PyModule_AddObject(py_module,
> "ALIGN_SIZE", align)) {
> >> +        Py_DECREF(align);
> >> +       return;
> >> +    }
> >> +
> >>  }
> >> diff --git a/tests/python_test.py b/tests/python_test.py
> >> index 2e5963f..5ca7a96 100644
> >> --- a/tests/python_test.py
> >> +++ b/tests/python_test.py
> >> @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
sanlock_daemon, size, offset):
> >>
> >>  @pytest.mark.parametrize("align, sector", [
> >>      # Invalid alignment
> >> -    (1024, 512),
> >> +    (1024, sanlock.SECTOR_SIZE[0]),
> >>      # Invalid sector size
> >> -    (1048576, 8192),
> >> +    (sanlock.ALIGN_SIZE[0], 8192),
> >>  ])
> >>  def test_write_lockspace_invalid_align_sector(
> >>          tmpdir, sanlock_daemon, align, sector):
> >> @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
> >>
> >>  @pytest.mark.parametrize("align, sector", [
> >>      # Invalid alignment
> >> -    (1024, 512),
> >> +    (1024, sanlock.SECTOR_SIZE[0]),
> >>      # Invalid sector size
> >> -    (1048576, 8192),
> >> +    (sanlock.ALIGN_SIZE[0], 8192),
> >>  ])
> >>  def test_write_resource_invalid_align_sector(
> >>          tmpdir, sanlock_daemon, align, sector):
> >> --
> >> 2.20.1
> >>
> >
> > Otherwise looks fine.
> > Nir

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Vojtech Juranek]

On pátek 3. května 2019 14:10:31 CEST Nir Soffer wrote:
>  On Friday, May 3, 2019, Amit Bawer <abawer(a)redhat.com&gt; wrote:
>  > On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
>  >> On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt;
>  
>  wrote:
>  >>> Previous patch switched from accepting align and sector flags to values.
>  >>> Exposing internal sanlock flags doesn't make sense now. Tuples with
>  >>> supported sector size and alignment values should be exposed instead.
>  >>> ---
>  >>> 
>  >>>  python/sanlock.c     | 21 ++++++++++++++-------
>  >>>  tests/python_test.py |  8 ++++----
>  >>>  2 files changed, 18 insertions(+), 11 deletions(-)
>  >>> 
>  >>> diff --git a/python/sanlock.c b/python/sanlock.c
>  >>> index d5bff8e..7c4502b 100644
>  >>> --- a/python/sanlock.c
>  >>> +++ b/python/sanlock.c
>  >>> @@ -1717,12 +1717,19 @@ initsanlock(void)
>  >>> 
>  >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
>  
>  "SETEV_REPLACE_EVENT");
>  
>  >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
>  
>  "SETEV_ALL_HOSTS");
>  
>  >>> -    /* Sector and align size flags */
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512, "SECTOR512");
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K, "SECTOR4K");
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
>  >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
>  >>> 
>  >>>  #undef PYSNLK_INIT_ADD_CONSTANT
>  >>> 
>  >>> +
>  >>> +    /* Tuples with supported sector size and alignment values */
>  >>> +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
>  
>  SECTOR_SIZE_4K);
>  
>  >> Py_BuildValue can fail and return NULL. The import will probably fail
>  
>  with MemoryError. This is unlikely
>  
>  >> but if this happen we will probably segfault in the next line instead of
>  
>  failing with clear exception.
>  
>  > Not sure it will segfault since the last arg in PyModule_AddObject is
>  
>  checked for validity and PyError is set:
>  https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799296
>  f7e/Python/modsupport.c#L638
>  
>  Is this documented?
>  
>  Even if it is documented, there is only one thing you should do after a
>  failure - retun. 
make sense for me, calling PyModule_AddObject with NULL will just tell that 
sector is NULL without any other hint why. So can we all agree on this?

    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512, SECTOR_SIZE_4K);
    if (!sector)
        return;
    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
        Py_DECREF(sector);
        return;
    }


>  Calling another function based on current implementation detail and
>  handling the next failure is sloppy and will make it harder to debug
>  issues, as the original MemoryError will be hidden by a later error.
>  
>  > However IMHO i would use  Py_XDECREF because i like it better and it
>  
>  checks for a NULL pointer first.
>  
>  This is another sloppy usage. This macro is good for the case when object
>  is expected to be NULL during cleanup. Here the object is not expected to
>  be NULL if we check the return value properly.
>  
>  >>> +    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
>  >>> +        Py_DECREF(sector);
>  >>> +        return;
>  >>> +    }
>  >>> +
>  >>> +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M,
>  
>  ALIGNMENT_2M, ALIGNMENT_4M, ALIGNMENT_8M);
>  
>  >>> +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align)) {
>  >>> +        Py_DECREF(align);
>  >>> +       return;
>  >>> +    }
>  >>> +
>  >>> 
>  >>>  }
>  >>> 
>  >>> diff --git a/tests/python_test.py b/tests/python_test.py
>  >>> index 2e5963f..5ca7a96 100644
>  >>> --- a/tests/python_test.py
>  >>> +++ b/tests/python_test.py
>  >>> @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
>  
>  sanlock_daemon, size, offset):
>  >>>  @pytest.mark.parametrize("align, sector", [
>  >>>  
>  >>>      # Invalid alignment
>  >>> 
>  >>> -    (1024, 512),
>  >>> +    (1024, sanlock.SECTOR_SIZE[0]),
>  >>> 
>  >>>      # Invalid sector size
>  >>> 
>  >>> -    (1048576, 8192),
>  >>> +    (sanlock.ALIGN_SIZE[0], 8192),
>  >>> 
>  >>>  ])
>  >>>  def test_write_lockspace_invalid_align_sector(
>  >>>  
>  >>>          tmpdir, sanlock_daemon, align, sector):
>  >>> @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
>  >>> 
>  >>>  @pytest.mark.parametrize("align, sector", [
>  >>>  
>  >>>      # Invalid alignment
>  >>> 
>  >>> -    (1024, 512),
>  >>> +    (1024, sanlock.SECTOR_SIZE[0]),
>  >>> 
>  >>>      # Invalid sector size
>  >>> 
>  >>> -    (1048576, 8192),
>  >>> +    (sanlock.ALIGN_SIZE[0], 8192),
>  >>> 
>  >>>  ])
>  >>>  def test_write_resource_invalid_align_sector(
>  >>>  
>  >>>          tmpdir, sanlock_daemon, align, sector):
>  >>> --
>  >>> 2.20.1
>  >> 
>  >> Otherwise looks fine.
>  >> Nir

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Nir Soffer]

On Friday, May 3, 2019, Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote:
>  On pátek 3. května 2019 14:10:31 CEST Nir Soffer wrote:
> > On Friday, May 3, 2019, Amit Bawer <abawer(a)redhat.com&gt; wrote:
> > > On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
> > >> On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt;
> >
> > wrote:
> > >>> Previous patch switched from accepting align and sector flags to
values.
> > >>> Exposing internal sanlock flags doesn't make
> sense now. Tuples with
> > >>> supported sector size and alignment values should be exposed instead.
> > >>> ---
> > >>>
> > >>>  python/sanlock.c     | 21 ++++++++++++++-------
> > >>>  tests/python_test.py |  8 ++++----
> > >>>  2 files changed, 18 insertions(+), 11 deletions(-)
> > >>>
> > >>> diff --git a/python/sanlock.c b/python/sanlock.c
> > >>> index d5bff8e..7c4502b 100644
> > >>> --- a/python/sanlock.c
> > >>> +++ b/python/sanlock.c
> > >>> @@ -1717,12 +1717,19 @@ initsanlock(void)
> > >>>
> > >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
> >
> > "SETEV_REPLACE_EVENT");
> >
> > >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
> >
> > "SETEV_ALL_HOSTS");
> >
> > >>> -    /* Sector and align size flags */
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512,
> "SECTOR512");
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K,
> "SECTOR4K");
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
> > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
> > >>>
> > >>>  #undef PYSNLK_INIT_ADD_CONSTANT
> > >>>
> > >>> +
> > >>> +    /* Tuples with supported sector size and alignment values */
> > >>> +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
> >
> > SECTOR_SIZE_4K);
> >
> > >> Py_BuildValue can fail and return NULL. The import will probably fail
> >
> > with MemoryError. This is unlikely
> >
> > >> but if this happen we will probably segfault in the next line instead
of
> >
> > failing with clear exception.
> >
> > > Not sure it will segfault since the last arg in PyModule_AddObject is
> >
> > checked for validity and PyError is set:
> > 
https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799296
> > f7e/Python/modsupport.c#L638
> >
> > Is this documented?
> >
> > Even if it is documented, there is only one thing you should do after a
> > failure - retun.
>
>  make sense for me, calling PyModule_AddObject with NULL will just tell 
that
>  sector is NULL without any other hint why. So can we all agree on
> this?
>
>      PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
SECTOR_SIZE_4K);
>      if (!sector)
>          return;
>      if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
>          Py_DECREF(sector);
>          return;
>      } 
+1

> > Calling another function based on current implementation detail and
> > handling the next failure is sloppy and will make it harder to debug
> > issues, as the original MemoryError will be hidden by a later error.
> >
> > > However IMHO i would use  Py_XDECREF because i like it better and it
> >
> > checks for a NULL pointer first.
> >
> > This is another sloppy usage. This macro is good for the case when object
> > is expected to be NULL during cleanup. Here the object is not expected to
> > be NULL if we check the return value properly.
> >
> > >>> +    if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector))
> {
> > >>> +        Py_DECREF(sector);
> > >>> +        return;
> > >>> +    }
> > >>> +
> > >>> +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M,
> >
> > ALIGNMENT_2M, ALIGNMENT_4M, ALIGNMENT_8M);
> >
> > >>> +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align))
> {
> > >>> +        Py_DECREF(align);
> > >>> +       return;
> > >>> +    }
> > >>> +
> > >>>
> > >>>  }
> > >>>
> > >>> diff --git a/tests/python_test.py b/tests/python_test.py
> > >>> index 2e5963f..5ca7a96 100644
> > >>> --- a/tests/python_test.py
> > >>> +++ b/tests/python_test.py
> > >>> @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
> >
> > sanlock_daemon, size, offset):
> > >>>  @pytest.mark.parametrize("align, sector", [
> > >>>
> > >>>      # Invalid alignment
> > >>>
> > >>> -    (1024, 512),
> > >>> +    (1024, sanlock.SECTOR_SIZE[0]),
> > >>>
> > >>>      # Invalid sector size
> > >>>
> > >>> -    (1048576, 8192),
> > >>> +    (sanlock.ALIGN_SIZE[0], 8192),
> > >>>
> > >>>  ])
> > >>>  def test_write_lockspace_invalid_align_sector(
> > >>>
> > >>>          tmpdir, sanlock_daemon, align, sector):
> > >>> @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
> > >>>
> > >>>  @pytest.mark.parametrize("align, sector", [
> > >>>
> > >>>      # Invalid alignment
> > >>>
> > >>> -    (1024, 512),
> > >>> +    (1024, sanlock.SECTOR_SIZE[0]),
> > >>>
> > >>>      # Invalid sector size
> > >>>
> > >>> -    (1048576, 8192),
> > >>> +    (sanlock.ALIGN_SIZE[0], 8192),
> > >>>
> > >>>  ])
> > >>>  def test_write_resource_invalid_align_sector(
> > >>>
> > >>>          tmpdir, sanlock_daemon, align, sector):
> > >>> --
> > >>> 2.20.1
> > >>
> > >> Otherwise looks fine.
> > >> Nir
>
>

[PATCH v4 2/2] Expose tuples with supported align and sector values

[Amit Bawer]

+1

On Fri, May 3, 2019 at 3:49 PM Vojtech Juranek <vjuranek(a)redhat.com&gt; wrote:

>  On pátek 3. května 2019 14:10:31 CEST Nir Soffer wrote:
>  > On Friday, May 3, 2019, Amit Bawer <abawer(a)redhat.com&gt; wrote:
>  > > On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer(a)redhat.com&gt; wrote:
>  > >> On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek(a)redhat.com&gt;
>  >
>  > wrote:
>  > >>> Previous patch switched from accepting align and sector flags to
>  values.
>  > >>> Exposing internal sanlock flags doesn't make sense now. Tuples
> with
>  > >>> supported sector size and alignment values should be exposed instead.
>  > >>> ---
>  > >>>
>  > >>>  python/sanlock.c     | 21 ++++++++++++++-------
>  > >>>  tests/python_test.py |  8 ++++----
>  > >>>  2 files changed, 18 insertions(+), 11 deletions(-)
>  > >>>
>  > >>> diff --git a/python/sanlock.c b/python/sanlock.c
>  > >>> index d5bff8e..7c4502b 100644
>  > >>> --- a/python/sanlock.c
>  > >>> +++ b/python/sanlock.c
>  > >>> @@ -1717,12 +1717,19 @@ initsanlock(void)
>  > >>>
>  > >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_REPLACE_EVENT,
>  >
>  > "SETEV_REPLACE_EVENT");
>  >
>  > >>>      PYSNLK_INIT_ADD_CONSTANT(SANLK_SETEV_ALL_HOSTS,
>  >
>  > "SETEV_ALL_HOSTS");
>  >
>  > >>> -    /* Sector and align size flags */
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR512,
> "SECTOR512");
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_SECTOR4K,
> "SECTOR4K");
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN1M, "ALIGN1M");
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN2M, "ALIGN2M");
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN4M, "ALIGN4M");
>  > >>> -    PYSNLK_INIT_ADD_CONSTANT(SANLK_RES_ALIGN8M, "ALIGN8M");
>  > >>>
>  > >>>  #undef PYSNLK_INIT_ADD_CONSTANT
>  > >>>
>  > >>> +
>  > >>> +    /* Tuples with supported sector size and alignment values */
>  > >>> +    PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
>  >
>  > SECTOR_SIZE_4K);
>  >
>  > >> Py_BuildValue can fail and return NULL. The import will probably fail
>  >
>  > with MemoryError. This is unlikely
>  >
>  > >> but if this happen we will probably segfault in the next line instead
>  of
>  >
>  > failing with clear exception.
>  >
>  > > Not sure it will segfault since the last arg in PyModule_AddObject is
>  >
>  > checked for validity and PyError is set:
>  >
>  https://github.com/python/cpython/blob/62be33870e2f8517314bf9c7275548e799296
>  > f7e/Python/modsupport.c#L638
>  >
>  > Is this documented?
>  >
>  > Even if it is documented, there is only one thing you should do after a
>  > failure - retun.
>
>  make sense for me, calling PyModule_AddObject with NULL will just tell
>  that
>  sector is NULL without any other hint why. So can we all agree on this?
>
>      PyObject *sector = Py_BuildValue("ii", SECTOR_SIZE_512,
>  SECTOR_SIZE_4K);
>      if (!sector)
>          return;
>      if (PyModule_AddObject(py_module, "SECTOR_SIZE", sector)) {
>          Py_DECREF(sector);
>          return;
>      }
>
>
>  > Calling another function based on current implementation detail and
>  > handling the next failure is sloppy and will make it harder to debug
>  > issues, as the original MemoryError will be hidden by a later error.
>  >
>  > > However IMHO i would use  Py_XDECREF because i like it better and it
>  >
>  > checks for a NULL pointer first.
>  >
>  > This is another sloppy usage. This macro is good for the case when object
>  > is expected to be NULL during cleanup. Here the object is not expected to
>  > be NULL if we check the return value properly.
>  >
>  > >>> +    if (PyModule_AddObject(py_module, "SECTOR_SIZE",
> sector)) {
>  > >>> +        Py_DECREF(sector);
>  > >>> +        return;
>  > >>> +    }
>  > >>> +
>  > >>> +    PyObject *align = Py_BuildValue("llll", ALIGNMENT_1M,
>  >
>  > ALIGNMENT_2M, ALIGNMENT_4M, ALIGNMENT_8M);
>  >
>  > >>> +    if (PyModule_AddObject(py_module, "ALIGN_SIZE", align))
> {
>  > >>> +        Py_DECREF(align);
>  > >>> +       return;
>  > >>> +    }
>  > >>> +
>  > >>>
>  > >>>  }
>  > >>>
>  > >>> diff --git a/tests/python_test.py b/tests/python_test.py
>  > >>> index 2e5963f..5ca7a96 100644
>  > >>> --- a/tests/python_test.py
>  > >>> +++ b/tests/python_test.py
>  > >>> @@ -256,9 +256,9 @@ def test_acquire_release_resource(tmpdir,
>  >
>  > sanlock_daemon, size, offset):
>  > >>>  @pytest.mark.parametrize("align, sector", [
>  > >>>
>  > >>>      # Invalid alignment
>  > >>>
>  > >>> -    (1024, 512),
>  > >>> +    (1024, sanlock.SECTOR_SIZE[0]),
>  > >>>
>  > >>>      # Invalid sector size
>  > >>>
>  > >>> -    (1048576, 8192),
>  > >>> +    (sanlock.ALIGN_SIZE[0], 8192),
>  > >>>
>  > >>>  ])
>  > >>>  def test_write_lockspace_invalid_align_sector(
>  > >>>
>  > >>>          tmpdir, sanlock_daemon, align, sector):
>  > >>> @@ -271,9 +271,9 @@ def test_write_lockspace_invalid_align_sector(
>  > >>>
>  > >>>  @pytest.mark.parametrize("align, sector", [
>  > >>>
>  > >>>      # Invalid alignment
>  > >>>
>  > >>> -    (1024, 512),
>  > >>> +    (1024, sanlock.SECTOR_SIZE[0]),
>  > >>>
>  > >>>      # Invalid sector size
>  > >>>
>  > >>> -    (1048576, 8192),
>  > >>> +    (sanlock.ALIGN_SIZE[0], 8192),
>  > >>>
>  > >>>  ])
>  > >>>  def test_write_resource_invalid_align_sector(
>  > >>>
>  > >>>          tmpdir, sanlock_daemon, align, sector):
>  > >>> --
>  > >>> 2.20.1
>  > >>
>  > >> Otherwise looks fine.
>  > >> Nir
>
>