the original check here is for rv != 0. and cleanup code checks the same.
if we want to change the rv checks for sanlock calls we probably should to that in a separate patch, not related to py3 changes anyway.

On Mon, Jun 10, 2019 at 3:41 PM Pavel Bar <pbar@redhat.com> wrote:


On Mon, Jun 3, 2019 at 11:31 PM Nir Soffer <nirsof@gmail.com> wrote:
From: Amit Bawer <abawer@redhat.com>

Implications:

- Py2 : no difference
- Py3 : if used, only accept bytes for lockspace/resource name.
---
 python/sanlock.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index c7518c6..7bdbd69 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1028,39 +1028,39 @@ mode. The version is the version of the lease that must be acquired or fail.\n\
 The disks must be in the format: [(path, offset), ... ]\n");

 static PyObject *
 py_acquire(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv, sanlockfd = -1, pid = -1, shared = 0;
-    const char *lockspace, *resource;
-    struct sanlk_resource *res;
+    int rv = -1, sanlockfd = -1, pid = -1, shared = 0;
+    PyObject *lockspace = NULL, *resource = NULL;
+    struct sanlk_resource *res = NULL;
     PyObject *disks, *version = Py_None;

     static char *kwlist[] = {"lockspace", "resource", "disks", "slkfd",
                                 "pid", "shared", "version", NULL};

     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiiO", kwlist,
-        &lockspace, &resource, &PyList_Type, &disks, &sanlockfd, &pid,
-        &shared, &version)) {
-        return NULL;
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|iiiO", kwlist,
+        convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
+        &PyList_Type, &disks, &sanlockfd, &pid, &shared, &version)) {
+        goto finally;
     }

     /* check if any of the slkfd or pid parameters was given */
     if (sanlockfd == -1 && pid == -1) {
         __set_exception(EINVAL, "Invalid slkfd and pid values");
-        return NULL;
+        goto finally;
     }

     /* parse and check sanlock resource */
     if (__parse_resource(disks, &res) < 0) {
-        return NULL;
+        goto finally;
     }

     /* prepare sanlock names */
-    strncpy(res->lockspace_name, lockspace, SANLK_NAME_LEN);
-    strncpy(res->name, resource, SANLK_NAME_LEN);
+    strncpy(res->lockspace_name, PyBytes_AsString(lockspace), SANLK_NAME_LEN);
+    strncpy(res->name, PyBytes_AsString(resource), SANLK_NAME_LEN);

     /* prepare sanlock flags */
     if (shared) {
         res->flags |= SANLK_RES_SHARED;
     }
@@ -1069,30 +1069,31 @@ py_acquire(PyObject *self __unused, PyObject *args, PyObject *keywds)
     if (version != Py_None) {
         res->flags |= SANLK_RES_LVER;
         res->lver = pyinteger_as_unsigned_long_long_mask(version);
         if (res->lver == (uint64_t)-1) {
             __set_exception(EINVAL, "Unable to convert the version value");
-            goto exit_fail;
+            goto finally;
         }
     }

     /* acquire sanlock resource (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
     rv = sanlock_acquire(sanlockfd, pid, 0, 1, &res, 0);
     Py_END_ALLOW_THREADS

     if (rv != 0) {
         __set_exception(rv, "Sanlock resource not acquired");
-        goto exit_fail;
+        goto finally;
     }

+finally:
+    Py_XDECREF(lockspace);
+    Py_XDECREF(resource);
     free(res);
+    if (rv != 0)

Is it intentional "rv != 0" instead of "rv < 0"? I do see "rv != 0" in the existing code a few lines above in the method, but wasn't it a mistake? If the convention is that any non-negative is a success, then it should be changed.
 
+        return NULL;
     Py_RETURN_NONE;
-
-exit_fail:
-    free(res);
-    return NULL;
 }

 /* release */
 PyDoc_STRVAR(pydoc_release, "\
 release(lockspace, resource, disks [, slkfd=fd, pid=owner])\n\
--
2.17.2