Re: [libvirt] [RFC PATCHv2 0/4] Adding 'config' driver

On Tue, Jan 28, 2014 at 12:07 PM, Daniel P. Berrange <berrange(a)redhat.com&gt;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.