Re: [libvirt] [PATCH 1/2 v3] Python: Refactoring virTypedParameter conversion for NUMA tuning APIs

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&apos;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.