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(a)redhat.com> wrote:
On Wed, Jun 12, 2019 at 1:13 AM Nir Soffer <nirsof(a)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(a)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()".
>
>
> @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
>
>