On 01/28/2012 07:53 AM, Guannan Ren wrote:
> *virDomainSetNumaParameters
> *virDomainGetNumaParameters
> ---
> python/Makefile.am | 4 +-
> python/libvirt-override-api.xml | 13 ++
> python/libvirt-override.c | 314 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 330 insertions(+), 1 deletions(-)
>
> diff --git a/python/Makefile.am b/python/Makefile.am
> index 3068eee..4302fa5 100644
> --- a/python/Makefile.am
> +++ b/python/Makefile.am
> @@ -8,6 +8,8 @@ SUBDIRS= . tests
> INCLUDES = \
> $(PYTHON_INCLUDES) \
> -I$(top_srcdir)/include \
> + -I$(top_srcdir)/src \
> + -I$(top_srcdir)/gnulib/lib \
Hmm, you converted TAB to space.
> -I$(top_builddir)/include \
Also, since gnulib has some directly-supplied headers (srcdir) and some
generated headers (builddir), we really should be using both locations
so as not to break VPATH.
> -I$(top_builddir)/$(subdir) \
> $(GETTEXT_CPPFLAGS)
> @@ -42,7 +44,7 @@ all-local: libvirt.py libvirt_qemu.py
>
> pyexec_LTLIBRARIES = libvirtmod.la libvirtmod_qemu.la
>
> -libvirtmod_la_SOURCES = libvirt-override.c typewrappers.c
> +libvirtmod_la_SOURCES = libvirt-override.c typewrappers.c ../src/util/virtypedparam.c
I'm not sure I like this. Rather than pulling in just one or two source
files, we should probably instead figure out how to directly link
against the libvirt_util library and have all of the functions
available. This would also make it possible to use VIR_FREE and friends
(at which point, we should disable the syntax-check exceptions currently
in effect on the python files).
I think I will do a preliminary patch, which does _just_ the makefile
work to pull in the use of libvirt_util, then we can rebase this patch
on top of that one. I know Alex Jia was also complaining about the
inability to use normal libvirt conventions, because the Makefile wasn't
yet set up for it, so this will be a good move overall.
> + <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'/>
Is th is type correct, or can it be any python dictionary type that maps
valid numa tunable parameter names to values?
> + <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='int' info='returns a dictionary of params in case of success, -1 in case of error'/>
The return type should be a python object - a dictionary on success,
PyNone on failure where libvirt populated an error message, or NULL on a
python exception.
> +++ b/python/libvirt-override.c
> @@ -21,6 +21,7 @@
> #include "libvirt/virterror.h"
> #include "typewrappers.h"
> #include "libvirt.h"
> +#include "util/virtypedparam.h"
Hmm, the rest of our code sets up INCLUDES so that we can use just
"virtypedparam.h" instead of "util/virtypedparam.h"; another thing for
me to do in pulling out the infrastructure into a preliminary patch.
>
> #ifndef __CYGWIN__
> extern void initlibvirtmod(void);
> @@ -61,6 +62,208 @@ static char *py_str(PyObject *obj)
> return PyString_AsString(str);
> }
>
> +/* Two helper functions to help the conversions between C to Python
> + * for the virTypedParameter used in the following APIs. */
> +static PyObject *
> +getPyVirTypedParameter(virTypedParameterPtr params, int nparams)
> +{
> + PyObject *info;
> + PyObject *key, *val;
> + PyObject *ret = NULL;
> + int i;
> +
> + if (!params)
> + return ret;
If we return NULL, we should ensure that there is a valid python
exception object ready for the caller to access. I'm thinking it might
be better to mark this function with ATTRIBUTE_NONNULL(1) to avoid
worrying about whether the caller has properly generated a python
exception before passing us NULL.
> +
> + /* convert to a Python tuple of long objects */
> + if ((info = PyDict_New()) == NULL) {
> + return ret;
> + }
This one's fine - PyDict_New guarantees a python exception is ready to go.
> +
> + for (i = 0 ; i < nparams ; i++) {
> + switch (params[i].type) {
> + case VIR_TYPED_PARAM_INT:
> + val = PyInt_FromLong((long)params[i].value.i);
> + break;
> +
> + case VIR_TYPED_PARAM_STRING:
> + val = libvirt_constcharPtrWrap(params[i].value.s);
> + break;
> +
> + default:
> + Py_DECREF(info);
> + return ret;
This returns NULL but without setting a python exception. Then again,
this can only happen if the server sent us something which we don't
understand (arguably a bug in the libvirt.so used by the server). Maybe
it would be better to return Py_None instead of NULL in this situation,
since I don't know what python exception we would raise.
At any rate, if my understanding is correct, your use of a simple
Py_DECREF of a non-empty python dictionary sets up garbage collection
for all the contents of that dictionary (that is, we don't have to
explicitly clean up the contents, just abandoning info was good enough).
> + }
> +
> + key = libvirt_constcharPtrWrap(params[i].field);
> + if (!key || !val)
> + goto fail;
> +
> + if (PyDict_SetItem(info, key, val) < 0)
> + goto fail;
> +
> + Py_DECREF(key);
> + Py_DECREF(val);
> + }
> + return info;
> +fail:
> + Py_XDECREF(info);
> + Py_XDECREF(key);
> + Py_XDECREF(val);
> + return ret;
> +}
The rest of this function looks right to me - any way that gets to the
fail label will properly have set a python exception and return NULL.
> +
> +static PyObject *
> +setPyVirTypedParameter(PyObject *info, virTypedParameterPtr params, int nparams)
A comment would go a long way on describing what this function does. My
understanding is that it takes an incoming python object 'info', which
should behave like a dictionary containing just name/value references,
as well as incoming params previously collected from the domain. Then
for every key in params found in info, we update params in-place to
contain the new value from info, so that we can then pass params back to
libvirt to update values.
I think this has a couple of design issues. Consider
virDomainSetBlockIoTune, which currently has 6 defined parameters when
getting values, but where the parameters are, to some degree, mutually
exclusive. That is, if I set VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC to
non-zero, then I must also omit VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC
or else set it to 0 when setting values. But if read_bytes_sec was
previously set, and the user passes in a python array containing just a
mapping for a non-zero total_bytes_sec, then modifying params in place
and passing back all 6 parameters will cause an error (we're passing in
conflicting non-zero values), while passing in a params of just length 1
will properly let libvirt recognize that I am replacing read_bytes_sec
with total_bytes_sec (well, it will once I fix the bugs I just noticed
in qemu_driver.c in handling that situation). The python code should
mirror the C calling conventions where I can pass in a smaller array
than what I get back out on queries, so that libvirt is free to reuse or
replace unspecified parameters as it sees fit.
In other words, I think we really want a different signature - we want
to pass in three items - a python dictionary, and a C array and length
that tell us what types the code is expecting, and in return get one
item - a new C array with length given by PyDict_Size of the incoming
dictionary, or NULL if a python exception has been raised. Something
like this signature:
/* Allocate a new typed parameter array with the same contents and
* length as info, and using the array params of length nparams as
* hints on what types to use when creating the new array. Return
* NULL on failure. */
static virTypedParamterPtr
setPyVirTypedParameter(PyObject *info,
const virTypedParameterPtr params,
int nparams)
> +{
> + PyObject *key, *val;
> + PyObject *ret = NULL;
> + int i;
> +
> + if (!info || !params)
> + return ret;
Not so good. We are returning NULL without setting a python exception.
Again an argument that these should be ATTRIBUTE_NONNULL, and that the
caller should not be passing invalid data.
> +
> + /* convert to a Python tuple of long objects */
Comment is wrong. We are converting a python dictionary into a C struct
virTypedParameter array.
> + for (i = 0; i < nparams; i++) {
I think we're iterating over the wrong struct. We want to convert ever
object in info into a new C structure, and fail if we don't know how to
do the conversion. That is, I think we need the overall structure of
this function to look more like:
pos = 0;
allocate return value according to size of info
while (PyDict_Next(info, &pos, &key, &value)) {
for (i = 0; i < nparams; i++) {
if params[i].field matches key, use params[i].type and break
}
if i == nparams, error out that we don't know how to convert key
populate ret[pos] according to learned type
}
> + key = libvirt_constcharPtrWrap(params[i].field);
Need to check for NULL return, and error out.
> + val = PyDict_GetItem(info, key);
> + Py_DECREF(key);
> +
> + if (val == NULL)
> + continue;
This says that a parameter in params but not in the dictionary is left
unmodified, but it doesn't filter out entries in the dictionary but not
in params. Again evidence that we are iterating over the wrong struct.
> +
> + switch (params[i].type) {
> + case VIR_TYPED_PARAM_INT:
> + {
> + long long_val;
> + if (PyInt_Check(val)) {
> + long_val = PyInt_AsLong(val);
> + if ((long_val == -1) && PyErr_Occurred())
> + return ret;
> + } else {
> + PyErr_SetString(PyExc_TypeError,
> + "Attribute value type must be int");
> + return ret;
> + }
> + params[i].value.i = (int)long_val;
This allows silent overflow. We should validate that (int)long_val ==
long_val, and reject it if it is out of range.
> + case VIR_TYPED_PARAM_STRING:
> + {
> + if (PyString_Check(val)) {
> + free(params[i].value.s);
> + params[i].value.s = PyString_AsString(val);
This can lead to some problematic allocation patterns, since the caller
must not free this string, but you are replacing a string that the
caller must otherwise free to avoid a leak. All the more reason you
need to return a new virTypedParameterPtr array, and not modify the
incoming one in place.
You are missing a check for a NULL return; but then again, the previous
call to PyString_Check guarantees that you should succeed. On the other
hand, since PyString_AsString properly raises a python exception if val
is not a string, we could skip the PyString_Check() call. I don't know
which way is easier to write.
> + } else {
> + PyErr_SetString(PyExc_TypeError,
> + "Attribute value type must be string");
> + return ret;
> + }
> + }
> + break;
> + default:
> + return ret;
> + }
> + }
> + return VIR_PY_NONE;
Here, I'd return the new array that we just created, or NULL; no need to
return VIR_PY_NONE since the result of this function is not passed back
to python, but only used in calling a libvirt C function.
>
> static PyObject *
> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args)
Indentation.
> + if (!setPyVirTypedParameter(info, params, nparams))
> + goto fail;
Given my above thoughts, you should be storing the result of this call,
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + i_retval = virDomainSetNumaParameters(domain, params, nparams, flags);
and passing that result, along with the size of info (and _not_ nparams).
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (i_retval < 0) {
> + ret = VIR_PY_INT_FAIL;
> + goto fail;
> + }
> +
> + /* The string generated by PyString_AsString
> + * must not be deallocated */
Ah, but if you follow my advice about setPyVirTypedParameter returning a
_new_ array, then what we really want to do here is:
virTypedParameterArrayClear(params, nparams);
VIR_FREE(params);
VIR_FREE(new_params);
That is, params contains strings returned by libvirt which the caller
must free, while new_params contains strings which point into python
objects and must not be freed.
> + free(params);
> + return VIR_PY_INT_SUCCESS;
> +fail:
> + /*same as above*/
> + free(params);
> + return ret;
We should probably combine these cleanups into a single path, using our
common idiom:
ret = VIR_PY_INT_SUCCESS;
cleanup:
free params...
return ret;
and change goto fail into goto cleanup.
> +}
> +
> +static PyObject *
> +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> + PyObject *args)
> +{
> +
> + if (i_retval < 0)
> + return VIR_PY_INT_FAIL;
Wrong return value - here, you want to return None, since the successful
return is a python dictionary and not the integer 0.
You might want to take a shortcut here - if nparams is 0, you can return
an empty dictionary, rather than continuing on with mallocing a 0-length
array and wasting time with more libvirt calls.
> +
> + if ((params = malloc(sizeof(*params)*nparams)) == NULL)
> + return PyErr_NoMemory();
> +
> + LIBVIRT_BEGIN_ALLOW_THREADS;
> + i_retval = virDomainGetNumaParameters(domain, params, &nparams, flags);
> + LIBVIRT_END_ALLOW_THREADS;
> +
> + if (i_retval < 0) {
> + ret = VIR_PY_INT_FAIL;
Again, this should return None, not -1.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list