On Wed, Jan 29, 2014 at 07:01:31AM -0500, Adam Walters wrote:
On Tue, Jan 28, 2014 at 12:07 PM, Daniel P. Berrange <berrange@redhat.com>wrote:
On Thu, Jan 23, 2014 at 03:14:19PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' which provides access to configuration data, such as secret and storage definitions, without requiring a connection to a hypervisor. This complements my previous patchset, which resolves the race condition on libvirtd startup.
So I had an idea about one possible alternative way to deal with this. Basically have a single global virConnectPtr instance which each non-virt driver wires up its hooks to when it starts. I've not fully tested this approach, but I've got a example patch which better illustrates what I mean:
diff --git a/src/datatypes.c b/src/datatypes.c index 73f17e7..9da2ff4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -124,6 +124,26 @@ error: return NULL; }
+virConnectPtr virGetGlobalConnect(void) +{ + static virConnectPtr globalconn; + virConnectPtr conn; + + if (globalconn) + return globalconn; + + if (!(conn = virGetConnect())) + return NULL; + + if (!(conn->uri = virURIParse("global:///"))) { + virObjectUnref(conn); + return NULL; + } + + globalconn = conn; + return globalconn; +} + /** * virConnectDispose: * @conn: the hypervisor connection to release diff --git a/src/datatypes.h b/src/datatypes.h index 9621c55..6806832 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -521,6 +521,8 @@ struct _virNWFilter { */
virConnectPtr virGetConnect(void); +virConnectPtr virGetGlobalConnect(void); + virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..8ac2bf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -762,6 +762,7 @@ virDomainSnapshotClass; virGetConnect; virGetDomain; virGetDomainSnapshot; +virGetGlobalConnect; virGetInterface; virGetNetwork; virGetNodeDevice; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9f7f946..a88ea23 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1102,6 +1102,10 @@ secretStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *base = NULL; + virConnectPtr conn; + + if ((conn = virGetGlobalConnect()) == NULL) + return -1;
if (VIR_ALLOC(driverState) < 0) return -1; @@ -1127,6 +1131,7 @@ secretStateInitialize(bool privileged, if (loadSecrets(driverState, &driverState->secrets) < 0) goto error;
+ conn->secretPrivateData = driverState; secretDriverUnlock(driverState); return 0;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c83aa8a..017a74d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,14 +68,7 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; - virConnectPtr conn = NULL; - - /* XXX Remove hardcoding of QEMU URI */ - if (driverState->privileged) - conn = virConnectOpen("qemu:///system"); - else - conn = virConnectOpen("qemu:///session"); - /* Ignoring NULL conn - let backends decide */ + virConnectPtr conn = virGetGlobalConnect();
for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; @@ -129,8 +122,6 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } virStoragePoolObjUnlock(pool); } - - virObjectUnref(conn); }
/**
It is based on the similar idea that you had, that we don't actually need to have a full connection with virt drivers active. The difference with my approach is that we're not exposing a new URI externally - this hack is only visible inside libvirtd, so we're free to change it any time we want.
Originally, I wanted to make the 'config:///' URI only accessible from within libvirt, but I simply didn't know how to accomplish that. I certainly see no reason why a new external URI would be needed currently. The only potential problem I can foresee with an internal URI is that eventually, if and when libvirt is split into multiple processes, the process may break down and require a public URI. I would think that could be dealt with if and when that bridge needs crossing, though.
Unfortunately, I'm in the middle of rebuilding my lab, so I can't really test the patch currently (reduced VM capacity while machines are being rebuilt). If you need, I could probably test your patch by this weekend, though.
Ah don't worry about it too much, as I'm off to FOSDEM on friday. I'll continue to explore this idea and see about reating a full patch to test next week. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|