[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4] python: refactoring virTypedParameter conversion for NUMA tuning APIs



On 02/08/2012 05:20 AM, Guannan Ren wrote:
>           *getPyVirTypedParameter
>           *setPyVirTypedParameter
>           *virDomainSetNumaParameters
>           *virDomainGetNumaParameters
> ---
>  python/libvirt-override-api.xml |   13 ++
>  python/libvirt-override.c       |  385 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 398 insertions(+), 0 deletions(-)
> 
> diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml
> index b2b8152..8eed0bb 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&apos;ed set of virDomainModificationImpact'/>
>      </function>
> +    <function name='virDomainSetNumaParameters' file='python'>
> +      <info>Change the NUMA tunables</info>

<info> is probably the most important element here; any <function> not
in this file copies its in-python documentation verbatim from
src/libvirt.c; but where we have a different python signature (such as
here, because while the C code needs a user-allocated array and length,
the python binding just takes a single python dictionary object), having
only a 4-word documentation when doing
'help(libvirt.virDomain.setNumaParameters)' pales in comparison to what
the C users will see.  But I'm not too worried about this; you aren't
the first person to do awfully short override text.

> +    <function name='virDomainGetNumaParameters' file='python'>
> +      <info>Get the NUMA parameters</info>
> +      <return type='str *' info='returns a dictionary of params in case of success, None in case of error'/>

In fact, the more I've been playing with the python stuff in trying to
review this patch, the more I've come to the conclusion that the
<return> element of this XML is more or less ignored, and only the
<info> element makes it anywhere that it might be useful.  But let's
focus on getting the functionality right before we worry about improving
the documentation.

>  
> +/* Two helper functions to help the conversions between C to Python
> + * for the virTypedParameter used in the following APIs */

I'd document this function in isolation, since the next function is
documented in isolation.

> +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> +getPyVirTypedParameter(PyObject **info,

I'd pass this as PyObject *info (no need for double-indirection, since
you aren't updating the pointer, just the object that it points to).
Either that, or offload the creation of info out of the caller into this
function, and actually update the pointer.  We'll see how the caller
uses it, before I decide...

> +                       const virTypedParameterPtr params, int nparams)
> +{
> +    PyObject *key, *val;
> +    int i;
> +
> +    for (i = 0 ; i < nparams ; i++) {
> +        switch (params[i].type) {
> +        case VIR_TYPED_PARAM_INT:
> +            val = PyInt_FromLong((long)params[i].value.i);

The cast is not necessary.  (But it doesn't hurt, either).

> +            break;
> +
> +        case VIR_TYPED_PARAM_UINT:
> +            val = PyInt_FromLong((unsigned 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_FromUnsignedLongLong((unsigned 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);

Same for all these casts.

> +            break;
> +
> +        case VIR_TYPED_PARAM_STRING:
> +            val = libvirt_constcharPtrWrap(params[i].value.s);
> +            break;
> +
> +        default:
> +            return 0;

If we get here, we have a coding error.  In fact, if we code things
correctly, we can let the compiler tell us if a new VIR_TYPED_PARAM is
ever added, where we need to cover it in our switch statement.

> +        }
> +
> +        key = libvirt_constcharPtrWrap(params[i].field);
> +        if (!key || !val)
> +            goto cleanup;
> +
> +        if (PyDict_SetItem(*info, key, val) < 0)
> +            goto cleanup;
> +
> +        Py_DECREF(key);
> +        Py_DECREF(val);
> +    }
> +    return 1;

I'd return 0 here, for consistency with the rest of libvirt conventions.

> +cleanup:
> +    Py_XDECREF(key);
> +    Py_XDECREF(val);
> +    return -1;
> +}
> +
> +/* 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 virTypedParameterPtr ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> +setPyVirTypedParameter(PyObject *info,
> +                       const virTypedParameterPtr params, int nparams)
> +{
> +    PyObject *key, *value;
> +    Py_ssize_t pos = 0;
> +    virTypedParameterPtr temp = NULL, ret = NULL;
> +    int size, i;
> +
> +    if ((size = PyDict_Size(info)) < 0)
> +        return NULL;

Here, we should also reject a size of 0, with a python exception, since
the setter APIs should not be called with a non-NULL array but a 0 size.

We could also take a shortcut an fail if size > nparams, since that
means at least one entry in the python dictionary is invalid; but
erroring out now would not give a very nice message.

> +
> +    if (VIR_ALLOC_N(ret, size) < 0) {
> +        PyErr_NoMemory();
> +        return NULL;
> +    }
> +
> +    temp = &ret[0];
> +    while (PyDict_Next(info, &pos, &key, &value)) {
> +        char *keystr = NULL;
> +        bool found = false;
> +
> +        if (PyString_Check(key)) {
> +            if ((keystr = PyString_AsString(key)) == NULL)
> +                goto cleanup;
> +        } else {
> +            PyErr_Format(PyExc_TypeError,
> +                            "Attribut name must be string");

s/Attribut/Attribute/

> +            goto cleanup;
> +        }

We can let python raise the exception on our behalf.

> +
> +        for (i = 0; i < nparams; i++) {
> +            if (STREQ(params[i].field, keystr)) {

Indentation got hard here.  A little refactoring to reject unfound keys
first makes life a bit easier.

> +                found = true;
> +                strncpy(temp->field, params[i].field,
> +                        sizeof(temp->field));

'make syntax-check' didn't like you:

prohibit_strncpy
python/libvirt-override.c:164:                strncpy(temp->field,
params[i].field,
maint.mk: use virStrncpy, not strncpy


> +                temp->type = params[i].type;
> +
> +                switch(params[i].type) {
> +                case VIR_TYPED_PARAM_INT:
> +                    {
> +                        long long_val;
> +                        if (PyInt_Check(value)) {
> +                            long_val = PyInt_AsLong(value);
> +                            if ((long_val == -1) && PyErr_Occurred())
> +                                goto cleanup;
> +                        } else {
> +                            PyErr_Format(PyExc_TypeError,
> +                                        "The value type of "
> +                                        "attribute \"%s\" must be int", keystr);
> +                            goto cleanup;
> +                        }

I still think we can let python throw the exception for us.

> +
> +                        if ((int)long_val == long_val) {
> +                            temp->value.i = (int)long_val;

Useless cast on the second line.  The first one is necessary, though.


> +                case VIR_TYPED_PARAM_BOOLEAN:
> +                    {
> +                        /* Hack - Python's definition of Py_True breaks strict
> +                         * aliasing rules, so can't directly compare
> +                         */
> +                        if (PyBool_Check(value)) {
> +                            PyObject *hacktrue = PyBool_FromLong(1);
> +                            temp->value.b = hacktrue == value ? 1: 0;

Formatting: space between ?: characters in the ternary operator.

> +                            Py_DECREF(hacktrue);
> +                        } else {
> +                            PyErr_Format(PyExc_TypeError,
> +                                         "The value type of "
> +                                         "attribute \"%s\" must be bool", keystr);
> +                            goto cleanup;
> +                        }
> +                    }
> +                    break;
> +                case VIR_TYPED_PARAM_STRING:
> +                    {
> +                        char *string_val;
> +                        if (PyString_Check(value)) {
> +                            if ((string_val = PyString_AsString(value)) == NULL)
> +                                goto cleanup;
> +                            if ((temp->value.s = strdup(string_val)) == NULL) {

If we don't strdup() here, then we have one less point of allocation
failure, and callers merely need remember to _not_ clear the returned
array on success.

> +                                PyErr_NoMemory();
> +                                goto cleanup;
> +                            }
> +                        } else {
> +                            PyErr_Format(PyExc_TypeError,
> +                                         "The value type of "
> +                                         "attribute \"%s\" must be string", keystr);
> +                            goto cleanup;
> +                        }
> +                    }
> +                    break;
> +                default:
> +                    goto cleanup;
> +                }
> +            }
> +        }
> +
> +        if (i == nparams && !found) {

This is redundant - found will always be true iff we broke out of the
for loop before setting i to nparams.  I'm hoisting this condition earlier.

> +                PyErr_Format(PyExc_LookupError,
> +                            "Attribute name \"%s\" could not be recognized", keystr);
> +                goto cleanup;
> +        }
> +
> +        temp++;
> +    }
> +    return ret;
> +
> +cleanup:
> +    virTypedParameterArrayClear(ret, size);
> +    VIR_FREE(ret);
> +    return NULL;
> +}
> +
>  /************************************************************************
>   *									*
>   *		Statistics						*
> @@ -1007,6 +1265,131 @@ libvirt_virDomainGetMemoryParameters(PyObject *self ATTRIBUTE_UNUSED,
>  }
>  
>  static PyObject *
> +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> +                                   PyObject *args)
> +{
> +    virDomainPtr domain;
> +    PyObject *pyobj_domain, *info;
> +    PyObject *ret = NULL;
> +    int i_retval;
> +    int nparams = 0, size = 0;
> +    unsigned int flags;
> +    virTypedParameterPtr params, new_params;
> +
> +    if (!PyArg_ParseTuple(args,
> +                          (char *)"OOi:virDomainSetNumaParameters",
> +                          &pyobj_domain, &info, &flags))
> +        return NULL;
> +    domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
> +
> +    if ((size = PyDict_Size(info)) < 0)
> +        return NULL;
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +
> +    if (i_retval < 0 || !nparams)
> +        return VIR_PY_INT_FAIL;

If i_retval < 0, then libvirt has set an error message.  But if nparams
is 0, then we need to raise a message.

> +
> +    if (VIR_ALLOC_N(params, nparams) < 0)
> +        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;
> +        goto cleanup;
> +    }
> +
> +    new_params = setPyVirTypedParameter(info, params, nparams);
> +    if (!new_params)
> +        goto cleanup;
> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    i_retval = virDomainSetNumaParameters(domain, new_params, size, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +
> +    if (i_retval < 0) {
> +        ret = VIR_PY_INT_FAIL;
> +        goto cleanup;
> +    }
> +
> +    ret = VIR_PY_INT_SUCCESS;

Looks good.

> +
> +cleanup:
> +    virTypedParameterArrayClear(params, nparams);
> +    VIR_FREE(params);
> +    virTypedParameterArrayClear(new_params, size);
> +    VIR_FREE(new_params);

See above comments about reducing malloc pressure; omitting strdup above
means we can omit the array clear here.

> +    return ret;
> +}
> +
> +static PyObject *
> +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
> +                                   PyObject *args)
> +{
> +    virDomainPtr domain;
> +    PyObject *pyobj_domain, *info;
> +    PyObject *ret = NULL;
> +    int i_retval;
> +    int nparams = 0;
> +    unsigned int flags;
> +    virTypedParameterPtr params;
> +
> +    if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainGetNumaParameters",
> +                          &pyobj_domain, &flags))
> +        return NULL;
> +    domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);
> +
> +    if ((info = PyDict_New()) == NULL) {
> +        return ret;
> +    }

Allocation...

> +
> +    LIBVIRT_BEGIN_ALLOW_THREADS;
> +    i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
> +    LIBVIRT_END_ALLOW_THREADS;
> +
> +    if (i_retval < 0)
> +        return VIR_PY_NONE;

...and memory leak.  I've made up my mind - it's better to have the
helper function allocate info.

> +
> +    if (!nparams)
> +        return info;

Here, we can do a one-shot allocation, since we aren't going to reach
the helper.

> +
> +    if (VIR_ALLOC_N(params, nparams) < 0)
> +        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_NONE;
> +        goto cleanup;
> +    }
> +
> +    i_retval = getPyVirTypedParameter(&info, params, nparams);
> +
> +    if (i_retval < 0) {
> +        Py_XDECREF(info);
> +        goto cleanup;

If the helper allocates info on success, then the helper can also free
it before returning failure, making our life easier.  In fact, changing
the helper's signature will make it even easier to use.

> +    } else if (i_retval == 0) {
> +        Py_XDECREF(info);
> +        ret = VIR_PY_NONE;
> +        goto cleanup;

Returning a 0-entry info is okay here.

> +    } else {
> +        ret = info;
> +    }
> +
> +cleanup:
> +    virTypedParameterArrayClear(params, nparams);
> +    VIR_FREE(params);
> +    return ret;
> +}
> +
> +static PyObject *
>  libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
>                            PyObject *args) {
>      virDomainPtr domain;
> @@ -5203,6 +5586,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 *) "virDomainSetNumaParameters", libvirt_virDomainSetNumaParameters, METH_VARARGS, NULL},
> +    {(char *) "virDomainGetNumaParameters", libvirt_virDomainGetNumaParameters, METH_VARARGS, NULL},

Here's the incremental diff I'm testing:

diff --git i/python/libvirt-override.c w/python/libvirt-override.c
index 3c6020b..d424f19 100644
--- i/python/libvirt-override.c
+++ w/python/libvirt-override.c
@@ -16,6 +16,9 @@
    which has over 180 autoconf-style HAVE_* definitions.  Shame on
them.  */
 #undef HAVE_PTHREAD_H

+/* We want to see *_LAST enums.  */
+#define VIR_ENUM_SENTINELS
+
 #include <Python.h>
 #include "libvirt/libvirt.h"
 #include "libvirt/virterror.h"
@@ -23,6 +26,8 @@
 #include "libvirt.h"
 #include "memory.h"
 #include "virtypedparam.h"
+#include "ignore-value.h"
+#include "util.h"

 #ifndef __CYGWIN__
 extern void initlibvirtmod(void);
@@ -63,46 +68,50 @@ 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 int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-getPyVirTypedParameter(PyObject **info,
-                       const virTypedParameterPtr params, int nparams)
+/* Helper function to convert a virTypedParameter output array into a
+ * Python dictionary for return to the user.  Return NULL on failure,
+ * after raising a python exception.  */
+static PyObject * ATTRIBUTE_NONNULL(1)
+getPyVirTypedParameter(const virTypedParameterPtr params, int nparams)
 {
-    PyObject *key, *val;
+    PyObject *key, *val, *info;
     int i;

+    if ((info = PyDict_New()) == NULL)
+        return NULL;
+
     for (i = 0 ; i < nparams ; i++) {
-        switch (params[i].type) {
+        switch ((virTypedParameterType) params[i].type) {
         case VIR_TYPED_PARAM_INT:
-            val = PyInt_FromLong((long)params[i].value.i);
+            val = PyInt_FromLong(params[i].value.i);
             break;

         case VIR_TYPED_PARAM_UINT:
-            val = PyInt_FromLong((unsigned long)params[i].value.ui);
+            val = PyInt_FromLong(params[i].value.ui);
             break;

         case VIR_TYPED_PARAM_LLONG:
-            val = PyLong_FromLongLong((long long)params[i].value.l);
+            val = PyLong_FromLongLong(params[i].value.l);
             break;

         case VIR_TYPED_PARAM_ULLONG:
-            val = PyLong_FromUnsignedLongLong((unsigned long
long)params[i].value.ul);
+            val = PyLong_FromUnsignedLongLong(params[i].value.ul);
             break;

         case VIR_TYPED_PARAM_DOUBLE:
-            val = PyFloat_FromDouble((double)params[i].value.d);
+            val = PyFloat_FromDouble(params[i].value.d);
             break;

         case VIR_TYPED_PARAM_BOOLEAN:
-            val = PyBool_FromLong((long)params[i].value.b);
+            val = PyBool_FromLong(params[i].value.b);
             break;

         case VIR_TYPED_PARAM_STRING:
             val = libvirt_constcharPtrWrap(params[i].value.s);
             break;

-        default:
+        case VIR_TYPED_PARAM_LAST:
+            /* Shouldn't get here */
             return 0;
         }

@@ -110,23 +119,28 @@ getPyVirTypedParameter(PyObject **info,
         if (!key || !val)
             goto cleanup;

-        if (PyDict_SetItem(*info, key, val) < 0)
+        if (PyDict_SetItem(info, key, val) < 0) {
+            Py_DECREF(info);
             goto cleanup;
+        }

         Py_DECREF(key);
         Py_DECREF(val);
     }
-    return 1;
+    return info;
+
 cleanup:
     Py_XDECREF(key);
     Py_XDECREF(val);
-    return -1;
+    return NULL;
 }

 /* 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. */
+ * hints on what types to use when creating the new array. The caller
+ * must NOT clear the array before freeing it, as it points into info
+ * rather than allocated strings.  Return NULL on failure, after
+ * raising a python exception.  */
 static virTypedParameterPtr ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 setPyVirTypedParameter(PyObject *info,
                        const virTypedParameterPtr params, int nparams)
@@ -139,6 +153,13 @@ setPyVirTypedParameter(PyObject *info,
     if ((size = PyDict_Size(info)) < 0)
         return NULL;

+    /* Libvirt APIs use NULL array and 0 size as a special case;
+     * setting should have at least one parameter.  */
+    if (size == 0) {
+        PyErr_Format(PyExc_LookupError, "Dictionary must not be empty");
+        return NULL;
+    }
+
     if (VIR_ALLOC_N(ret, size) < 0) {
         PyErr_NoMemory();
         return NULL;
@@ -147,167 +168,108 @@ setPyVirTypedParameter(PyObject *info,
     temp = &ret[0];
     while (PyDict_Next(info, &pos, &key, &value)) {
         char *keystr = NULL;
-        bool found = false;

-        if (PyString_Check(key)) {
-            if ((keystr = PyString_AsString(key)) == NULL)
-                goto cleanup;
-        } else {
-            PyErr_Format(PyExc_TypeError,
-                            "Attribut name must be string");
+        if ((keystr = PyString_AsString(key)) == NULL)
             goto cleanup;
-        }

         for (i = 0; i < nparams; i++) {
-            if (STREQ(params[i].field, keystr)) {
-                found = true;
-                strncpy(temp->field, params[i].field,
-                        sizeof(temp->field));
-                temp->type = params[i].type;
-
-                switch(params[i].type) {
-                case VIR_TYPED_PARAM_INT:
-                    {
-                        long long_val;
-                        if (PyInt_Check(value)) {
-                            long_val = PyInt_AsLong(value);
-                            if ((long_val == -1) && PyErr_Occurred())
-                                goto cleanup;
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                        "The value type of "
-                                        "attribute \"%s\" must be int",
keystr);
-                            goto cleanup;
-                        }
-
-                        if ((int)long_val == long_val) {
-                            temp->value.i = (int)long_val;
-                        } else {
-                            PyErr_Format(PyExc_ValueError,
-                                         "The value of "
-                                         "attribute \"%s\" is out of
int range", keystr);
-                            goto cleanup;
-                       }
-                    }
-                    break;
-                case VIR_TYPED_PARAM_UINT:
-                    {
-                        long long_val;
-                        if (PyInt_Check(value)) {
-                            long_val = PyInt_AsLong(value);
-                            if ((long_val == -1) && PyErr_Occurred())
-                                goto cleanup;
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                         "The value type of "
-                                         "attribute \"%s\" must be
int", keystr);
-                            goto cleanup;
-                        }
-
-                        if ((unsigned int)long_val == long_val) {
-                            temp->value.ui = (unsigned int)long_val;
-                        } else {
-                            PyErr_Format(PyExc_ValueError,
-                                         "The value of "
-                                         "attribute \"%s\" is out of
int range", keystr);
-                            goto cleanup;
-                        }
-                    }
-                    break;
-                case VIR_TYPED_PARAM_LLONG:
-                    {
-                        long long llong_val;
-                        if (PyLong_Check(value)) {
-                            llong_val = PyLong_AsLongLong(value);
-                            if ((llong_val == -1) && PyErr_Occurred())
-                                goto cleanup;
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                         "The value type of "
-                                         "attribute \"%s\" must be
long", keystr);
-                            goto cleanup;
-                        }
-                        temp->value.l = llong_val;
-                    }
-                    break;
-                case VIR_TYPED_PARAM_ULLONG:
-                    {
-                        unsigned long long ullong_val;
-                        if (PyLong_Check(value)) {
-                            ullong_val = PyLong_AsUnsignedLongLong(value);
-                            if ((ullong_val == -1) && PyErr_Occurred())
-                                goto cleanup;
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                         "The value type of "
-                                         "attribute \"%s\" must be
long", keystr);
-                            goto cleanup;
-
-                        }
-                        temp->value.ul = ullong_val;
-                    }
-                    break;
-                case VIR_TYPED_PARAM_DOUBLE:
-                    {
-                        double double_val;
-                        if (PyFloat_Check(value)) {
-                            double_val = PyFloat_AsDouble(value);
-                            if ((double_val == -1) && PyErr_Occurred())
-                                goto cleanup;
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                        "The value type of "
-                                        "attribute \"%s\" must be
float", keystr);
-                           goto cleanup;
-                        }
-                        temp->value.d = double_val;
-                    }
-                    break;
-                case VIR_TYPED_PARAM_BOOLEAN:
-                    {
-                        /* Hack - Python's definition of Py_True breaks
strict
-                         * aliasing rules, so can't directly compare
-                         */
-                        if (PyBool_Check(value)) {
-                            PyObject *hacktrue = PyBool_FromLong(1);
-                            temp->value.b = hacktrue == value ? 1: 0;
-                            Py_DECREF(hacktrue);
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                         "The value type of "
-                                         "attribute \"%s\" must be
bool", keystr);
-                            goto cleanup;
-                        }
-                    }
-                    break;
-                case VIR_TYPED_PARAM_STRING:
-                    {
-                        char *string_val;
-                        if (PyString_Check(value)) {
-                            if ((string_val = PyString_AsString(value))
== NULL)
-                                goto cleanup;
-                            if ((temp->value.s = strdup(string_val)) ==
NULL) {
-                                PyErr_NoMemory();
-                                goto cleanup;
-                            }
-                        } else {
-                            PyErr_Format(PyExc_TypeError,
-                                         "The value type of "
-                                         "attribute \"%s\" must be
string", keystr);
-                            goto cleanup;
-                        }
-                    }
-                    break;
-                default:
-                    goto cleanup;
-                }
-            }
+            if (STREQ(params[i].field, keystr))
+                break;
         }
+        if (i == nparams) {
+            PyErr_Format(PyExc_LookupError,
+                         "Attribute name \"%s\" could not be recognized",
+                         keystr);
+            goto cleanup;
+        }
+
+        ignore_value(virStrcpyStatic(temp->field, keystr));
+        temp->type = params[i].type;

-        if (i == nparams && !found) {
-                PyErr_Format(PyExc_LookupError,
-                            "Attribute name \"%s\" could not be
recognized", keystr);
+        switch((virTypedParameterType) params[i].type) {
+        case VIR_TYPED_PARAM_INT:
+        {
+            long long_val = PyInt_AsLong(value);
+            if ((long_val == -1) && PyErr_Occurred())
+                goto cleanup;
+            if ((int)long_val == long_val) {
+                temp->value.i = long_val;
+            } else {
+                PyErr_Format(PyExc_ValueError,
+                             "The value of "
+                             "attribute \"%s\" is out of int range",
keystr);
+                goto cleanup;
+            }
+        }
+        break;
+        case VIR_TYPED_PARAM_UINT:
+        {
+            long long_val = PyInt_AsLong(value);
+            if ((long_val == -1) && PyErr_Occurred())
+                goto cleanup;
+            if ((unsigned int)long_val == long_val) {
+                temp->value.ui = long_val;
+            } else {
+                PyErr_Format(PyExc_ValueError,
+                             "The value of "
+                             "attribute \"%s\" is out of int range",
keystr);
+                goto cleanup;
+            }
+        }
+        break;
+        case VIR_TYPED_PARAM_LLONG:
+        {
+            long long llong_val = PyLong_AsLongLong(value);
+            if ((llong_val == -1) && PyErr_Occurred())
                 goto cleanup;
+            temp->value.l = llong_val;
+        }
+        break;
+        case VIR_TYPED_PARAM_ULLONG:
+        {
+            unsigned long long ullong_val =
PyLong_AsUnsignedLongLong(value);
+            if ((ullong_val == -1) && PyErr_Occurred())
+                goto cleanup;
+            temp->value.ul = ullong_val;
+        }
+        break;
+        case VIR_TYPED_PARAM_DOUBLE:
+        {
+            double double_val = PyFloat_AsDouble(value);
+            if ((double_val == -1) && PyErr_Occurred())
+                goto cleanup;
+            temp->value.d = double_val;
+        }
+        break;
+        case VIR_TYPED_PARAM_BOOLEAN:
+        {
+            /* Hack - Python's definition of Py_True breaks strict
+             * aliasing rules, so can't directly compare
+             */
+            if (PyBool_Check(value)) {
+                PyObject *hacktrue = PyBool_FromLong(1);
+                temp->value.b = hacktrue == value ? 1 : 0;
+                Py_DECREF(hacktrue);
+            } else {
+                PyErr_Format(PyExc_TypeError,
+                             "The value type of "
+                             "attribute \"%s\" must be bool", keystr);
+                goto cleanup;
+            }
+        }
+        break;
+        case VIR_TYPED_PARAM_STRING:
+        {
+            char *string_val = PyString_AsString(value);
+            if (!string_val)
+                goto cleanup;
+            temp->value.s = string_val;
+            break;
+        }
+
+        case VIR_TYPED_PARAM_LAST:
+            /* Shouldn't get here */
+            goto cleanup;
         }

         temp++;
@@ -315,7 +277,6 @@ setPyVirTypedParameter(PyObject *info,
     return ret;

 cleanup:
-    virTypedParameterArrayClear(ret, size);
     VIR_FREE(ret);
     return NULL;
 }
@@ -1289,9 +1250,15 @@ libvirt_virDomainSetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
     i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
     LIBVIRT_END_ALLOW_THREADS;

-    if (i_retval < 0 || !nparams)
+    if (i_retval < 0)
         return VIR_PY_INT_FAIL;

+    if (size == 0) {
+        PyErr_Format(PyExc_LookupError,
+                     "Domain has no settable attributes");
+        return NULL;
+    }
+
     if (VIR_ALLOC_N(params, nparams) < 0)
         return PyErr_NoMemory();

@@ -1322,7 +1289,6 @@ libvirt_virDomainSetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
 cleanup:
     virTypedParameterArrayClear(params, nparams);
     VIR_FREE(params);
-    virTypedParameterArrayClear(new_params, size);
     VIR_FREE(new_params);
     return ret;
 }
@@ -1332,7 +1298,7 @@ libvirt_virDomainGetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
                                    PyObject *args)
 {
     virDomainPtr domain;
-    PyObject *pyobj_domain, *info;
+    PyObject *pyobj_domain;
     PyObject *ret = NULL;
     int i_retval;
     int nparams = 0;
@@ -1344,10 +1310,6 @@ libvirt_virDomainGetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
         return NULL;
     domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain);

-    if ((info = PyDict_New()) == NULL) {
-        return ret;
-    }
-
     LIBVIRT_BEGIN_ALLOW_THREADS;
     i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
     LIBVIRT_END_ALLOW_THREADS;
@@ -1356,7 +1318,7 @@ libvirt_virDomainGetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
         return VIR_PY_NONE;

     if (!nparams)
-        return info;
+        return PyDict_New();

     if (VIR_ALLOC_N(params, nparams) < 0)
         return PyErr_NoMemory();
@@ -1370,18 +1332,7 @@ libvirt_virDomainGetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
         goto cleanup;
     }

-    i_retval = getPyVirTypedParameter(&info, params, nparams);
-
-    if (i_retval < 0) {
-        Py_XDECREF(info);
-        goto cleanup;
-    } else if (i_retval == 0) {
-        Py_XDECREF(info);
-        ret = VIR_PY_NONE;
-        goto cleanup;
-    } else {
-        ret = info;
-    }
+    ret = getPyVirTypedParameter(params, nparams);

 cleanup:
     virTypedParameterArrayClear(params, nparams);


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]