Re: [PATCH 11/18] python: Use PyBytes converter for resource acquire API

[Amit Bawer]

the original check here is for rv != 0. and cleanup code checks the same.
if we want to change the rv checks for sanlock calls we probably should to
that in a separate patch, not related to py3 changes anyway.

On Mon, Jun 10, 2019 at 3:41 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/resource name.
> > ---
> >  python/sanlock.c | 35 ++++++++++++++++++-----------------
> >  1 file changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/python/sanlock.c b/python/sanlock.c
> > index c7518c6..7bdbd69 100644
> > --- a/python/sanlock.c
> > +++ b/python/sanlock.c
> > @@ -1028,39 +1028,39 @@ mode. The version is the version of the lease
> > that must be acquired or fail.\n\
> >  The disks must be in the format: [(path, offset), ... ]\n");
> >
> >  static PyObject *
> >  py_acquire(PyObject *self __unused, PyObject *args, PyObject *keywds)
> >  {
> > -    int rv, sanlockfd = -1, pid = -1, shared = 0;
> > -    const char *lockspace, *resource;
> > -    struct sanlk_resource *res;
> > +    int rv = -1, sanlockfd = -1, pid = -1, shared = 0;
> > +    PyObject *lockspace = NULL, *resource = NULL;
> > +    struct sanlk_resource *res = NULL;
> >      PyObject *disks, *version = Py_None;
> >
> >      static char *kwlist[] = {"lockspace", "resource",
> "disks", "slkfd",
> >                                  "pid", "shared",
> "version", NULL};
> >
> >      /* parse python tuple */
> > -    if (!PyArg_ParseTupleAndKeywords(args, keywds, "ssO!|iiiO", kwlist,
> > -        &lockspace, &resource, &PyList_Type, &disks, &sanlockfd,
> &pid,
> > -        &shared, &version)) {
> > -        return NULL;
> > +    if (!PyArg_ParseTupleAndKeywords(args, keywds, "O&O&O!|iiiO",
> kwlist,
> > +        convert_to_pybytes, &lockspace, convert_to_pybytes, &resource,
> > +        &PyList_Type, &disks, &sanlockfd, &pid, &shared,
> &version)) {
> > +        goto finally;
> >      }
> >
> >      /* check if any of the slkfd or pid parameters was given */
> >      if (sanlockfd == -1 && pid == -1) {
> >          __set_exception(EINVAL, "Invalid slkfd and pid values");
> > -        return NULL;
> > +        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);
> >
> >      /* prepare sanlock flags */
> >      if (shared) {
> >          res->flags |= SANLK_RES_SHARED;
> >      }
> > @@ -1069,30 +1069,31 @@ py_acquire(PyObject *self __unused, PyObject
> > *args, PyObject *keywds)
> >      if (version != Py_None) {
> >          res->flags |= SANLK_RES_LVER;
> >          res->lver = pyinteger_as_unsigned_long_long_mask(version);
> >          if (res->lver == (uint64_t)-1) {
> >              __set_exception(EINVAL, "Unable to convert the version
> > value");
> > -            goto exit_fail;
> > +            goto finally;
> >          }
> >      }
> >
> >      /* acquire sanlock resource (gil disabled) */
> >      Py_BEGIN_ALLOW_THREADS
> >      rv = sanlock_acquire(sanlockfd, pid, 0, 1, &res, 0);
> >      Py_END_ALLOW_THREADS
> >
> >      if (rv != 0) {
> >          __set_exception(rv, "Sanlock resource not acquired");
> > -        goto exit_fail;
> > +        goto finally;
> >      }
> >
> > +finally:
> > +    Py_XDECREF(lockspace);
> > +    Py_XDECREF(resource);
> >      free(res);
> > +    if (rv != 0)
> >
>
>  Is it intentional "rv != 0" instead of "rv < 0"? I do see "rv
> != 0" in the
>  existing code a few lines above in the method, but wasn't it a mistake? If
>  the convention is that any non-negative is a success, then it should be
>  changed.
>
>
> > +        return NULL;
> >      Py_RETURN_NONE;
> > -
> > -exit_fail:
> > -    free(res);
> > -    return NULL;
> >  }
> >
> >  /* release */
> >  PyDoc_STRVAR(pydoc_release, "\
> >  release(lockspace, resource, disks [, slkfd=fd, pid=owner])\n\
> > --
> > 2.17.2
> >
> >