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(a)redhat.com> wrote:
On Mon, Jun 3, 2019 at 11:31 PM Nir Soffer <nirsof(a)gmail.com> wrote:
> From: Amit Bawer <abawer(a)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
>
>