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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
