[Libvir] python/libvir.c returning wrong value on error?

Hi all, Looking at python/libvir.c, all the custom functions return VIR_PY_NONE on error. This unfortunately doesn't map well to some of the generated python bindings which expect an error val of -1. So if these commands fail, no exception will be thrown at the python level. (Ex. virDomainGetAutostart, virDomainGetVcpus and their python equiv.) I'm wondering where the fix should be: in python/libvir.c, changing these error return values, or in the generator somewhere? Thanks, Cole

On Wed, Feb 06, 2008 at 03:18:13PM -0500, Cole Robinson wrote:
Hi all,
Looking at python/libvir.c, all the custom functions return VIR_PY_NONE on error. This unfortunately doesn't map well to some of the generated python bindings which expect an error val of -1. So if these commands fail, no exception will be thrown at the python level. (Ex. virDomainGetAutostart, virDomainGetVcpus and their python equiv.)
I'm wondering where the fix should be: in python/libvir.c, changing these error return values, or in the generator somewhere?
The fix should be in the libvir.c file - I didn't write this custom bindings correctly. Simply remove the line if (c_retval < 0) return VIR_PY_NONE; So 'c_retval' will get propagated back to the pure python layer which will do the -1 check Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

Daniel P. Berrange wrote:
On Wed, Feb 06, 2008 at 03:18:13PM -0500, Cole Robinson wrote:
Hi all,
Looking at python/libvir.c, all the custom functions return VIR_PY_NONE on error. This unfortunately doesn't map well to some of the generated python bindings which expect an error val of -1. So if these commands fail, no exception will be thrown at the python level. (Ex. virDomainGetAutostart, virDomainGetVcpus and their python equiv.)
I'm wondering where the fix should be: in python/libvir.c, changing these error return values, or in the generator somewhere?
The fix should be in the libvir.c file - I didn't write this custom bindings correctly. Simply remove the line
if (c_retval < 0) return VIR_PY_NONE;
So 'c_retval' will get propagated back to the pure python layer which will do the -1 check
Regards, Dan.
Hmm, I don't think any of the cases where there is an error actually end up returning c_retval, its always python wrapped contents of something passed by reference, so I don't know how that would work. The patch below seems to do the job: I tested it with the autostart function on a xen domain (which errors as its not supported) and it now correctly throws an exception from the python bindings. Thanks, Cole diff --git a/python/libvir.c b/python/libvir.c index 96555b3..67d6252 100644 --- a/python/libvir.c +++ b/python/libvir.c @@ -27,6 +27,8 @@ extern void initcygvirtmod(void); is so common that we encapsulate it here. Now, each use is simply return VIR_PY_NONE; */ #define VIR_PY_NONE (Py_INCREF (Py_None), Py_None) +#define VIR_PY_INT_FAIL (libvirt_intWrap(-1)) +#define VIR_PY_INT_SUCCESS (libvirt_intWrap(0)) /************************************************************************ * * @@ -214,15 +216,15 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self ATTRIBUTE_UNUSED, c_retval = virDomainGetSchedulerType(domain, &nparams); if (c_retval == NULL) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; free(c_retval); if ((params = malloc(sizeof(*params)*nparams)) == NULL) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; if (virDomainGetSchedulerParameters(domain, params, &nparams) < 0) { free(params); - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; } /* convert to a Python tupple of long objects */ @@ -269,17 +271,17 @@ libvirt_virDomainSetSchedulerParameters(PyObject *self ATTRIBUTE_UNUSED, default: free(params); - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; } } if (virDomainSetSchedulerParameters(domain, params, nparams) < 0) { free(params); - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; } free(params); - return VIR_PY_NONE; + return VIR_PY_INT_SUCCESS; } static PyObject * @@ -383,11 +385,11 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED, domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); if (virNodeGetInfo(virDomainGetConnect(domain), &nodeinfo) != 0) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; cpumaplen = VIR_CPU_MAPLEN(VIR_NODEINFO_MAXCPUS(nodeinfo)); if ((cpumap = malloc(cpumaplen)) == NULL) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; memset(cpumap, 0, cpumaplen); truth = PyBool_FromLong(1); @@ -403,7 +405,7 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED, Py_DECREF(truth); free(cpumap); - return VIR_PY_NONE; + return VIR_PY_INT_SUCCESS; } @@ -1030,7 +1032,7 @@ libvirt_virDomainGetAutostart(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; py_retval = libvirt_intWrap(autostart); return(py_retval); } @@ -1053,7 +1055,7 @@ libvirt_virNetworkGetAutostart(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) - return VIR_PY_NONE; + return VIR_PY_INT_FAIL; py_retval = libvirt_intWrap(autostart); return(py_retval); }

On Wed, Feb 06, 2008 at 05:35:09PM -0500, Cole Robinson wrote:
Daniel P. Berrange wrote:
On Wed, Feb 06, 2008 at 03:18:13PM -0500, Cole Robinson wrote:
Hi all,
Looking at python/libvir.c, all the custom functions return VIR_PY_NONE on error. This unfortunately doesn't map well to some of the generated python bindings which expect an error val of -1. So if these commands fail, no exception will be thrown at the python level. (Ex. virDomainGetAutostart, virDomainGetVcpus and their python equiv.)
I'm wondering where the fix should be: in python/libvir.c, changing these error return values, or in the generator somewhere?
The fix should be in the libvir.c file - I didn't write this custom bindings correctly. Simply remove the line
if (c_retval < 0) return VIR_PY_NONE;
So 'c_retval' will get propagated back to the pure python layer which will do the -1 check
Regards, Dan.
Hmm, I don't think any of the cases where there is an error actually end up returning c_retval, its always python wrapped contents of something passed by reference, so I don't know how that would work.
The patch below seems to do the job: I tested it with the autostart function on a xen domain (which errors as its not supported) and it now correctly throws an exception from the python bindings.
Okay, this is similar to changing a void public API to an int returning one, it makes sense, and should be no problem, patch is very clean, so great ! I applied it and commited it to CVS, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Cole Robinson
-
Daniel P. Berrange
-
Daniel Veillard