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

[Adam Walters]

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.


>
> 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:|
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140129/cac94d17/attachment-0001.htm>