i noticed that while working on it, but its more readable to know we handle the bad situation explicitly.
On Mon, Jun 10, 2019 at 7:07 PM Pavel Bar pbar@redhat.com wrote:
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,§or)) {
return NULL;
- if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|li",
kwlist,
convert_to_pybytes, &lockspace, convert_to_pybytes,&resource,
&PyList_Type, &disks, &align, §or)) {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
sanlock-devel@lists.fedorahosted.org
-
Amit Bawer