If "rv != 0", "ls_list" is NULL, so returning "ls_list" is totally the same as returning NULL explicitly. That's what the old code did.

But I'm totally for leaving the 2 lines that you added, because they do add for both readability, maintenance and stability for future changes (what if "ls_list" will be updated before we jump to "finally").

On Mon, Jun 10, 2019 at 6:59 PM Amit Bawer <abawer@redhat.com> wrote:

we check for sanlock rv error and return NULL when rv != 0, this if is not redundant at all.

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

I was talking about other 2 lines:

+    if (rv != 0)
+        return NULL;

On Mon, Jun 10, 2019 at 6:53 PM Amit Bawer <abawer@redhat.com> wrote:

that's why they are called redundant. nobody needs them.

both pointer are initialized to NULL if not allocated, so call to free is safe by ISO-C standards.

On Mon, Jun 10, 2019 at 5:29 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 name.
---
 python/sanlock.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/python/sanlock.c b/python/sanlock.c
index 0964671..ed9b4f0 100644
--- a/python/sanlock.c
+++ b/python/sanlock.c
@@ -1226,59 +1226,64 @@ Align can be one of (1048576, 2097152, 4194304, 8388608).\n\
 Sector can be one of (512, 4096).");

 static PyObject *
 py_read_resource_owners(PyObject *self __unused, PyObject *args, PyObject *keywds)
 {
-    int rv, hss_count = 0;
+    int rv = -1, hss_count = 0;
     int sector = SECTOR_SIZE_512;
     long align = ALIGNMENT_1M;
-    const char *lockspace, *resource;
+    PyObject *lockspace = NULL, *resource = NULL;
     struct sanlk_resource *res = NULL;
     struct sanlk_host *hss = NULL;
     PyObject *disks, *ls_list = NULL;

     static char *kwlist[] = {"lockspace", "resource", "disks", "align",
                              "sector", NULL};

     /* parse python tuple */
-    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|li", kwlist,
-        &lockspace, &resource, &PyList_Type, &disks, &align, &sector)) {
-        return NULL;
+    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|li", kwlist,
+        convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
+        &PyList_Type, &disks, &align, &sector)) {
+        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);

     /* set resource alignment and sector flags */

     if (add_align_flag(align, &res->flags) == -1)
-        goto exit_fail;
+        goto finally;

     if (add_sector_flag(sector, &res->flags) == -1)
-        goto exit_fail;
+        goto finally;

     /* read resource owners (gil disabled) */
     Py_BEGIN_ALLOW_THREADS
     rv = sanlock_read_resource_owners(res, 0, &hss, &hss_count);
     Py_END_ALLOW_THREADS

     if (rv != 0) {
         __set_exception(rv, "Unable to read resource owners");
-        goto exit_fail;
+        goto finally;
     }

     ls_list = __hosts_to_list(hss, hss_count);

-exit_fail:
-    if (res) free(res);
-    if (hss) free(hss);
+finally:
+    Py_XDECREF(lockspace);
+    Py_XDECREF(resource);
+    free(res);
+    free(hss);
+    if (rv != 0)
+        return NULL;

The above 2 lines (the "if" statement) are redundant, that's why they were not in the original code. Although they do add for readability, so better leave them.

 

     return ls_list;
 }

 /* killpath */
 PyDoc_STRVAR(pydoc_killpath, "\
--
2.17.2