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