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

[Adam Walters <adam@xxxxxxxxxxxxxxxxx>]

On Tue, Jan 28, 2014 at 12:07 PM, Daniel P. Berrange <berrange@xxxxxxxxxx> 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 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list