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

[Daniel P. Berrangé]

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 :|