Re: [libvirt] [PATCH 36/41] remote: open secondary drivers via remote driver if needed

29 Jul 2019


      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

    