When the client has a connection to one of the hypervisor specific
daemons (eg virtqemud), the app may still expect to use the secondary
driver APIs (storage, network, etc). None of these will be registered in
the hypervisor daemon, so we must explicitly open a connection to each
of the daemons for the secondary drivers we need.

We don't want to open these secondary driver connections at the same
time as the primary connection is opened though. That would mean that
establishing a connection to virtqemud would immediately trigger
activation of virtnetworkd, virnwfilterd, etc despite that that these
drivers may never be used by the app.

Thus we only open the secondary driver connections at time of first use
by an API call.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/remote/remote_daemon.h          |  13 +++
 src/remote/remote_daemon_dispatch.c | 146 ++++++++++++++++++----------
 2 files changed, 109 insertions(+), 50 deletions(-)

diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index a403d2593a..a2d9af4036 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -70,12 +70,25 @@ struct daemonClientPrivate {
      * called, it will be set back to NULL if that succeeds.
      */
     virConnectPtr conn;
+
+    /* These secondary drivers may point back to 'conn'
+     * in the monolithic daemon setups. Otherwise they
+     * can be NULL and opened on first use, pointing
+     * to remote driver use of an external daemon
+     */
     virConnectPtr interfaceConn;
+    const char *interfaceURI;
     virConnectPtr networkConn;
+    const char *networkURI;
     virConnectPtr nodedevConn;
+    const char *nodedevURI;
     virConnectPtr nwfilterConn;
+    const char *nwfilterURI;
     virConnectPtr secretConn;
+    const char *secretURI;
     virConnectPtr storageConn;
+    const char *storageURI;
+    bool readonly;
 
     daemonClientStreamPtr streams;
 };
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index b677bd44ee..f1304695bd 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1954,18 +1954,35 @@ remoteGetHypervisorConn(virNetServerClientPtr client)
 }
 
 
+static virConnectPtr
+remoteGetSecondaryConn(bool readonly, virConnectPtr *conn, const char *uri)
+{
+    if (!*conn) {
+        if (uri) {
+            VIR_DEBUG("Opening driver %s", uri);
+            if (readonly)
+                *conn = virConnectOpenReadOnly(uri);
+            else
+                *conn = virConnectOpen(uri);
+            if (!conn)
+                return NULL;
+            VIR_DEBUG("Opened driver %p", *conn);
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+            return NULL;
+        }
+    }
+
+    return *conn;
+}
+
 static virConnectPtr
 remoteGetInterfaceConn(virNetServerClientPtr client)
 {
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->interfaceConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->interfaceConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->interfaceConn, priv->interfaceURI);
 }
 
 
@@ -1975,12 +1992,7 @@ remoteGetNetworkConn(virNetServerClientPtr client)
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->networkConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->networkConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->networkConn, priv->networkURI);
 }
 
 
@@ -1990,12 +2002,7 @@ remoteGetNodeDevConn(virNetServerClientPtr client)
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->nodedevConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->nodedevConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->nodedevConn, priv->nodedevURI);
 }
 
 
@@ -2005,12 +2012,7 @@ remoteGetNWFilterConn(virNetServerClientPtr client)
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->nwfilterConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->nwfilterConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->nwfilterConn, priv->nwfilterURI);
 }
 
 
@@ -2020,12 +2022,7 @@ remoteGetSecretConn(virNetServerClientPtr client)
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->secretConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->secretConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->secretConn, priv->secretURI);
 }
 
 
@@ -2035,15 +2032,11 @@ remoteGetStorageConn(virNetServerClientPtr client)
     struct daemonClientPrivate *priv =
         virNetServerClientGetPrivateData(client);
 
-    if (!priv->storageConn) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
-        return NULL;
-    }
-
-    return priv->storageConn;
+    return remoteGetSecondaryConn(priv->readonly, &priv->storageConn, priv->storageURI);
 }
 
 
+
 void *remoteClientNew(virNetServerClientPtr client,
                       void *opaque ATTRIBUTE_UNUSED)
 {
@@ -2075,6 +2068,9 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
     unsigned int flags;
     struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
     int rv = -1;
+#ifndef LIBVIRTD
+    const char *type = NULL;
+#endif
 
     VIR_DEBUG("priv=%p conn=%p", priv, priv->conn);
     virMutexLock(&priv->lock);
@@ -2093,20 +2089,70 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
     if (virNetServerClientGetReadonly(client))
         flags |= VIR_CONNECT_RO;
 
-    priv->conn =
-        flags & VIR_CONNECT_RO
-        ? virConnectOpenReadOnly(name)
-        : virConnectOpen(name);
-
-    if (priv->conn == NULL)
-        goto cleanup;
-
-    priv->interfaceConn = virObjectRef(priv->conn);
-    priv->networkConn = virObjectRef(priv->conn);
-    priv->nodedevConn = virObjectRef(priv->conn);
-    priv->nwfilterConn = virObjectRef(priv->conn);
-    priv->secretConn = virObjectRef(priv->conn);
-    priv->storageConn = virObjectRef(priv->conn);
+    priv->readonly = flags & VIR_CONNECT_RO;
+
+    VIR_DEBUG("Opening driver %s", name);
+    if (!(priv->conn = priv->readonly ?
+          virConnectOpenReadOnly(name) :
+          virConnectOpen(name)))
+        goto cleanup;
+    VIR_DEBUG("Opened %p", priv->conn);
+
+#ifndef LIBVIRTD
+    if (!(type = virConnectGetType(priv->conn)))
+        goto cleanup;
+
+    VIR_DEBUG("Primary driver type is '%s'", type);
+    if (STREQ(type, "QEMU") ||
+        STREQ(type, "LIBXL") ||
+        STREQ(type, "LXC") ||
+        STREQ(type, "VBOX") ||
+        STREQ(type, "bhyve") ||
+        STREQ(type, "vz") ||
+        STREQ(type, "Parallels")) {
+        VIR_DEBUG("Hypervisor driver found, setting URIs for secondary drivers");
+        priv->interfaceURI =  getuid() == 0 ? "interface:///system" : "interface:///session";
+        priv->networkURI = getuid() == 0 ? "network:///system" : "network:///session";
+        priv->nodedevURI =  getuid() == 0 ? "nodedev:///system" : "nodedev:///session";
+        if (getuid() == 0)
+            priv->nwfilterURI = "nwfilter:///system";
+        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";
+        priv->storageURI = getuid() == 0 ? "storage:///system" : "storage:///session";
+    } else if (STREQ(type, "interface")) {
+        VIR_DEBUG("Interface driver found");
+        priv->interfaceConn = virObjectRef(priv->conn);
+    } else if (STREQ(type, "network")) {
+        VIR_DEBUG("Network driver found");
+        priv->networkConn = virObjectRef(priv->conn);
+    } else if (STREQ(type, "nodedev")) {
+        VIR_DEBUG("Nodedev driver found");
+        priv->nodedevConn = virObjectRef(priv->conn);
+    } else if (STREQ(type, "nwfilter")) {
+        VIR_DEBUG("NWFilter driver found");
+        priv->nwfilterConn = virObjectRef(priv->conn);
+    } else if (STREQ(type, "secret")) {
+        VIR_DEBUG("Secret driver found");
+        priv->secretConn = virObjectRef(priv->conn);
+    } else if (STREQ(type, "storage")) {
+        VIR_DEBUG("Storage driver found");
+        priv->storageConn = virObjectRef(priv->conn);
+
+        /* Co-open the secret driver, as apps using the storage driver may well
+         * need access to secrets for storage auth
+         */
+        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";
+    } else {
+#endif /* LIBVIRTD */
+        VIR_DEBUG("Pointing secondary drivers to primary");
+        priv->interfaceConn = virObjectRef(priv->conn);
+        priv->networkConn = virObjectRef(priv->conn);
+        priv->nodedevConn = virObjectRef(priv->conn);
+        priv->nwfilterConn = virObjectRef(priv->conn);
+        priv->secretConn = virObjectRef(priv->conn);
+        priv->storageConn = virObjectRef(priv->conn);
+#ifndef LIBVIRTD
+    }
+#endif /* LIBVIRTD */
 
     /* force update the @readonly attribute which was inherited from the
      * virNetServerService object - this is important for sockets that are RW
-- 
2.21.0

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

On Tue, Jul 23, 2019 at 05:03:14PM +0100, Daniel P. Berrangé wrote:
> When the client has a connection to one of the hypervisor specific
> daemons (eg virtqemud), the app may still expect to use the secondary
> driver APIs (storage, network, etc). None of these will be registered in
> the hypervisor daemon, so we must explicitly open a connection to each
> of the daemons for the secondary drivers we need.
> 
> We don't want to open these secondary driver connections at the same
> time as the primary connection is opened though. That would mean that
> establishing a connection to virtqemud would immediately trigger
> activation of virtnetworkd, virnwfilterd, etc despite that that these
> drivers may never be used by the app.
> 
> Thus we only open the secondary driver connections at time of first use
> by an API call.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/remote/remote_daemon.h          |  13 +++
>  src/remote/remote_daemon_dispatch.c | 146 ++++++++++++++++++----------
>  2 files changed, 109 insertions(+), 50 deletions(-)


> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
> index b677bd44ee..f1304695bd 100644
> --- a/src/remote/remote_daemon_dispatch.c
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1954,18 +1954,35 @@ remoteGetHypervisorConn(virNetServerClientPtr client)
>  }
>  
>  
> +static virConnectPtr
> +remoteGetSecondaryConn(bool readonly, virConnectPtr *conn, const char *uri)
> +{
> +    if (!*conn) {
> +        if (uri) {
> +            VIR_DEBUG("Opening driver %s", uri);
> +            if (readonly)
> +                *conn = virConnectOpenReadOnly(uri);
> +            else
> +                *conn = virConnectOpen(uri);
> +            if (!conn)
> +                return NULL;

This needs to be  !*conn

> +            VIR_DEBUG("Opened driver %p", *conn);
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
> +            return NULL;
> +        }
> +    }
> +
> +    return *conn;
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
[...]
> +++ b/src/remote/remote_daemon_dispatch.c
> @@ -1954,18 +1954,35 @@ remoteGetHypervisorConn(virNetServerClientPtr client)
>  }
>  
>  
> +static virConnectPtr
> +remoteGetSecondaryConn(bool readonly, virConnectPtr *conn, const char *uri)

We seem to mostly have a single empty line between functions in this
file, so please stick to that style. Also, have each argument on its
own line.

Additional comments: it personally would make more sense to me if
readonly was the last argument, though I won't object if you prefer
keeping it this way; however, the way you return the connection
pointer in addition to storing it in the user-provided location looks
weird to me.

You could have

  static bool
  remoteGetSecondaryConn(virConnectPtr *conn,
                         const char *uri,
                         bool readonly)

or actually even

  static void
  remoteGetSecondaryConn(virConnectPtr *conn,
                         const char *uri,
                         bool readonly)

since you're not doing any additional check on the return value in
the caller. Then...

[...]
>  static virConnectPtr
>  remoteGetInterfaceConn(virNetServerClientPtr client)
>  {
>      struct daemonClientPrivate *priv =
>          virNetServerClientGetPrivateData(client);
>  
> -    if (!priv->interfaceConn) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
> -        return NULL;
> -    }
> -
> -    return priv->interfaceConn;
> +    return remoteGetSecondaryConn(priv->readonly, &priv->interfaceConn, priv->interfaceURI);

... you could leave the 'return' statement alone, and just replace
the check on priv->xxxConn with a call to remoteGetSecondaryConn().

[...]
>  }
>  
>  
> +
>  void *remoteClientNew(virNetServerClientPtr client,
>                        void *opaque ATTRIBUTE_UNUSED)

Unrelated whitespace change.

[...]
> @@ -2093,20 +2089,70 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> +    VIR_DEBUG("Opening driver %s", name);
> +    if (!(priv->conn = priv->readonly ?
> +          virConnectOpenReadOnly(name) :
> +          virConnectOpen(name)))
> +        goto cleanup;
> +    VIR_DEBUG("Opened %p", priv->conn);

Ewww. Please get rid of the Elvis operator and just use a regular
if/else instead.

> +
> +#ifndef LIBVIRTD
> +    if (!(type = virConnectGetType(priv->conn)))
> +        goto cleanup;
> +
> +    VIR_DEBUG("Primary driver type is '%s'", type);
> +    if (STREQ(type, "QEMU") ||
> +        STREQ(type, "LIBXL") ||
> +        STREQ(type, "LXC") ||
> +        STREQ(type, "VBOX") ||
> +        STREQ(type, "bhyve") ||
> +        STREQ(type, "vz") ||
> +        STREQ(type, "Parallels")) {

Wait, we store the connection type as a string? Ewww.

> +        VIR_DEBUG("Hypervisor driver found, setting URIs for secondary drivers");
> +        priv->interfaceURI =  getuid() == 0 ? "interface:///system" : "interface:///session";
> +        priv->networkURI = getuid() == 0 ? "network:///system" : "network:///session";
> +        priv->nodedevURI =  getuid() == 0 ? "nodedev:///system" : "nodedev:///session";
> +        if (getuid() == 0)
> +            priv->nwfilterURI = "nwfilter:///system";
> +        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";
> +        priv->storageURI = getuid() == 0 ? "storage:///system" : "storage:///session";

Lots of repeated calls to getuid() and lots of Elvis operators
here... I would rewrite it along the lines of

  if (getuid() == 0) {
      priv->interfaceURI = "interface:///system";
      priv->networkURI = "network:///system";
      priv->nodedevURI = "nodedev:///system";
      priv->secretURI = "secret:///system";
      priv->storageURI = "storage:///system";
      priv->nwfilterURI = "nwfilter:///system";
  } else {
      priv->interfaceURI = "interface:///session";
      priv->networkURI = "network:///session";
      priv->nodedevURI = "nodedev:///session";
      priv->secretURI = "secret:///session";
      priv->storageURI = "storage:///session";
      /* No session URI for the nwfilter driver */
  }

[...]
> +    } else if (STREQ(type, "storage")) {
> +        VIR_DEBUG("Storage driver found");
> +        priv->storageConn = virObjectRef(priv->conn);
> +
> +        /* Co-open the secret driver, as apps using the storage driver may well
> +         * need access to secrets for storage auth
> +         */
> +        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";

Again, lose the Elvis operator.

Could there be other dependencies like this one we might be missing?
I guess we're gonna find out as people start using this :)

> +    } else {
> +#endif /* LIBVIRTD */

The comment should be "! LIBVIRTD". Same below.

> +        VIR_DEBUG("Pointing secondary drivers to primary");
> +        priv->interfaceConn = virObjectRef(priv->conn);
> +        priv->networkConn = virObjectRef(priv->conn);
> +        priv->nodedevConn = virObjectRef(priv->conn);
> +        priv->nwfilterConn = virObjectRef(priv->conn);
> +        priv->secretConn = virObjectRef(priv->conn);
> +        priv->storageConn = virObjectRef(priv->conn);

Do we even need this code for the non-libvirtd case? We have listed
all drivers, primary and secondary, above, so I can't think of any
valid reason we'd end up here unless there's a bug, and in that case
we'd just be masking it, no? So the structure should be more like

  #ifdef LIBVIRTD
  /* point all secondary drivers to primary */
  #else /* ! LIBVIRTD */
  if (STREQ(type, ...) {
      ...
  } else if (STREQ(type, ...) {
  ...
  } else {
      /* freak out */
  }
  #endif /* ! LIBVIRTD */

-- 
Andrea Bolognani / Red Hat / Virtualization

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

On Mon, Jul 29, 2019 at 10:33:08AM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
> [...]
> > +++ b/src/remote/remote_daemon_dispatch.c
> > @@ -1954,18 +1954,35 @@ remoteGetHypervisorConn(virNetServerClientPtr client)
> >  }
> >  
> >  
> > +static virConnectPtr
> > +remoteGetSecondaryConn(bool readonly, virConnectPtr *conn, const char *uri)
> 
> We seem to mostly have a single empty line between functions in this
> file, so please stick to that style. Also, have each argument on its
> own line.
> 
> Additional comments: it personally would make more sense to me if
> readonly was the last argument, though I won't object if you prefer
> keeping it this way; however, the way you return the connection
> pointer in addition to storing it in the user-provided location looks
> weird to me.
> 
> You could have
> 
>   static bool
>   remoteGetSecondaryConn(virConnectPtr *conn,
>                          const char *uri,
>                          bool readonly)
> 
> or actually even
> 
>   static void
>   remoteGetSecondaryConn(virConnectPtr *conn,
>                          const char *uri,
>                          bool readonly)
> 
> since you're not doing any additional check on the return value in
> the caller. Then...
> 
> [...]
> >  static virConnectPtr
> >  remoteGetInterfaceConn(virNetServerClientPtr client)
> >  {
> >      struct daemonClientPrivate *priv =
> >          virNetServerClientGetPrivateData(client);
> >  
> > -    if (!priv->interfaceConn) {
> > -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("hypervisor connection not open"));
> > -        return NULL;
> > -    }
> > -
> > -    return priv->interfaceConn;
> > +    return remoteGetSecondaryConn(priv->readonly, &priv->interfaceConn, priv->interfaceURI);
> 
> ... you could leave the 'return' statement alone, and just replace
> the check on priv->xxxConn with a call to remoteGetSecondaryConn().
> 
> [...]
> >  }
> >  
> >  
> > +
> >  void *remoteClientNew(virNetServerClientPtr client,
> >                        void *opaque ATTRIBUTE_UNUSED)
> 
> Unrelated whitespace change.
> 
> [...]
> > @@ -2093,20 +2089,70 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> > +    VIR_DEBUG("Opening driver %s", name);
> > +    if (!(priv->conn = priv->readonly ?
> > +          virConnectOpenReadOnly(name) :
> > +          virConnectOpen(name)))
> > +        goto cleanup;
> > +    VIR_DEBUG("Opened %p", priv->conn);
> 
> Ewww. Please get rid of the Elvis operator and just use a regular
> if/else instead.
> 
> > +
> > +#ifndef LIBVIRTD
> > +    if (!(type = virConnectGetType(priv->conn)))
> > +        goto cleanup;
> > +
> > +    VIR_DEBUG("Primary driver type is '%s'", type);
> > +    if (STREQ(type, "QEMU") ||
> > +        STREQ(type, "LIBXL") ||
> > +        STREQ(type, "LXC") ||
> > +        STREQ(type, "VBOX") ||
> > +        STREQ(type, "bhyve") ||
> > +        STREQ(type, "vz") ||
> > +        STREQ(type, "Parallels")) {
> 
> Wait, we store the connection type as a string? Ewww.
> 
> > +        VIR_DEBUG("Hypervisor driver found, setting URIs for secondary drivers");
> > +        priv->interfaceURI =  getuid() == 0 ? "interface:///system" : "interface:///session";
> > +        priv->networkURI = getuid() == 0 ? "network:///system" : "network:///session";
> > +        priv->nodedevURI =  getuid() == 0 ? "nodedev:///system" : "nodedev:///session";
> > +        if (getuid() == 0)
> > +            priv->nwfilterURI = "nwfilter:///system";
> > +        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";
> > +        priv->storageURI = getuid() == 0 ? "storage:///system" : "storage:///session";
> 
> Lots of repeated calls to getuid() and lots of Elvis operators
> here... I would rewrite it along the lines of
> 
>   if (getuid() == 0) {
>       priv->interfaceURI = "interface:///system";
>       priv->networkURI = "network:///system";
>       priv->nodedevURI = "nodedev:///system";
>       priv->secretURI = "secret:///system";
>       priv->storageURI = "storage:///system";
>       priv->nwfilterURI = "nwfilter:///system";
>   } else {
>       priv->interfaceURI = "interface:///session";
>       priv->networkURI = "network:///session";
>       priv->nodedevURI = "nodedev:///session";
>       priv->secretURI = "secret:///session";
>       priv->storageURI = "storage:///session";
>       /* No session URI for the nwfilter driver */
>   }
> 
> [...]
> > +    } else if (STREQ(type, "storage")) {
> > +        VIR_DEBUG("Storage driver found");
> > +        priv->storageConn = virObjectRef(priv->conn);
> > +
> > +        /* Co-open the secret driver, as apps using the storage driver may well
> > +         * need access to secrets for storage auth
> > +         */
> > +        priv->secretURI = getuid() == 0 ? "secret:///system" : "secret:///session";
> 
> Again, lose the Elvis operator.
> 
> Could there be other dependencies like this one we might be missing?
> I guess we're gonna find out as people start using this :)
> 
> > +    } else {
> > +#endif /* LIBVIRTD */
> 
> The comment should be "! LIBVIRTD". Same below.
> 
> > +        VIR_DEBUG("Pointing secondary drivers to primary");
> > +        priv->interfaceConn = virObjectRef(priv->conn);
> > +        priv->networkConn = virObjectRef(priv->conn);
> > +        priv->nodedevConn = virObjectRef(priv->conn);
> > +        priv->nwfilterConn = virObjectRef(priv->conn);
> > +        priv->secretConn = virObjectRef(priv->conn);
> > +        priv->storageConn = virObjectRef(priv->conn);
> 
> Do we even need this code for the non-libvirtd case? We have listed
> all drivers, primary and secondary, above, so I can't think of any
> valid reason we'd end up here unless there's a bug, and in that case
> we'd just be masking it, no? So the structure should be more like

It is handling the remote driver case for virtproxyd, but we could
make that more explicit.

> 
>   #ifdef LIBVIRTD
>   /* point all secondary drivers to primary */
>   #else /* ! LIBVIRTD */
>   if (STREQ(type, ...) {
>       ...
>   } else if (STREQ(type, ...) {
>   ...
>   } else {
>       /* freak out */
>   }
>   #endif /* ! LIBVIRTD */

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On Mon, 2019-07-29 at 14:46 +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 29, 2019 at 10:33:08AM +0200, Andrea Bolognani wrote:
> > On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
> > > +        VIR_DEBUG("Pointing secondary drivers to primary");
> > > +        priv->interfaceConn = virObjectRef(priv->conn);
> > > +        priv->networkConn = virObjectRef(priv->conn);
> > > +        priv->nodedevConn = virObjectRef(priv->conn);
> > > +        priv->nwfilterConn = virObjectRef(priv->conn);
> > > +        priv->secretConn = virObjectRef(priv->conn);
> > > +        priv->storageConn = virObjectRef(priv->conn);
> > 
> > Do we even need this code for the non-libvirtd case? We have listed
> > all drivers, primary and secondary, above, so I can't think of any
> > valid reason we'd end up here unless there's a bug, and in that case
> > we'd just be masking it, no?
> 
> It is handling the remote driver case for virtproxyd, but we could
> make that more explicit.

Yeah, that'd be great! I didn't realize that was the case at all.

-- 
Andrea Bolognani / Red Hat / Virtualization

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