[Libvir] [PATCH] virDomainLookup* functions raise VIR_ERR_NO_DOMAIN if the domain doesn't exist
The patches that I'm going to follow up with change virDomainLookup* functions so that they raise VIR_ERR_NO_DOMAIN where a domain doesn't exist (but there is no other error), else raise some other error where there's an actual error occuring. This will allow wrappers in other languages to distinguish the three cases found / not found / error. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
This patch: (1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter will be used later to make the same change to virNetworkLookup* functions). (2) Changes the documentation of virDomainLookup* functions to match the new behaviour. (3) Changes qemu & test drivers to have the new behaviour. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 03, 2007 at 04:12:20PM +0100, Richard W.M. Jones wrote:
This patch:
(1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter will be used later to make the same change to virNetworkLookup* functions).
(2) Changes the documentation of virDomainLookup* functions to match the new behaviour.
(3) Changes qemu & test drivers to have the new behaviour.
Looks good. 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 Tue, Jul 03, 2007 at 04:12:20PM +0100, Richard W.M. Jones wrote:
This patch:
(1) Adds the VIR_ERR_NO_DOMAIN and VIR_ERR_NO_NETWORK errors (the latter will be used later to make the same change to virNetworkLookup* functions).
(2) Changes the documentation of virDomainLookup* functions to match the new behaviour.
(3) Changes qemu & test drivers to have the new behaviour.
Looks fine, +1 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/
Here we just fix the remote driver so that if the server goes down, the process doesn't die on SIGPIPE. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 03, 2007 at 04:12:56PM +0100, Richard W.M. Jones wrote:
Here we just fix the remote driver so that if the server goes down, the process doesn't die on SIGPIPE.
This isn't thread safe. Changing signals with sigaction affects the entire process - so if you've got multiple threads running with multiple remote connections open, you can get overlapping calls. Either we should just block sigpipe in virInitialize, or mandate that the application needs to block sigpipe itself. I'm not sure really what best course is - I'd probably lean towards saying it is the application's responsibility to deal with sig handlers. 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 Tue, Jul 03, 2007 at 04:12:56PM +0100, Richard W.M. Jones wrote:
Here we just fix the remote driver so that if the server goes down, the process doesn't die on SIGPIPE.
This isn't thread safe. Changing signals with sigaction affects the entire process - so if you've got multiple threads running with multiple remote connections open, you can get overlapping calls. Either we should just block sigpipe in virInitialize, or mandate that the application needs to block sigpipe itself. I'm not sure really what best course is - I'd probably lean towards saying it is the application's responsibility to deal with sig handlers.
Yes, you're right. I'll change this so that instead it adds some supporting documentation for app writers. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
I'm not quite sure what the problem is (although the problem is in xm_internal), but when you use xm_internal over remote, it sometimes doesn't initialize its internal cache correctly, so it thinks that there are no inactive domains. The fix is a one-liner which I hit upon by accident -- I don't really understand why it works: @@ -489,7 +487,7 @@ xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { - if (nconnections == 0) { + if (configCache == NULL) { configCache = virHashCreate(50); if (!configCache) return (-1); But the attached patch also adds proper error messages to xenXMConfigCacheRefresh too. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
I'm not quite sure what the problem is (although the problem is in xm_internal), but when you use xm_internal over remote, it sometimes doesn't initialize its internal cache correctly, so it thinks that there are no inactive domains.
The fix is a one-liner which I hit upon by accident -- I don't really understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented by the xenXMOpen & xenXMClose methods. So the change you show below should be identical to previous behaviour. Is something calling the xenXMClose method too many times maybe ? I guess some judicious use of syslog would show it up
@@ -489,7 +487,7 @@ xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { - if (nconnections == 0) { + if (configCache == NULL) { configCache = virHashCreate(50); if (!configCache) return (-1);
But the attached patch also adds proper error messages to xenXMConfigCacheRefresh too.
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 Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
I'm not quite sure what the problem is (although the problem is in xm_internal), but when you use xm_internal over remote, it sometimes doesn't initialize its internal cache correctly, so it thinks that there are no inactive domains.
The fix is a one-liner which I hit upon by accident -- I don't really understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented by the xenXMOpen & xenXMClose methods. So the change you show below should be identical to previous behaviour. Is something calling the xenXMClose method too many times maybe ? I guess some judicious use of syslog would show it up
But I can assure you that the patch is necessary, even if I don't understand how it works ... Rich, -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, Jul 04, 2007 at 05:12:53AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:14:30PM +0100, Richard W.M. Jones wrote:
I'm not quite sure what the problem is (although the problem is in xm_internal), but when you use xm_internal over remote, it sometimes doesn't initialize its internal cache correctly, so it thinks that there are no inactive domains.
The fix is a one-liner which I hit upon by accident -- I don't really understand why it works:
Very peculiar - the nconnections stuff is incremented / decremented by the xenXMOpen & xenXMClose methods. So the change you show below should be identical to previous behaviour. Is something calling the xenXMClose method too many times maybe ? I guess some judicious use of syslog would show it up
Hum, I really dislike the following: int xenXMClose(virConnectPtr conn ATTRIBUTE_UNUSED) { if (!nconnections--) { I don't want to have to rely on C operator priorities to try to guess if the code is correct. nconnections--; if (nconnections <= 0) { is way cleaner IMHO.
@@ -489,7 +487,7 @@ xenXMOpen (virConnectPtr conn ATTRIBUTE_UNUSED, const char *name ATTRIBUTE_UNUSED, int flags ATTRIBUTE_UNUSED) { - if (nconnections == 0) { + if (configCache == NULL) { configCache = virHashCreate(50); if (!configCache) return (-1);
IMHO that patch is wrong because it would not reset nconnections to 0 I would prefer to understand what is really going on. Could you check with the cleanup of xenXMClose to see if this is sufficient ?
But the attached patch also adds proper error messages to xenXMConfigCacheRefresh too.
That is fine. There is still one thing I don't understand in the original patch, the replacement of xenDaemonDomainLookupByName() by xenDaemonLookupByName() which I don't see in the current 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/
If you recall the current driver model, which looks like this: libvirt.c | (virDriver) | V +---- xen_unified.c | (virDriver) | V +---- xen_internal.c | +---- xend_internal.c etc. The interface between libvirt.c and xen_unified.c is virDriver. And currently the same interface is reused between xen_unified.c and its underlying drivers. This patch defines a new structure called xenUnifiedDriver, which begins as a subset of virDriver, and eventually will go away in favour of direct calls from xen_unified to the underlying parts (much simpler to understand what's going on that way, and we won't need the current driver ordering hacks, and anyway the xen drivers call each other semi-randomly). libvirt.c | (virDriver) | V +---- xen_unified.c | (xenUnifiedDriver + direct calls) | V +---- xen_internal.c | +---- xend_internal.c etc. This patch starts by removing the id, name and version fields from virDriver. It also removes getMaxVcpus and the domainLookup* fields, which will make more sense when you see patches #6 and #7 in this series. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
This patch starts by removing the id, name and version fields from virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will make more sense when you see patches #6 and #7 in this series.
Yes, i guess 4, 6 & 7 should really be all one patch - since if you apply , but not 6 & 7 the driver is non-functional. Anyway, the combo of this & the other patches look like they're doing what I'd expect, so objections here... 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, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
This patch starts by removing the id, name and version fields from virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will make more sense when you see patches #6 and #7 in this series.
Yes, i guess 4, 6 & 7 should really be all one patch - since if you apply , but not 6 & 7 the driver is non-functional. Anyway, the combo of this & the other patches look like they're doing what I'd expect, so objections here...
Honnestly I'm not really convinced as such the patches are an improvement. You introduce a new structure, it's still indirect, how is that better ? Now if you want to get rid of the indirection, why not but: - this forces to make all those functions public again - puts more logic in xen_unified, I'm not sure its code would become more maintainable This would probably help in debugging sessions, that's right. Do we want to do this while Xen support is still in flux, we need to do Xen-API support (on localhost, I would not extend over network) in a not so distant future, the driver based approach simplifies the integration of new code. I'm not against applying those 3 patches but at the moment I still feel unconvinced :-) 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, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
This patch starts by removing the id, name and version fields from virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will make more sense when you see patches #6 and #7 in this series. Yes, i guess 4, 6 & 7 should really be all one patch - since if you apply , but not 6 & 7 the driver is non-functional. Anyway, the combo of this & the other patches look like they're doing what I'd expect, so objections here...
Honnestly I'm not really convinced as such the patches are an improvement. You introduce a new structure, it's still indirect, how is that better ?
I didn't explain it well, but here's why: At the moment if you want to find out how, say, "reboot" is implemented you need to first of all examine all 5 underlying drivers to see which have a non-NULL function in that slot in the driver table. Secondly you need to examine xen_unified.c in two places: at the top of the file to see what order the backend drivers are normally called in, then at the xenUnifiedDomainReboot function itself to see if the function follows that order or is one of the exceptions that does its own thing. So you have to examine 7 places in the code. I want to bring that all together so that you only have to examine one place (xenUnifiedDomainReboot) to see how it is implemented. The fact that at the moment virDriver is a dual-purpose structure was just about convenience when I originally implemented xen-unified. It makes no particular sense to have the interface between libvirt.c and drivers be the same as the xen-internal interface between xen_unified.c and its underlying drivers.
Now if you want to get rid of the indirection, why not but: - this forces to make all those functions public again
In dynamic libs these functions will be hidden because of the libvirt_syms file. In static libvirt.a the functions will be exported, but they are already exported at the moment via the driver table, so there is no practical difference.
- puts more logic in xen_unified, I'm not sure its code would become more maintainable This would probably help in debugging sessions, that's right. Do we want to do this while Xen support is still in flux, we need to do Xen-API support (on localhost, I would not extend over network) in a not so distant future, the driver based approach simplifies the integration of new code.
I really don't think the patch as it stands is a major change though - I mean the calls are still going through a structure, it just happens to be a xen-specific structure rather than the virDriver structure. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, Jul 04, 2007 at 12:21:18PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
On Wed, Jul 04, 2007 at 05:24:03AM +0100, Daniel P. Berrange wrote:
On Tue, Jul 03, 2007 at 04:19:13PM +0100, Richard W.M. Jones wrote:
This patch starts by removing the id, name and version fields from virDriver.
It also removes getMaxVcpus and the domainLookup* fields, which will make more sense when you see patches #6 and #7 in this series. Yes, i guess 4, 6 & 7 should really be all one patch - since if you apply , but not 6 & 7 the driver is non-functional. Anyway, the combo of this & the other patches look like they're doing what I'd expect, so objections here...
Honnestly I'm not really convinced as such the patches are an improvement. You introduce a new structure, it's still indirect, how is that better ?
I didn't explain it well, but here's why: At the moment if you want to find out how, say, "reboot" is implemented you need to first of all examine all 5 underlying drivers to see which have a non-NULL function in that slot in the driver table. Secondly you need to examine xen_unified.c in two places: at the top of the file to see what order the backend drivers are normally called in, then at the xenUnifiedDomainReboot function itself to see if the function follows that order or is one of the exceptions that does its own thing. So you have to examine 7 places in the code. I want to bring that all together so that you only have to examine one place (xenUnifiedDomainReboot) to see how it is implemented.
Hum, okay :-) +1 but ultimately the real fix is to drop that sub driver structure. 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/
Fix the whitespace in xs_internal.c xenStoreLookupByName function. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Tue, Jul 03, 2007 at 04:19:31PM +0100, Richard W.M. Jones wrote:
Fix the whitespace in xs_internal.c xenStoreLookupByName function.
sure, +1 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/
Because the type field is now missing from xenUnifiedDriver, the old method of implementing xenUnifiedGetMaxVcpus didn't work. There's only one underlying driver which can implement this anyway, so just make the call directly. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Richard W.M. Jones wrote:
diff -u -p -r1.14 xen_unified.c --- src/xen_unified.c 26 Jun 2007 11:42:46 -0000 1.14 +++ src/xen_unified.c 3 Jul 2007 15:01:39 -0000 @@ -276,17 +269,17 @@ xenUnifiedGetURI (virConnectPtr conn) static int xenUnifiedGetMaxVcpus (virConnectPtr conn, const char *type) { - GET_PRIVATE(conn);
Not quite sure what happened there, but obviously the above call to GET_PRIVATE needs to stay because the priv pointer is used in the subsequent code ...
+ if (priv->opened[XEN_UNIFIED_HYPERVISOR_OFFSET]) + return xenHypervisorGetMaxVcpus (conn, type); + else { + xenUnifiedError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; + }
Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
This changes the three virDomainLookup* calls to make direct calls to the underlying drivers, and to return VIR_ERR_NO_DOMAIN error if the domain is not found. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones