[libvirt] [PATCH v2] 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'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 numa tunable objects'/> + <arg name='flags' type='int' info='an OR'ed set of virDomainModificationImpact'/> + </function> + <function name='virDomainGetNumaParameters' file='python'> + <info>Get the NUMA parameters</info> + <return type='virTypedParameterPtr' info='the numa 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'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); + break; + + default: + free(params); + Py_DECREF(info); + return VIR_PY_NONE; + } + + key = libvirt_constcharPtrWrap(params[i].field); + 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}, -- 1.7.7.5
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@redhat.com> To: "Guannan Ren" <gren@redhat.com> Cc: libvir-list@redhat.com Sent: Saturday, January 21, 2012 11:04:15 PM Subject: Re: [libvirt] [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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/24/2012 05:07 AM, Guan Nan Ren wrote:
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.
I finally realized _why_ we do it - that's because we want to pass the correct types to libvirt.c, but python is not strongly typed. That is, if libvirt is expecting a particular named value to be VIR_TYPED_PARAM_ULLONG, but the python code passes '1', we should be able to properly convert that python value to C's 1ULL, rather than rejecting the python code for using an incorrect type, since there is not quite a notion of incorrect type in python. That means that either we hard-code the list of all known parameter names and their type, or we make things done via a runtime query by getting all parameters to learn their types before formatting the user's settings back into something that libvirt will parse. It may also mean that libvirt itself should be a bit nicer about things - now that we have virTypedParam.c, maybe that function should be taught to do some type conversions (if the user passes in VIR_TYPED_PARAM_INT, but the code wanted VIR_TYPED_PARAM_ULLONG, libvirt could do the type conversion on the user's behalf); if libvirt is made smarter, then the python glue code might be simpler. But there's still the issue of back-compat - a newer python library talking to an older libvirt that didn't do automatic type conversion would still be stuck needing to query for types first. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
----- Original Message ----- From: "Eric Blake" <eblake@redhat.com> To: "Guan Nan Ren" <gren@redhat.com> Cc: libvir-list@redhat.com Sent: Tuesday, January 24, 2012 8:56:30 PM Subject: Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs On 01/24/2012 05:07 AM, Guan Nan Ren wrote:
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.
I finally realized _why_ we do it - that's because we want to pass the correct types to libvirt.c, but python is not strongly typed. That is, if libvirt is expecting a particular named value to be VIR_TYPED_PARAM_ULLONG, but the python code passes '1', we should be able to properly convert that python value to C's 1ULL, rather than rejecting the python code for using an incorrect type, since there is not quite a notion of incorrect type in python. There is another reason for this, we couldn't determine which one or more parameter will be set in case of multiple parameters, unless we hard-code the parameter name to compare. So the a little of overhead caused by the loop and reassigning new value to given parameter based on arg is acceptable. That means that either we hard-code the list of all known parameter names and their type, or we make things done via a runtime query by getting all parameters to learn their types before formatting the user's settings back into something that libvirt will parse. It may also mean that libvirt itself should be a bit nicer about things - now that we have virTypedParam.c, maybe that function should be taught to do some type conversions (if the user passes in VIR_TYPED_PARAM_INT, but the code wanted VIR_TYPED_PARAM_ULLONG, libvirt could do the type conversion on the user's behalf); if libvirt is made smarter, then the python glue code might be simpler. But there's still the issue of back-compat - a newer python library talking to an older libvirt that didn't do automatic type conversion would still be stuck needing to query for types first. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Guan Nan Ren -
Guannan Ren