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

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



On 02/08/2012 09:29 PM, Guannan Ren wrote:
> On 02/09/2012 09:41 AM, Eric Blake wrote:
>> From: Guannan Ren<gren redhat com>
>>
>>            *getPyVirTypedParameter
>>            *setPyVirTypedParameter
>>            *virDomainSetNumaParameters
>>            *virDomainGetNumaParameters
>>
>> Signed-off-by: Eric Blake<eblake redhat com>
>> ---
>>
>> v5: Incorporate my review comments on v4
>>

>> +    for (i = 0 ; i<  nparams ; i++) {
>> +        switch ((virTypedParameterType) params[i].type) {
>> +        case VIR_TYPED_PARAM_INT:
>> +            val = PyInt_FromLong(params[i].value.i);
>> +            break;
>> +

>> +
>> +        case VIR_TYPED_PARAM_LAST:
>> +            /* Shouldn't get here */
>> +            return 0;
>> +        }
> 
>               The effect of return 0 is the same as return NULL that will
>               trigger the exception if defined before.
>               Otherwise if the exception is not defined, the exception
> is as follows:
>               "System Error: error return without exception set"
>               In the case of having compiler to check out, it's ok here.
> 

Hmm; originally, I was returning 0 on success and -1 on failure, then I
changed the signature to return NULL on failure and object on success,
but not this line.  0 acts like NULL, but it would be better written as
NULL.

At any rate, my trick of doing
switch ((virTypedParameterType) params[i].type)
will ensure that at least gcc, with warnings, will warn us if we ever
miss any cases known at compile time.  Conversely, if we are talking to
a newer server that knows a new type, we should not silently reject it,
so this case really does need an error message, at which point we want
to have a default handler, at which point my hack no longer helps (gcc
only warns about uncovered enum values if there is no default case).
I'll fix it.

>>   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)
>> +        return VIR_PY_INT_FAIL;
>> +
>> +    if (size == 0) {
>> +        PyErr_Format(PyExc_LookupError,
>> +                     "Domain has no settable attributes");
>> +        return NULL;
>> +    }
> 
>         A typo,  "if (nparams == 0)"

Good catch.

> 
>        Thanks for these comments,  ACK.

I'm squashing this in, then pushing.  Are you still planning on
retro-fitting other virTypedParam callers to use the new helper functions?

diff --git i/python/libvirt-override.c w/python/libvirt-override.c
index cb75cbe..e7c2bd5 100644
--- i/python/libvirt-override.c
+++ w/python/libvirt-override.c
@@ -81,7 +81,7 @@ getPyVirTypedParameter(const virTypedParameterPtr
params, int nparams)
         return NULL;

     for (i = 0 ; i < nparams ; i++) {
-        switch ((virTypedParameterType) params[i].type) {
+        switch (params[i].type) {
         case VIR_TYPED_PARAM_INT:
             val = PyInt_FromLong(params[i].value.i);
             break;
@@ -110,9 +110,14 @@ getPyVirTypedParameter(const virTypedParameterPtr
params, int nparams)
             val = libvirt_constcharPtrWrap(params[i].value.s);
             break;

-        case VIR_TYPED_PARAM_LAST:
-            /* Shouldn't get here */
-            return 0;
+        default:
+            /* Possible if a newer server has a bug and sent stuff we
+             * don't recognize.  */
+            PyErr_Format(PyExc_LookupError,
+                         "Type value \"%d\" not recognized",
+                         params[i].type);
+            val = NULL;
+            break;
         }

         key = libvirt_constcharPtrWrap(params[i].field);
@@ -186,7 +191,7 @@ setPyVirTypedParameter(PyObject *info,
         ignore_value(virStrcpyStatic(temp->field, keystr));
         temp->type = params[i].type;

-        switch((virTypedParameterType) params[i].type) {
+        switch(params[i].type) {
         case VIR_TYPED_PARAM_INT:
         {
             long long_val = PyInt_AsLong(value);
@@ -267,8 +272,12 @@ setPyVirTypedParameter(PyObject *info,
             break;
         }

-        case VIR_TYPED_PARAM_LAST:
-            /* Shouldn't get here */
+        default:
+            /* Possible if a newer server has a bug and sent stuff we
+             * don't recognize.  */
+            PyErr_Format(PyExc_LookupError,
+                         "Type value \"%d\" not recognized",
+                         params[i].type);
             goto cleanup;
         }

@@ -1246,6 +1255,12 @@ libvirt_virDomainSetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
     if ((size = PyDict_Size(info)) < 0)
         return NULL;

+    if (size == 0) {
+        PyErr_Format(PyExc_LookupError,
+                     "Need non-empty dictionary to set attributes");
+        return NULL;
+    }
+
     LIBVIRT_BEGIN_ALLOW_THREADS;
     i_retval = virDomainGetNumaParameters(domain, NULL, &nparams, flags);
     LIBVIRT_END_ALLOW_THREADS;
@@ -1253,7 +1268,7 @@ libvirt_virDomainSetNumaParameters(PyObject *self
ATTRIBUTE_UNUSED,
     if (i_retval < 0)
         return VIR_PY_INT_FAIL;

-    if (size == 0) {
+    if (nparams == 0) {
         PyErr_Format(PyExc_LookupError,
                      "Domain has no settable attributes");
         return NULL;

-- 
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]