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.
If you think this approach will work I'll look at doing a complete patch for it for all non-virt drivers.
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:|