On Fri, May 3, 2019 at 12:31 AM Nir Soffer <nsoffer@redhat.com> wrote:
On Thu, May 2, 2019 at 4:03 PM Vojtech Juranek <vjuranek@redhat.com> 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/62be33870e2f8517314bf9c7275548e799296f7e/Python/modsupport.c#L638
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