Re: [PATCH v2 3/8] python: Streamline initexception

[Nir Soffer]

On Mon, Jun 17, 2019 at 12:47 PM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  Just something to consider: *maybe* adding "finally" label
> at the end and
>  freeing all the local variables using "Py_CLEAR" in the reverse order of
>  their initialization and always returning "excp" (initialized to NULL at
>  the beginning) could be more readable. Just a thought.
>  
The point of this patch is to remove the cleanup. Why keep the local
variable when we can get
rid of them quickly?

On Sun, Jun 16, 2019 at 11:50 PM Nir Soffer <nirsof(a)gmail.com&gt; wrote:
> > When we create the sanlock expcetion we create a function, create a
> >
>
>  expcetion --> exception
>
>
> > method from the function, create a dict from the method, and finally
> > create an exception from the dict. This simple flow does not require
> > goto for cleanup.
> >
> > Creating a dict is simpler and less error prone using Py_BuildValue().
> >
> > When we finish with unused pointers, it is safer to set them to NULL
> > using Py_CLEAR(), ensuring that we don't access freed memory.
> >
> > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt;
> > ---
> >  python/sanlock.c | 31 ++++++++++++-------------------
> >  1 file changed, 12 insertions(+), 19 deletions(-)
> >
> > diff --git a/python/sanlock.c b/python/sanlock.c
> > index 21a7a76..5c2fd1a 100644
> > --- a/python/sanlock.c
> > +++ b/python/sanlock.c
> > @@ -1741,33 +1741,26 @@ sanlock_exception = {
> >  };
> >
> >  static PyObject *
> >  initexception(void)
> >  {
> > -    int rv;
> > -    PyObject *dict, *func, *meth, *excp = NULL;
> > -
> > -    if ((dict = PyDict_New()) == NULL)
> > -        goto exit_fail;
> > -
> > -    if ((func = PyCFunction_New(&sanlock_exception, NULL)) == NULL)
> > -        goto exit_fail;
> > +    PyObject *func = PyCFunction_New(&sanlock_exception, NULL);
> > +    if (func == NULL)
> > +        return NULL;
> >
> > -    meth = PyObject_CallFunction((PyObject *) &PyProperty_Type, "O",
> > func);
> > -    Py_DECREF(func);
> > +    PyObject *meth = PyObject_CallFunction((PyObject *)
> > &PyProperty_Type, "O", func);
> > +    Py_CLEAR(func);
> >      if (meth == NULL)
> > -        goto exit_fail;
> > -
> > -    rv = PyDict_SetItemString(dict, sanlock_exception.ml_name, meth);
> > -    Py_DECREF(meth);
> > -    if (rv < 0)
> > -        goto exit_fail;
> > +        return NULL;
> >
> > -    excp = PyErr_NewException("sanlock.SanlockException", NULL, dict);
> > +    PyObject *dict = Py_BuildValue("{s:O}", sanlock_exception.ml_name,
> > meth);
> > +    Py_CLEAR(meth);
> > +    if (dict == NULL)
> > +        return NULL;
> >
> > -exit_fail:
> > -    Py_XDECREF(dict);
> > +    PyObject *excp = PyErr_NewException("sanlock.SanlockException",
> > NULL, dict);
> > +    Py_CLEAR(dict);
> >
> >      return excp;
> >  }
> >
> >  static int
> > --
> > 2.17.2
> >
> >