On Wed, Jun 12, 2019 at 12:27 PM Amit Bawer <abawer@redhat.com> wrote:
Even if its accidental, those tests are stubs, so it has no effect other than checking that argument parsing works as expected.

On Wed, Jun 12, 2019 at 11:11 AM Pavel Bar <pbar@redhat.com> wrote:


On Wed, Jun 12, 2019 at 1:13 AM Nir Soffer <nirsof@gmail.com> wrote:
The async argument to add_lockspace() and rem_lockspace() is not valid
syntax in python 3.7, since async is a keyword now.

Replace async=False with wait=True, already used in other sanlock
methods.

This change breaks compatibility with existing python 2 code, but we
plan to ship sanlock 3.8 with python 3 support only on Fedora 30 and
EL 8.1 which do not support python 2, so this does not break existing
users.

Python code that wants to support both sanlock 3.7 and 3.8 will have to
change the behaviour based on python version.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---

 python/sanlock.c     | 26 ++++++++++++++------------
 tests/python_test.py |  8 ++++----
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index d122c8e..7f0916a 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -799,40 +799,41 @@ finally:
     Py_RETURN_NONE;
 }

 /* add_lockspace */
 PyDoc_STRVAR(pydoc_add_lockspace, "\
-add_lockspace(lockspace, host_id, path, offset=0, iotimeout=0, async=False)\n\
-Add a lockspace, acquiring a host_id in it. If async is True the function\n\
+add_lockspace(lockspace, host_id, path, offset=0, iotimeout=0, wait=True)\n\
+Add a lockspace, acquiring a host_id in it. If wait is False the function\n\
 will return immediatly and the status can be checked using inq_lockspace.\n\
 The iotimeout option configures the io timeout for the specific lockspace,\n\
 overriding the default value (see the sanlock daemon parameter -o).");

 static PyObject *
 py_add_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv = -1, async = 0, flags = 0;
+    int rv = -1, flags = 0;
+    int wait = 1;
     uint32_t iotimeout = 0;
     PyObject *lockspace = NULL;
     PyObject *path = NULL;
     struct sanlk_lockspace ls;

     static char *kwlist[] = {"lockspace", "host_id", "path", "offset",
-                                "iotimeout", "async", NULL};
+                                "iotimeout", "wait", NULL};

     /* initialize lockspace structure */
     memset(&ls, 0, sizeof(struct sanlk_lockspace));

     /* parse python tuple */
     if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&kO&|kIi", kwlist,
         convert_to_pybytes, &lockspace, &ls.host_id, pypath_converter, &path,
-        &ls.host_id_disk.offset, &iotimeout, &async)) {
+        &ls.host_id_disk.offset, &iotimeout, &wait)) {
         goto finally;
     }

     /* prepare sanlock_add_lockspace flags */
-    if (async) {
+    if (!wait) {
         flags |= SANLK_ADD_ASYNC;
     }

     /* prepare sanlock names */
     strncpy(ls.name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
@@ -918,46 +919,47 @@ finally:
     return NULL;
 }

 /* rem_lockspace */
 PyDoc_STRVAR(pydoc_rem_lockspace, "\
-rem_lockspace(lockspace, host_id, path, offset=0, async=False, unused=False)\n\
-Remove a lockspace, releasing the acquired host_id. If async is True the\n\
+rem_lockspace(lockspace, host_id, path, offset=0, wait=True, unused=False)\n\
+Remove a lockspace, releasing the acquired host_id. If wait is False the\n\
 function will return immediately and the status can be checked using\n\
 inq_lockspace. If unused is True the command will fail (EBUSY) if there is\n\
 at least one acquired resource in the lockspace. Otherwise (the default)\n\
 sanlock will try to terminate processes holding resource leases and upon\n\
 successful termination these leases will be released.");

 static PyObject *
 py_rem_lockspace(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv = -1, async = 0, unused = 0, flags = 0;
+    int rv = -1, unused = 0, flags = 0;
+    int wait = 1;
     PyObject *lockspace = NULL;
     PyObject *path = NULL;
     struct sanlk_lockspace ls;

     static char *kwlist[] = {"lockspace", "host_id", "path", "offset",
-                                "async", "unused", NULL};
+                                "wait", "unused", NULL};

     /* initialize lockspace structure */
     memset(&ls, 0, sizeof(struct sanlk_lockspace));

     /* parse python tuple */
     if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&kO&|kii", kwlist,
         convert_to_pybytes, &lockspace, &ls.host_id, pypath_converter, &path,
         &ls.host_id_disk.offset,
-        &async, &unused)) {
+        &wait, &unused)) {
         goto finally;
     }

     /* prepare sanlock names */
     strncpy(ls.name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
     strncpy(ls.host_id_disk.path, PyBytes_AsString(path), SANLK_PATH_LEN - 1);

     /* prepare sanlock_rem_lockspace flags */
-    if (async) {
+    if (!wait) {
         flags |= SANLK_REM_ASYNC;
     }

     if (unused) {
         flags |= SANLK_REM_UNUSED;
diff --git a/tests/python_test.py b/tests/python_test.py
index a2e25fb..3242f57 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -339,11 +339,11 @@ def test_add_rem_lockspace_async(tmpdir, sanlock_daemon):
     sanlock.write_lockspace(b"ls_name", path, iotimeout=1)
     acquired = sanlock.inq_lockspace(b"ls_name", 1, path, wait=False)
     assert acquired is False

     # This will take 3 seconds.
-    sanlock.add_lockspace(b"ls_name", 1, path, iotimeout=1, **{"async": True})
+    sanlock.add_lockspace(b"ls_name", 1, path, iotimeout=1, wait=False)

     # While the lockspace is being aquired, we expect to get None.
     time.sleep(1)
     acquired = sanlock.inq_lockspace(b"ls_name", 1, path, wait=False)
     assert acquired is None
@@ -351,11 +351,11 @@ def test_add_rem_lockspace_async(tmpdir, sanlock_daemon):
     # Once the lockspace is acquired, we exepect to get True.
     acquired = sanlock.inq_lockspace(b"ls_name", 1, path, wait=True)
     assert acquired is True

     # This will take about 3 seconds.
-    sanlock.rem_lockspace(b"ls_name", 1, path, **{"async": True})
+    sanlock.rem_lockspace(b"ls_name", 1, path, wait=False)

     # Wait until the lockspace change state from True to None.
     while sanlock.inq_lockspace(b"ls_name", 1, path, wait=False):
         time.sleep(1)

@@ -512,19 +512,19 @@ def raises_sanlock_errno(expected_errno=errno.ECONNREFUSED):
 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_rem_lockspace_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
-        sanlock.rem_lockspace(name, 1, path, 0)
+        sanlock.rem_lockspace(name, 1, path, 0, wait=False)

Just wanted to be sure that this is intentional:
Previously "async" was not passed and it's default value was "async=False" that now has changed to "wait=True". But now you explicitly pass "wait=False".
Is it on purpose?
The same question about the below "test_add_lockspace_parse_args()". 

Yes, using non-default value seems like a more interesting test. But since we use no_sanlock_daemon
for these tests, the value does not have any affect on the result of the test. 
 


 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_add_lockspace_parse_args(no_sanlock_daemon, name, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
-        sanlock.add_lockspace(name, 1, path, 0)
+        sanlock.add_lockspace(name, 1, path, 0, wait=False)


 @pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_write_lockspace_parse_args(no_sanlock_daemon, name, filename, encoding):
--
2.17.2