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

[Daniel P. Berrange]

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 at 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 :|