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&lt;gren(a)redhat.com&gt; >> >> *getPyVirTypedParameter >> *setPyVirTypedParameter >> *virDomainSetNumaParameters >> *virDomainGetNumaParameters >> >> Signed-off-by: Eric Blake&lt;eblake(a)redhat.com&gt; >> --- >> >> 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?