[Libvir] [PATCH] Python bindings now generate exceptions for libvirt errors
Following on from this thread: https://www.redhat.com/archives/libvir-list/2007-March/thread.html#00341 The first attachment is a patch which changes the generated Python bindings so that they always raise an exception when an underlying error occurs in the C libvirt library. The second attachment is for information only (not to be applied) - it shows the differences in the generated file libvirtclass.py. I have checked each of these changes by hand against both the libvirt.c interface description and the C implementation of the Python bindings (libvir.c and libvirt-py.c), and each change seems correct. I have _not_ tested this against virt-manager. It is quite possible that virt-manager will cease to work if it wasn't checking the return values from affected functions carefully. I'll check this later today and supply a patch (to virt-manager) if necessary. I have some additional questions about the Python bindings, but I'll put those in a separate thread. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
Second version of this patch. The difference is that this one adds the optional argument libvirtError (..., conn=self) for functions which are members of the virConnect class. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
Third version of this patch, which should allow virt-manager to start up correctly*. virt-manager relies on virDomainGetID returning -1 in the case when a domain is inactive. On further investigation it turns out that virDomainGetID and virDomainGetName can never fail**, since all they do is read fields from the domain pointer that you pass. It was also claimed that virDomainGetUUID was also error-free, but that's actually not the case. So this patch disables exceptions in those two functions only. Note that the documentation for virDomainGetID is wrong. Thanks to Hugh Brock and Dan Berrange for finding and diagnosing the problem. Rich. * Not tested: for me virt-manager fails for another reason, with or without this patch. ** Well, they can fail in the case where you've corrupted memory and your virDomainPtr isn't really a virDomainPtr, but at that point all bets are off anyway ... -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Wed, Mar 28, 2007 at 10:23:06AM +0100, Richard W.M. Jones wrote:
Third version of this patch, which should allow virt-manager to start up correctly*.
virt-manager relies on virDomainGetID returning -1 in the case when a domain is inactive. On further investigation it turns out that virDomainGetID and virDomainGetName can never fail**, since all they do
they fail if the pointer is wrong, which is a very possible case from a C API point of view.
is read fields from the domain pointer that you pass. It was also claimed that virDomainGetUUID was also error-free, but that's actually not the case.
So this patch disables exceptions in those two functions only.
If you are 100% sure you can't pass a wrong pointer though the python binding then okay.
Note that the documentation for virDomainGetID is wrong.
I don't see the error
** Well, they can fail in the case where you've corrupted memory and your virDomainPtr isn't really a virDomainPtr, but at that point all bets are off anyway ...
Disagreeing, passing a NULL pointer can happen if the user code made a mistake, or relied on libvirt to catch it up. 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/
Daniel Veillard wrote:
So this patch disables exceptions in those two functions only.
If you are 100% sure you can't pass a wrong pointer though the python binding then okay.
OK so the problem here is that virDomainGetID can return -1 to mean one of two things: Either that the domain is inactive. Or that an error occurred. The first case happens because xm_internal.c marks inactive domains by setting ->id = -1: virDomainPtr xenXMDomainLookupByName(virConnectPtr conn, const char *domname) //... /* Ensure its marked inactive, because may be cached handle to a previously active domain */ ret->id = -1; (and other places). virt-manager contains code which relies on this, specifically: def current_memory_pretty(self): if self.get_id() == -1: return "0.00 MB" return self.get_memory_pretty() The second case (error) is when domain is NULL or corrupt: unsigned int virDomainGetID(virDomainPtr domain) { if (!VIR_IS_DOMAIN(domain)) { virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); return ((unsigned int) -1); } return (domain->id); } Anyway, adding code to throw an exception when get_id returns -1 (as in patch versions 1 & 2) wasn't clever because it breaks the first case. So the third version of the patch doesn't add the exception code for just this function and virDomainGetName which is similar. We're not making things worse by not adding exception code to this function (after all - it didn't throw an exception before, and it doesn't throw one now: so no change!). But in general there is a real problem here, because we ought to throw an exception when there's an error.
Note that the documentation for virDomainGetID is wrong.
I don't see the error
The virDomainGetID documentation is wrong when it says: Returns the domain ID number or (unsigned int) -1 in case of error It should say something like: Returns the domain ID number. Returns (unsigned int) -1 either for error or in the Xen case if the domain is inactive. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
On Wed, Mar 28, 2007 at 11:06:01AM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
So this patch disables exceptions in those two functions only.
If you are 100% sure you can't pass a wrong pointer though the python binding then okay.
OK so the problem here is that virDomainGetID can return -1 to mean one of two things: Either that the domain is inactive. Or that an error occurred.
I don't see how an inactive domain could have an ID which is kind of the same as an process ID but at the hypervisor level.
The first case happens because xm_internal.c marks inactive domains by setting ->id = -1:
Since it is not running that sounds reasonnable.
virt-manager contains code which relies on this, specifically:
def current_memory_pretty(self): if self.get_id() == -1: return "0.00 MB" return self.get_memory_pretty()
I tend to think it's wrong, it should check the domain status instead.
The second case (error) is when domain is NULL or corrupt:
unsigned int virDomainGetID(virDomainPtr domain) { if (!VIR_IS_DOMAIN(domain)) { virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); return ((unsigned int) -1); } return (domain->id); }
Anyway, adding code to throw an exception when get_id returns -1 (as in patch versions 1 & 2) wasn't clever because it breaks the first case. So the third version of the patch doesn't add the exception code for just this function and virDomainGetName which is similar.
Hum, an inactive domain should still have a name, even if it has an id, now that sounds broken to me.
We're not making things worse by not adding exception code to this function (after all - it didn't throw an exception before, and it doesn't throw one now: so no change!). But in general there is a real problem here, because we ought to throw an exception when there's an error.
Note that the documentation for virDomainGetID is wrong.
I don't see the error
The virDomainGetID documentation is wrong when it says:
Returns the domain ID number or (unsigned int) -1 in case of error
It should say something like:
Returns the domain ID number. Returns (unsigned int) -1 either for error or in the Xen case if the domain is inactive.
To me it's an error. You ask for something which clearly isn't available. If the fact of being inactive is only represented internally by having id == -1 then we probably made a mistake, we should have domain->flags & DOMAIN_IS_DEFINED bit set. We have no small entry point to tell if a domain is just defined and not running, so the id == -1 trick has been used as a differenciator but that seems kind of broken in retrospect. But let's separate the 2 issues, apply that patch, and let's try to solve the way to detect defined domain easilly and properly. 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/
Daniel Veillard wrote:
But let's separate the 2 issues, apply that patch, and let's try to solve the way to detect defined domain easilly and properly.
Agreed. Version 3 of the patch has been committed. Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 "[Negative numbers] darken the very whole doctrines of the equations and make dark of the things which are in their nature excessively obvious and simple" (Francis Maseres FRS, mathematician, 1759)
Richard W.M. Jones wrote:
Daniel Veillard wrote:
But let's separate the 2 issues, apply that patch, and let's try to solve the way to detect defined domain easilly and properly.
Agreed. Version 3 of the patch has been committed.
Yes, actually, I agree, it seems strange that you have to make two different calls to get all domains active and inactive. And overloading the id to indicate inactivity is lame, even though Xen does it. It's totally reasonable to imagine another virtualization system that maintained a domain's id across active/inactive cycles. --Hugh -- Red Hat Virtualization Group http://redhat.com/virtualization Hugh Brock | virt-manager http://virt-manager.org hbrock@redhat.com | virtualization library http://libvirt.org
On Wed, Mar 28, 2007 at 07:07:32AM -0400, Daniel Veillard wrote:
If the fact of being inactive is only represented internally by having id == -1 then we probably made a mistake, we should have domain->flags & DOMAIN_IS_DEFINED bit set.
We already have a state flag for it VIR_DOMAIN_SHUTOFF which is set in virDomainInfoPtr, we can fix virt-manager not to call get_id if the domain is shutdown. 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 -=|
On Wed, Mar 28, 2007 at 03:13:28PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 28, 2007 at 07:07:32AM -0400, Daniel Veillard wrote:
If the fact of being inactive is only represented internally by having id == -1 then we probably made a mistake, we should have domain->flags & DOMAIN_IS_DEFINED bit set.
We already have a state flag for it
VIR_DOMAIN_SHUTOFF
which is set in virDomainInfoPtr, we can fix virt-manager not to call get_id if the domain is shutdown.
It would be nice though if each time you look whether the domain is defined or not, libvirt could give teh answer directly instead of going though the potentially expensive Info call. This would also require to double check invalidation of the _virDomain cached data at the proper places. 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/
On Wed, Mar 28, 2007 at 10:45:34AM -0400, Daniel Veillard wrote:
On Wed, Mar 28, 2007 at 03:13:28PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 28, 2007 at 07:07:32AM -0400, Daniel Veillard wrote:
If the fact of being inactive is only represented internally by having id == -1 then we probably made a mistake, we should have domain->flags & DOMAIN_IS_DEFINED bit set.
We already have a state flag for it
VIR_DOMAIN_SHUTOFF
which is set in virDomainInfoPtr, we can fix virt-manager not to call get_id if the domain is shutdown.
It would be nice though if each time you look whether the domain is defined or not, libvirt could give teh answer directly instead of going though the potentially expensive Info call. This would also require to double check invalidation of the _virDomain cached data at the proper places.
The virDomainGetInfo is a fast-path call - its either a direct hypercall, or a hypercall via the proxy - we never make an expensive XenD call unless they've broken the proxy somehow. 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 -=|
On Wed, Mar 28, 2007 at 03:55:24PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 28, 2007 at 10:45:34AM -0400, Daniel Veillard wrote:
It would be nice though if each time you look whether the domain is defined or not, libvirt could give teh answer directly instead of going though the potentially expensive Info call. This would also require to double check invalidation of the _virDomain cached data at the proper places.
The virDomainGetInfo is a fast-path call - its either a direct hypercall, or a hypercall via the proxy - we never make an expensive XenD call unless they've broken the proxy somehow.
I was thinking about the upcoming remote support, and an RPC is expensive for this kind of informations IMHO. 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/
Daniel Veillard wrote:
On Wed, Mar 28, 2007 at 03:55:24PM +0100, Daniel P. Berrange wrote:
It would be nice though if each time you look whether the domain is defined or not, libvirt could give teh answer directly instead of going though the potentially expensive Info call. This would also require to double check invalidation of the _virDomain cached data at the proper places. The virDomainGetInfo is a fast-path call - its either a direct hypercall, or a hypercall via the proxy - we never make an expensive XenD call unless
On Wed, Mar 28, 2007 at 10:45:34AM -0400, Daniel Veillard wrote: they've broken the proxy somehow.
I was thinking about the upcoming remote support, and an RPC is expensive for this kind of informations IMHO.
Just what I was going to say :-) Rich. -- Emerging Technologies, Red Hat http://et.redhat.com/~rjones/ 64 Baker Street, London, W1U 7DF Mobile: +44 7866 314 421 Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in UK and Wales under Company Registration No. 3798903 Directors: Michael Cunningham (USA), Charlie Peters (USA) and David Owens (Ireland)
On Wed, Mar 28, 2007 at 11:10:38AM -0400, Daniel Veillard wrote:
On Wed, Mar 28, 2007 at 03:55:24PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 28, 2007 at 10:45:34AM -0400, Daniel Veillard wrote:
It would be nice though if each time you look whether the domain is defined or not, libvirt could give teh answer directly instead of going though the potentially expensive Info call. This would also require to double check invalidation of the _virDomain cached data at the proper places.
The virDomainGetInfo is a fast-path call - its either a direct hypercall, or a hypercall via the proxy - we never make an expensive XenD call unless they've broken the proxy somehow.
I was thinking about the upcoming remote support, and an RPC is expensive for this kind of informations IMHO.
The only way to avoid that overhead then would be to add an extra bit of info to the virDomainPtr struct, which is populated at the time the virDomainPtr object is allocated, so we can avoid calling back into the driver model. That would raise some troublesome cache invalidation issues on the client end, though I guess we've already got that issue with the ID value. 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 -=|
On Wed, Mar 28, 2007 at 04:20:18PM +0100, Daniel P. Berrange wrote:
On Wed, Mar 28, 2007 at 11:10:38AM -0400, Daniel Veillard wrote:
I was thinking about the upcoming remote support, and an RPC is expensive for this kind of informations IMHO.
The only way to avoid that overhead then would be to add an extra bit of info to the virDomainPtr struct, which is populated at the time the virDomainPtr object is allocated, so we can avoid calling back into the driver model.
Seems to me domain->flags bit DOMAIN_IS_DEFINED is the right place.
That would raise some troublesome cache invalidation issues on the client end, though I guess we've already got that issue with the ID value.
That could be refreshed each time virConnectListDefinedDomains() virDomainUndefine() virNodeGetInfo() virDomainCreate() is called. The exact same issue could be pointed out for DefinedNetworks, except I would not expect the cost to really be an issue I would guess it's called far less frequently, but I'm just guessing. 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/
On Tue, Mar 27, 2007 at 12:45:13PM +0100, Richard W.M. Jones wrote:
Following on from this thread: https://www.redhat.com/archives/libvir-list/2007-March/thread.html#00341
The first attachment is a patch which changes the generated Python bindings so that they always raise an exception when an underlying error occurs in the C libvirt library.
The second attachment is for information only (not to be applied) - it shows the differences in the generated file libvirtclass.py. I have checked each of these changes by hand against both the libvirt.c interface description and the C implementation of the Python bindings (libvir.c and libvirt-py.c), and each change seems correct.
I have _not_ tested this against virt-manager. It is quite possible that virt-manager will cease to work if it wasn't checking the return values from affected functions carefully. I'll check this later today and supply a patch (to virt-manager) if necessary.
I have some additional questions about the Python bindings, but I'll put those in a separate thread.
That makes sense, my only worry is about the effect in applications, but it's really cleaner from a Python viewpoint to raise exceptions there, and fix application code when it's not too late. I guess someone using for example RHEL5 and updating their libvirt would also update python-virtinst and virt-manager accordingly. Let's apply this, the earlier it's fixed the smaller the collateral damages ! thanks a lot, especially for going though the rather tortuous generator.py code ... 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 (4)
-
Daniel P. Berrange -
Daniel Veillard -
Hugh Brock -
Richard W.M. Jones