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

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 */