Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
- Original Message -
From: Eric Blake ebl...@redhat.com
To: Guan Nan Ren g...@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 ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs
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 ebl...@redhat.com
To: Guannan Ren g...@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 ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman
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.
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 ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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 ORapos;ed set of
virDomainModificationImpact'/
/function
+function name='virDomainSetNumaParameters' file='python'
+ infoChange 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 ORapos;ed set of
virDomainModificationImpact'/
+/function
+function name='virDomainGetNumaParameters' file='python'
+ infoGet 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 ORapos;ed set of
virDomainModificationImpact'/
+/function
function name='virConnectListStoragePools' file='python'
infolist 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 *
