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

[Amit Bawer]

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(a)redhat.com&gt; 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(a)redhat.com&gt; 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(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
> >>>>>
> >>>>>