On 02/08/2012 09:29 PM, Guannan Ren wrote:
> On 02/09/2012 09:41 AM, Eric Blake wrote:
>> From: Guannan Ren<gren@xxxxxxxxxx>
>>
>> *getPyVirTypedParameter
>> *setPyVirTypedParameter
>> *virDomainSetNumaParameters
>> *virDomainGetNumaParameters
>>
>> Signed-off-by: Eric Blake<eblake@xxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list