On 01/19/2012 09:38 AM, Guan Nan Ren wrote: > Hi > > Anybody could give a hand on reviewing this patch, > I appreciate it. > Chinese New Year is coming, best wishes to this community :) > > Guannan Ren Happy New Year! > ----- Original Message ----- > From: "Guannan Ren&quot;&lt;gren(a)redhat.com&gt; > To: libvir-list(a)redhat.com > Sent: Monday, January 16, 2012 6:58:06 PM > Subject: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs > > *virDomainSetNumaParameters > *virDomainGetNumaParameters > --- > python/libvirt-override-api.xml | 13 +++ > python/libvirt-override.c | 186 > +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 199 insertions(+), 0 deletions(-) > > diff --git a/python/libvirt-override-api.xml > b/python/libvirt-override-api.xml > index 704fee9..e09290c 100644 > --- a/python/libvirt-override-api.xml > +++ b/python/libvirt-override-api.xml > @@ -248,6 +248,19 @@ > <arg name='domain' type='virDomainPtr' info='pointer to domain object'/> > <arg name='flags' type='int' info='an OR&apos;ed set of > virDomainModificationImpact'/> > </function> > +<function name='virDomainSetNumaParameters' file='python'> > +<info>Change the NUMA tunables</info> > +<return type='int' info='-1 in case of error, 0 in case of success.'/> > +<arg name='domain' type='virDomainPtr' info='pointer to domain > object'/> > +<arg name='params' type='virTypedParameterPtr' info='pointer to > memory tunable objects'/>

> +<arg name='flags' type='int' info='an OR&apos;ed set of > virDomainModificationImpact'/> > +</function> > +<function name='virDomainGetNumaParameters' file='python'> > +<info>Get the NUMA parameters</info> > +<return type='virTypedParameterPtr' info='the I/O tunables value or > None in case of error'/>

> +<arg name='domain' type='virDomainPtr' info='pointer to domain > object'/> > +<arg name='flags' type='int' info='an OR&apos;ed set of > virDomainModificationImpact'/> > +</function> > <function name='virConnectListStoragePools' file='python'> > <info>list the storage pools, stores the pointers to the names in > @names</info> > <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor > connection'/> > diff --git a/python/libvirt-override.c b/python/libvirt-override.c > index d2aad0f..27ad1d8 100644 > --- a/python/libvirt-override.c > +++ b/python/libvirt-override.c > @@ -1000,6 +1000,190 @@ libvirt_virDomainGetMemoryParameters(PyObject > *self ATTRIBUTE_UNUSED, > } > > static PyObject * > +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED, > + PyObject *args) { > + virDomainPtr domain; > + PyObject *pyobj_domain, *info; > + int i_retval; > + int nparams = 0, i; > + unsigned int flags; > + virTypedParameterPtr params; > + > + if (!PyArg_ParseTuple(args, > + (char *)"OOi:virDomainSetNumaParameters", > +&pyobj_domain,&info,&flags)) > + return(NULL); > + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); > + > + LIBVIRT_BEGIN_ALLOW_THREADS; > + i_retval = virDomainGetNumaParameters(domain, NULL,&nparams, > flags); > + LIBVIRT_END_ALLOW_THREADS; > + > + if (i_retval< 0) > + return VIR_PY_INT_FAIL; > + > + if ((params = malloc(sizeof(*params)*nparams)) == NULL) > + return VIR_PY_INT_FAIL; > + > + LIBVIRT_BEGIN_ALLOW_THREADS; > + i_retval = virDomainGetNumaParameters(domain, params,&nparams, > flags); > + LIBVIRT_END_ALLOW_THREADS; > + > + 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; > + > + switch (params[i].type) { > + case VIR_TYPED_PARAM_INT: > + params[i].value.i = (int)PyInt_AS_LONG(val); > + break; > + > + case VIR_TYPED_PARAM_UINT: > + params[i].value.ui = (unsigned int)PyInt_AS_LONG(val); > + break; > + > + case VIR_TYPED_PARAM_LLONG: > + params[i].value.l = (long long)PyLong_AsLongLong(val); > + break; > + > + case VIR_TYPED_PARAM_ULLONG: > + params[i].value.ul = (unsigned long > long)PyLong_AsLongLong(val); > + break; > + > + case VIR_TYPED_PARAM_DOUBLE: > + params[i].value.d = (double)PyFloat_AsDouble(val); > + break; > + > + case VIR_TYPED_PARAM_BOOLEAN: > + { > + /* Hack - Python's definition of Py_True breaks strict > + * aliasing rules, so can't directly compare > + */ > + PyObject *hacktrue = PyBool_FromLong(1); > + params[i].value.b = hacktrue == val ? 1: 0; > + Py_DECREF(hacktrue); > + } > + break; > + > + case VIR_TYPED_PARAM_STRING: > + params[i].value.s = PyString_AsString(val); > + break; > + > + default: > + free(params); > + return VIR_PY_INT_FAIL; > + } > + } > + > + LIBVIRT_BEGIN_ALLOW_THREADS; > + i_retval = virDomainSetNumaParameters(domain, params, nparams, > flags); > + LIBVIRT_END_ALLOW_THREADS; > + if (i_retval< 0) { > + free(params); > + return VIR_PY_INT_FAIL; > + } > + > + free(params); > + return VIR_PY_INT_SUCCESS; > +} > + > +static PyObject * > +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED, > + PyObject *args) { > + virDomainPtr domain; > + PyObject *pyobj_domain, *info; > + int i_retval; > + int nparams = 0, i; > + unsigned int flags; > + virTypedParameterPtr params; > + > + if (!PyArg_ParseTuple(args, (char > *)"Oi:virDomainGetNumaParameters", > +&pyobj_domain,&flags)) > + return(NULL); > + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); > + > + LIBVIRT_BEGIN_ALLOW_THREADS; > + i_retval = virDomainGetNumaParameters(domain, NULL,&nparams, > flags); > + LIBVIRT_END_ALLOW_THREADS; > + > + if (i_retval< 0) > + return VIR_PY_NONE; > + > + if ((params = malloc(sizeof(*params)*nparams)) == NULL) > + return VIR_PY_NONE; > + > + 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; > + } > + > + /* convert to a Python tuple of long objects */ > + if ((info = PyDict_New()) == NULL) { > + free(params); > + return VIR_PY_NONE; > + } > + > + 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; > + > + case VIR_TYPED_PARAM_UINT: > + val = PyInt_FromLong((long)params[i].value.ui); > + break; > + > + case VIR_TYPED_PARAM_LLONG: > + val = PyLong_FromLongLong((long long)params[i].value.l); > + break; > + > + case VIR_TYPED_PARAM_ULLONG: > + val = PyLong_FromLongLong((long long)params[i].value.ul); > + break; > + > + case VIR_TYPED_PARAM_DOUBLE: > + val = PyFloat_FromDouble((double)params[i].value.d); > + break; > + > + case VIR_TYPED_PARAM_BOOLEAN: > + val = PyBool_FromLong((long)params[i].value.b); > + break; > + > + case VIR_TYPED_PARAM_STRING: > + val = libvirt_constcharPtrWrap(params[i].value.s); The above style isn't consistent with previous codes, the libvirt_constcharPtrWrap is a wrapper function, it will be better to uniform them. In addition, the function libvirt_constcharPtrWrap will call PyString_FromString() to return a new reference of a string, but the original string memory hasn't been released, so original function/caller need to free it, moreover, the libvirt_constcharPtrWrap is called in a loop, so you also need to free it in a loop and 'default' branch. > + break; > + > + default: > + free(params); > + Py_DECREF(info); > + return VIR_PY_NONE; > + } > + > + key = libvirt_constcharPtrWrap(params[i].field); Memory leaks again. > + PyDict_SetItem(info, key, val); > + } > + free(params); > + return(info); > +} > + > +static PyObject * > libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED, > PyObject *args) { > virDomainPtr domain; > @@ -5162,6 +5346,8 @@ static PyMethodDef libvirtMethods[] = { > {(char *) "virDomainGetBlkioParameters", > libvirt_virDomainGetBlkioParameters, METH_VARARGS, NULL}, > {(char *) "virDomainSetMemoryParameters", > libvirt_virDomainSetMemoryParameters, METH_VARARGS, NULL}, > {(char *) "virDomainGetMemoryParameters", > libvirt_virDomainGetMemoryParameters, METH_VARARGS, NULL}, > + {(char *) "virDomainGetNumaParameters", > libvirt_virDomainGetNumaParameters, METH_VARARGS, NULL}, > + {(char *) "virDomainSetNumaParameters", > libvirt_virDomainSetNumaParameters, METH_VARARGS, NULL}, > {(char *) "virDomainGetVcpus", libvirt_virDomainGetVcpus, > METH_VARARGS, NULL}, > {(char *) "virDomainPinVcpu", libvirt_virDomainPinVcpu, > METH_VARARGS, NULL}, > {(char *) "virDomainPinVcpuFlags", > libvirt_virDomainPinVcpuFlags, METH_VARARGS, NULL}, -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

----- Original Message ----- From: "Alex Jia&quot;&lt;ajia(a)redhat.com&gt; To: "Guan Nan Ren&quot;&lt;gren(a)redhat.com&gt; Cc: libvir-list(a)redhat.com Sent: Thursday, January 19, 2012 1:39:58 PM Subject: Re: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs On 01/19/2012 11:29 AM, Alex Jia wrote: > On 01/19/2012 09:38 AM, Guan Nan Ren wrote: >> Hi >> >> Anybody could give a hand on reviewing this patch, >> I appreciate it. >> Chinese New Year is coming, best wishes to this community :) >> >> Guannan Ren > Happy New Year! >> ----- Original Message ----- >> From: "Guannan Ren&quot;&lt;gren(a)redhat.com&gt; >> To: libvir-list(a)redhat.com >> Sent: Monday, January 16, 2012 6:58:06 PM >> Subject: [libvirt] [PATCH] Add two NUMA tuning python bindings APIs >> >> *virDomainSetNumaParameters >> *virDomainGetNumaParameters >> --- >> python/libvirt-override-api.xml | 13 +++ >> python/libvirt-override.c | 186 >> +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 199 insertions(+), 0 deletions(-) >> >> diff --git a/python/libvirt-override-api.xml >> b/python/libvirt-override-api.xml >> index 704fee9..e09290c 100644 >> --- a/python/libvirt-override-api.xml >> +++ b/python/libvirt-override-api.xml >> @@ -248,6 +248,19 @@ >> <arg name='domain' type='virDomainPtr' info='pointer to domain object'/> >> <arg name='flags' type='int' info='an OR&apos;ed set of >> virDomainModificationImpact'/> >> </function> >> +<function name='virDomainSetNumaParameters' file='python'> >> +<info>Change the NUMA tunables</info> >> +<return type='int' info='-1 in case of error, 0 in case of success.'/> >> +<arg name='domain' type='virDomainPtr' info='pointer to domain >> object'/> >> +<arg name='params' type='virTypedParameterPtr' info='pointer to >> memory tunable objects'/> A copy-paste error, s/memory/numa/. >> +<arg name='flags' type='int' info='an OR&apos;ed set of >> virDomainModificationImpact'/> >> +</function> >> +<function name='virDomainGetNumaParameters' file='python'> >> +<info>Get the NUMA parameters</info> >> +<return type='virTypedParameterPtr' info='the I/O tunables value or >> None in case of error'/> Save as above, s/I/O/numa/. >> +<arg name='domain' type='virDomainPtr' info='pointer to domain >> object'/> >> +<arg name='flags' type='int' info='an OR&apos;ed set of >> virDomainModificationImpact'/> >> +</function> >> <function name='virConnectListStoragePools' file='python'> >> <info>list the storage pools, stores the pointers to the names in >> @names</info> >> <arg name='conn' type='virConnectPtr' info='pointer to the hypervisor >> connection'/> >> diff --git a/python/libvirt-override.c b/python/libvirt-override.c >> index d2aad0f..27ad1d8 100644 >> --- a/python/libvirt-override.c >> +++ b/python/libvirt-override.c >> @@ -1000,6 +1000,190 @@ libvirt_virDomainGetMemoryParameters(PyObject >> *self ATTRIBUTE_UNUSED, >> } >> >> static PyObject * >> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED, >> + PyObject *args) { >> + virDomainPtr domain; >> + PyObject *pyobj_domain, *info; >> + int i_retval; >> + int nparams = 0, i; >> + unsigned int flags; >> + virTypedParameterPtr params; >> + >> + if (!PyArg_ParseTuple(args, >> + (char *)"OOi:virDomainSetNumaParameters", >> +&pyobj_domain,&info,&flags)) >> + return(NULL); >> + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); >> + >> + LIBVIRT_BEGIN_ALLOW_THREADS; >> + i_retval = virDomainGetNumaParameters(domain, NULL,&nparams, >> flags); >> + LIBVIRT_END_ALLOW_THREADS; >> + >> + if (i_retval< 0) >> + return VIR_PY_INT_FAIL; >> + >> + if ((params = malloc(sizeof(*params)*nparams)) == NULL) >> + return VIR_PY_INT_FAIL; >> + >> + LIBVIRT_BEGIN_ALLOW_THREADS; >> + i_retval = virDomainGetNumaParameters(domain, params,&nparams, >> flags); >> + LIBVIRT_END_ALLOW_THREADS; >> + >> + 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; >> + >> + switch (params[i].type) { >> + case VIR_TYPED_PARAM_INT: >> + params[i].value.i = (int)PyInt_AS_LONG(val); >> + break; >> + >> + case VIR_TYPED_PARAM_UINT: >> + params[i].value.ui = (unsigned int)PyInt_AS_LONG(val); >> + break; >> + >> + case VIR_TYPED_PARAM_LLONG: >> + params[i].value.l = (long long)PyLong_AsLongLong(val); >> + break; >> + >> + case VIR_TYPED_PARAM_ULLONG: >> + params[i].value.ul = (unsigned long >> long)PyLong_AsLongLong(val); >> + break; >> + >> + case VIR_TYPED_PARAM_DOUBLE: >> + params[i].value.d = (double)PyFloat_AsDouble(val); >> + break; >> + >> + case VIR_TYPED_PARAM_BOOLEAN: >> + { >> + /* Hack - Python's definition of Py_True breaks strict >> + * aliasing rules, so can't directly compare >> + */ >> + PyObject *hacktrue = PyBool_FromLong(1); >> + params[i].value.b = hacktrue == val ? 1: 0; >> + Py_DECREF(hacktrue); >> + } >> + break; >> + >> + case VIR_TYPED_PARAM_STRING: >> + params[i].value.s = PyString_AsString(val); >> + break; >> + >> + default: >> + free(params); >> + return VIR_PY_INT_FAIL; >> + } >> + } >> + >> + LIBVIRT_BEGIN_ALLOW_THREADS; >> + i_retval = virDomainSetNumaParameters(domain, params, nparams, >> flags); >> + LIBVIRT_END_ALLOW_THREADS; >> + if (i_retval< 0) { >> + free(params); >> + return VIR_PY_INT_FAIL; >> + } >> + >> + free(params); >> + return VIR_PY_INT_SUCCESS; >> +} >> + >> +static PyObject * >> +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED, >> + PyObject *args) { >> + virDomainPtr domain; >> + PyObject *pyobj_domain, *info; >> + int i_retval; >> + int nparams = 0, i; >> + unsigned int flags; >> + virTypedParameterPtr params; >> + >> + if (!PyArg_ParseTuple(args, (char >> *)"Oi:virDomainGetNumaParameters", >> +&pyobj_domain,&flags)) >> + return(NULL); >> + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); >> + >> + LIBVIRT_BEGIN_ALLOW_THREADS; >> + i_retval = virDomainGetNumaParameters(domain, NULL,&nparams, >> flags); >> + LIBVIRT_END_ALLOW_THREADS; >> + >> + if (i_retval< 0) >> + return VIR_PY_NONE; >> + >> + if ((params = malloc(sizeof(*params)*nparams)) == NULL) >> + return VIR_PY_NONE; >> + >> + 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; >> + } >> + >> + /* convert to a Python tuple of long objects */ >> + if ((info = PyDict_New()) == NULL) { >> + free(params); >> + return VIR_PY_NONE; >> + } >> + >> + 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; >> + >> + case VIR_TYPED_PARAM_UINT: >> + val = PyInt_FromLong((long)params[i].value.ui); >> + break; >> + >> + case VIR_TYPED_PARAM_LLONG: >> + val = PyLong_FromLongLong((long long)params[i].value.l); >> + break; >> + >> + case VIR_TYPED_PARAM_ULLONG: >> + val = PyLong_FromLongLong((long long)params[i].value.ul); >> + break; >> + >> + case VIR_TYPED_PARAM_DOUBLE: >> + val = PyFloat_FromDouble((double)params[i].value.d); >> + break; >> + >> + case VIR_TYPED_PARAM_BOOLEAN: >> + val = PyBool_FromLong((long)params[i].value.b); >> + break; >> + >> + case VIR_TYPED_PARAM_STRING: >> + val = libvirt_constcharPtrWrap(params[i].value.s); > The above style isn't consistent with previous codes, the > libvirt_constcharPtrWrap is > a wrapper function, it will be better to uniform them. > > In addition, the function libvirt_constcharPtrWrap will call > PyString_FromString() to > return a new reference of a string, but the original string memory > hasn't been released, > so original function/caller need to free it, moreover, the > libvirt_constcharPtrWrap is > called in a loop, so you also need to free it in a loop and 'default' > branch. Hi Alex The call to PyString_FromString() will cause Python memory manager to allocate a new string object in Python's private heap rather than incrementing a reference to the string in system heap. Meanwhile, the function returns the ownership of a new reference(probably the first one) to the string object to "val". According to the Memory Management doc,"PyDict_SetItem() and friends don’t take over ownership" that means It doesn't increment the reference to the string object.

So, The object reference returned from the C function that is called from Python is returned to the Python program with only one reference. as for the string in the system heap, it is release by "free(params)" above the last return.

Guannan Ren