3:33 a.m.
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