David Lively <dlively@xxxxxxxxxxxxxxx> wrote:
...
Hi David,
No big deal, but there are several debug printf uses here that look
like they try to print NULL pointers upon memory allocation failure.
It's ok with glibc's printf of course, but not for others.
> commit efd5098e9a834562cddbf1618e36eb43c272f8ea
> Author: David Lively <dlively@xxxxxxxxxxxxxxx>
> Date: Thu Nov 20 16:36:14 2008 -0500
>
> vi-patch: fix-python-events
>
> Get python events working again after upstream changes, and make the
> test implementation properly clean up after itself and implement the
> new EventImpl properly.
...
> diff --git a/python/libvir.c b/python/libvir.c
...
> @@ -1725,9 +1745,18 @@ libvirt_virEventAddHandleFunc (int fd,
> "eventInvokeHandleCallback");
> if(!python_cb) {
> #if DEBUG_ERROR
> - printf("%s Error finding eventInvokeHandleCallback\n", __FUNCTION__);
> + printf("%s: Error finding eventInvokeHandleCallback\n", __FUNCTION__);
> #endif
> PyErr_Print();
> + PyErr_Clear();
> + goto cleanup;
> + }
> + if (!PyCallable_Check(python_cb)) {
> +#if DEBUG_ERROR
> + char *name = py_str(python_cb);
> + printf("%s: %s is not callable\n", __FUNCTION__, name);
Please handle name == NULL.
Otherwise, with DEBUG_ERROR, the printf will segfault
on most non-glibc-based systems.
E.g., define this, then wrap the arg with NN(...).
static inline char *NN (const char *s)
{
return s ? s : (char *) "<NULL>";
}
printf("%s: %s is not callable\n", __FUNCTION__, NN(name));
> + free(name);
> +#endif
> goto cleanup;
> }
> Py_INCREF(python_cb);
> @@ -1748,8 +1777,17 @@ libvirt_virEventAddHandleFunc (int fd,
> PyTuple_SetItem(pyobj_args, 2, python_cb);
> PyTuple_SetItem(pyobj_args, 3, cb_args);
>
> - if(addHandleObj && PyCallable_Check(addHandleObj))
> - result = PyEval_CallObject(addHandleObj, pyobj_args);
> + result = PyEval_CallObject(addHandleObj, pyobj_args);
> + if (!result) {
> + PyErr_Print();
> + PyErr_Clear();
> + } else if (!PyInt_Check(result)) {
> +#if DEBUG_ERROR
> + printf("%s: %s should return an int\n", addHandleName, __FUNCTION__);
Same here, addHandleName can be NULL.
> +#endif
> + } else {
> + retval = (int)PyInt_AsLong(result);
> + }
>
> Py_XDECREF(result);
> Py_DECREF(pyobj_args);
> @@ -1757,23 +1795,26 @@ libvirt_virEventAddHandleFunc (int fd,
> cleanup:
> LIBVIRT_RELEASE_THREAD_STATE;
>
> - return 0;
> + return retval;
> }
>
> static void
> -libvirt_virEventUpdateHandleFunc(int fd, int event)
> +libvirt_virEventUpdateHandleFunc(int watch, int event)
> {
> - PyObject *result = NULL;
> + PyObject *result;
> PyObject *pyobj_args;
>
> LIBVIRT_ENSURE_THREAD_STATE;
>
> pyobj_args = PyTuple_New(2);
> - PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
> + PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(watch));
> PyTuple_SetItem(pyobj_args, 1, libvirt_intWrap(event));
>
> - if(updateHandleObj && PyCallable_Check(updateHandleObj))
> - result = PyEval_CallObject(updateHandleObj, pyobj_args);
> + result = PyEval_CallObject(updateHandleObj, pyobj_args);
> + if (!result) {
> + PyErr_Print();
> + PyErr_Clear();
> + }
>
> Py_XDECREF(result);
> Py_DECREF(pyobj_args);
> @@ -1783,25 +1824,45 @@ libvirt_virEventUpdateHandleFunc(int fd, int event)
>
>
> static int
> -libvirt_virEventRemoveHandleFunc(int fd)
> +libvirt_virEventRemoveHandleFunc(int watch)
> {
> - PyObject *result = NULL;
> + PyObject *result;
> PyObject *pyobj_args;
> + PyObject *opaque;
> + PyObject *ff;
> + int retval = -1;
> + virFreeCallback cff;
>
> LIBVIRT_ENSURE_THREAD_STATE;
>
> pyobj_args = PyTuple_New(1);
> - PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd));
> + PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(watch));
>
> - if(removeHandleObj && PyCallable_Check(removeHandleObj))
> - result = PyEval_CallObject(removeHandleObj, pyobj_args);
> + result = PyEval_CallObject(removeHandleObj, pyobj_args);
> + if (!result) {
> + PyErr_Print();
> + PyErr_Clear();
> + } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
> +#if DEBUG_ERROR
> + printf("%s: %s must return opaque obj registered with %s"
> + "to avoid leaking libvirt memory\n",
> + __FUNCTION__, removeHandleName, addHandleName);
Same here, for both.
> +#endif
> + } else {
> + opaque = PyTuple_GetItem(result, 1);
> + ff = PyTuple_GetItem(result, 2);
> + cff = PyvirFreeCallback_Get(ff);
> + if (cff)
> + (*cff)(PyvirVoidPtr_Get(opaque));
> + retval = 0;
> + }
>
> Py_XDECREF(result);
> Py_DECREF(pyobj_args);
>
> LIBVIRT_RELEASE_THREAD_STATE;
>
> - return 0;
> + return retval;
> }
>
> static int
> @@ -1810,7 +1871,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
> void *opaque,
> virFreeCallback ff)
> {
> - PyObject *result = NULL;
> + PyObject *result;
>
> PyObject *python_cb;
>
> @@ -1819,6 +1880,7 @@ libvirt_virEventAddTimeoutFunc(int timeout,
> PyObject *opaque_obj;
> PyObject *cb_args;
> PyObject *pyobj_args;
> + int retval = -1;
>
> LIBVIRT_ENSURE_THREAD_STATE;
>
> @@ -1827,9 +1889,18 @@ libvirt_virEventAddTimeoutFunc(int timeout,
> "eventInvokeTimeoutCallback");
> if(!python_cb) {
> #if DEBUG_ERROR
> - printf("%s Error finding eventInvokeTimeoutCallback\n", __FUNCTION__);
> + printf("%s: Error finding eventInvokeTimeoutCallback\n", __FUNCTION__);
> #endif
> PyErr_Print();
> + PyErr_Clear();
> + goto cleanup;
> + }
> + if (!PyCallable_Check(python_cb)) {
> +#if DEBUG_ERROR
> + char *name = py_str(python_cb);
> + printf("%s: %s is not callable\n", __FUNCTION__, name);
Same here.
> + free(name);
> +#endif
> goto cleanup;
> }
> Py_INCREF(python_cb);
> @@ -1850,15 +1921,24 @@ libvirt_virEventAddTimeoutFunc(int timeout,
> PyTuple_SetItem(pyobj_args, 1, python_cb);
> PyTuple_SetItem(pyobj_args, 2, cb_args);
>
> - if(addTimeoutObj && PyCallable_Check(addTimeoutObj))
> - result = PyEval_CallObject(addTimeoutObj, pyobj_args);
> + result = PyEval_CallObject(addTimeoutObj, pyobj_args);
> + if (!result) {
> + PyErr_Print();
> + PyErr_Clear();
> + } else if (!PyInt_Check(result)) {
> +#if DEBUG_ERROR
> + printf("%s: %s should return an int\n", addTimeoutName, __FUNCTION__);
addTimeoutName can be NULL.
> +#endif
> + } else {
> + retval = (int)PyInt_AsLong(result);
> + }
>
> Py_XDECREF(result);
> Py_DECREF(pyobj_args);
>
> cleanup:
> LIBVIRT_RELEASE_THREAD_STATE;
> - return 0;
> + return retval;
> }
>
> static void
...
> @@ -1887,45 +1970,85 @@ libvirt_virEventRemoveTimeoutFunc(int timer)
> {
> PyObject *result = NULL;
> PyObject *pyobj_args;
> + PyObject *opaque;
> + PyObject *ff;
> + int retval = -1;
> + virFreeCallback cff;
>
> LIBVIRT_ENSURE_THREAD_STATE;
>
> pyobj_args = PyTuple_New(1);
> PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(timer));
>
> - if(updateTimeoutObj && PyCallable_Check(updateTimeoutObj))
> - result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
> + result = PyEval_CallObject(removeTimeoutObj, pyobj_args);
> + if (!result) {
> + PyErr_Print();
> + PyErr_Clear();
> + } else if (!PyTuple_Check(result) || PyTuple_Size(result) != 3) {
> +#if DEBUG_ERROR
> + printf("%s: %s must return opaque obj registered with %s"
> + "to avoid leaking libvirt memory\n",
> + __FUNCTION__, removeTimeoutName, addTimeoutName);
Likewise.
> +#endif
> + } else {
> + opaque = PyTuple_GetItem(result, 1);
> + ff = PyTuple_GetItem(result, 2);
> + cff = PyvirFreeCallback_Get(ff);
> + if (cff)
> + (*cff)(PyvirVoidPtr_Get(opaque));
> + retval = 0;
> + }
>
> Py_XDECREF(result);
> Py_DECREF(pyobj_args);
>
> LIBVIRT_RELEASE_THREAD_STATE;
>
> - return 0;
> + return retval;
> }
>
> static PyObject *
> libvirt_virEventRegisterImpl(ATTRIBUTE_UNUSED PyObject * self,
> PyObject * args)
> {
> + /* Unref the previously-registered impl (if any) */
> Py_XDECREF(addHandleObj);
> + free(addHandleName);
> Py_XDECREF(updateHandleObj);
> + free(updateHandleName);
> Py_XDECREF(removeHandleObj);
> + free(removeHandleName);
> Py_XDECREF(addTimeoutObj);
> + free(addTimeoutName);
> Py_XDECREF(updateTimeoutObj);
> + free(updateTimeoutName);
> Py_XDECREF(removeTimeoutObj);
> -
> - if (!PyArg_ParseTuple
> - (args, (char *) "OOOOOO:virEventRegisterImpl",
> - &addHandleObj,
> - &updateHandleObj,
> - &removeHandleObj,
> - &addTimeoutObj,
> - &updateTimeoutObj,
> - &removeTimeoutObj
> - ))
> + free(removeTimeoutName);
> +
> + /* Parse and check arguments */
> + if (!PyArg_ParseTuple(args, (char *) "OOOOOO:virEventRegisterImpl",
> + &addHandleObj, &updateHandleObj,
> + &removeHandleObj, &addTimeoutObj,
> + &updateTimeoutObj, &removeTimeoutObj) ||
> + !PyCallable_Check(addHandleObj) ||
> + !PyCallable_Check(updateHandleObj) ||
> + !PyCallable_Check(removeHandleObj) ||
> + !PyCallable_Check(addTimeoutObj) ||
> + !PyCallable_Check(updateTimeoutObj) ||
> + !PyCallable_Check(removeTimeoutObj))
> return VIR_PY_INT_FAIL;
>
> + /* Get argument string representations (for error reporting) */
> + addHandleName = py_str(addTimeoutObj);
> + updateHandleName = py_str(updateHandleObj);
> + removeHandleName = py_str(removeHandleObj);
> + addTimeoutName = py_str(addTimeoutObj);
> + updateTimeoutName = py_str(updateTimeoutObj);
> + removeTimeoutName = py_str(removeTimeoutObj);
...
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list