On Wed, Jun 12, 2019 at 12:27 PM Amit Bawer <abawer(a)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(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()".
>
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
>>
>>