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