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

[Guannan Ren]

On 02/03/2012 01:48 AM, Eric Blake wrote:
> 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.
>
      Hi Eric

          I saw your comments about the nonnull attribute usage as follows
          http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308
          I am not clear about it is still helpful to use it here?

      Guannan Ren