Thanks for these comments.
I will refactor the loop code, rethink about other codes and make v3 patch.
Guannan Ren
----- Original Message -----
From: "Eric Blake" <eblake@xxxxxxxxxx>
To: "Guannan Ren" <gren@xxxxxxxxxx>
Cc: libvir-list@xxxxxxxxxx
Sent: Saturday, January 21, 2012 11:04:15 PM
Subject: Re: [PATCH v2] Add two NUMA tuning python bindings APIs
On 01/19/2012 02:16 AM, Guannan Ren wrote:
> *virDomainSetNumaParameters
> *virDomainGetNumaParameters
> ---
> python/libvirt-override-api.xml | 13 +++
> python/libvirt-override.c | 186 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 199 insertions(+), 0 deletions(-)
>
> static PyObject *
> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args) {
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
> + LIBVIRT_END_ALLOW_THREADS;
Why are we getting the existing parameters, instead of just blindly
constructing params based on args? I think that's a waste of effort,
considering that libvirt will already reject things if the user passes
in unknown key names. Besides,
> +
> + if (i_retval < 0) {
> + free(params);
> + return VIR_PY_INT_FAIL;
> + }
> +
> + /* convert to a Python tuple of long objects */
> + for (i = 0; i < nparams; i++) {
> + PyObject *key, *val;
> + key = libvirt_constcharPtrWrap(params[i].field);
> + val = PyDict_GetItem(info, key);
> + Py_DECREF(key);
> +
> + if (val == NULL)
> + continue;
this says that you will silently discard any unknown keys passed in from
the python code, whereas libvirt would noisily reject unknown keys. I
don't think the python code should behave differently from the C code.
> +
> + switch (params[i].type) {
> + case VIR_TYPED_PARAM_INT:
> + params[i].value.i = (int)PyInt_AS_LONG(val);
> + break;
We're starting to repeat this code sequence in several places - we ought
to refactor things to share this conversion to and from python types and
virTypedParameter, so that all of the glue code can share the same
conversions (and fixing bugs in one will fix all of them at the same time).
> +static PyObject *
> +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args) {
Indentation; also, we've lately been sticking the { of a function on
column 1:
static PyObject *
libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
{
> +
> + if (i_retval < 0)
> + return VIR_PY_NONE;
I think this is okay; it tells python that the C call successfully
reported an error, and that the caller can then query for the last
libvirt error.
> +
> + if ((params = malloc(sizeof(*params)*nparams)) == NULL)
> + return VIR_PY_NONE;
Bad - we didn't raise any libvirt error, so the user wouldn't be able to
tell why we returned NONE. Rather, we should really be calling return
PyErr_NoMemory() in situations where we failed to malloc(), so that
python can correctly raise a memory exception. (You were just
copying-and-pasting; the problem is pervasive throughout this file).
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (i_retval < 0) {
> + free(params);
> + return VIR_PY_NONE;
> + }
Okay.
> +
> + /* convert to a Python tuple of long objects */
> + if ((info = PyDict_New()) == NULL) {
> + free(params);
Memory leak - params might include VIR_TYPED_PARAM_STRING contents, so
we need to call virTypedParameterArrayClear before freeing params.
> + return VIR_PY_NONE;
Bad - PyDict_New already raised the python exception for no memory, but
we silently discarded it by returning NONE instead of NULL. We should
really be propagating any python errors back up the call chain.
> + }
> +
> + for (i = 0 ; i < nparams ; i++) {
> + PyObject *key, *val;
> +
> + switch (params[i].type) {
> + case VIR_TYPED_PARAM_INT:
> + val = PyInt_FromLong((long)params[i].value.i);
> + break;
Again, worth refactoring code to share this loop.
> +
> + default:
> + free(params);
Another memory leak if params had any string contents.
> + Py_DECREF(info);
> + return VIR_PY_NONE;
> + }
> +
> + key = libvirt_constcharPtrWrap(params[i].field);
> + PyDict_SetItem(info, key, val);
Bad - we should be checking for failure of PyDict_SetItem, and
propagating that failure back to the caller.
> + }
> + free(params);
Another memory leak.
> + return(info);
> +}
> +
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list