Re: [PATCH 14/18] python: Use PyBytes converter for read_resource_owners API

[Amit Bawer]

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