5:22 p.m.
On 02/08/2012 09:29 PM, Guannan Ren wrote:
On 02/09/2012 09:41 AM, Eric Blake wrote:
> From: Guannan Ren<gren(a)redhat.com>
>
> *getPyVirTypedParameter
> *setPyVirTypedParameter
> *virDomainSetNumaParameters
> *virDomainGetNumaParameters
>
> Signed-off-by: Eric Blake<eblake(a)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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org