On Mon, Jun 17, 2019 at 12:47 PM Pavel Bar <pbar(a)redhat.com> 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> 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>
> ---
> 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
>
>