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

[Andrea Bolognani]

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