[libvirt] [PATCH 40/41] remote: switch to connect to per-driver daemons by default

[Andrea Bolognani]

On Mon, 2019-07-29 at 16:49 +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 29, 2019 at 02:32:31PM +0200, Andrea Bolognani wrote:
> > On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
> > > -        if (virAsprintf(&sockname,
> > > -                        "%s/" LIBVIRTD_USER_UNIX_SOCKET, userdir) < 0)
> > > +        if (virAsprintf(&sockname, "%s/%s-sock",
> > > +                        userdir, sock_prefix) < 0)
> > 
> > I kinda just noticed, but don't we support R/O connections in
> > session mode?
> 
> The client app is required to be the same user ID as the daemon.
> As such there's no meaningful security separation between the
> two from a DAC pov, so R/O socket was deemed to be a waste of
> time.
> 
> If you had SELinux strictly locking things down it could be
> considered slightly more secure, but no one has ever cared
> enough to enable it.

Alright.

> 
> 
> > > +        if (!direct_sock_name) {
> > > +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > > +                           _("Cannot use direct socket mode if no URI is set"));
> > > +            return NULL;
> > > +        }
> > 
> > Is the error message accurate? We should be way past making sure we
> > have a URI to work with by now.
> 
> We'll only hit  direct_sock_name == NULL, if driver == NULL.
> 
> We'll only hit driver == NULL, if the original URI was NULL.

Okay.

> > > +#ifndef WIN32
> > > +static const char *
> > > +remoteGetDaemonPathEnv(void)
> > > +{
> > > +    /* We prefer a VIRTD_PATH env var to use for all daemons,
> > > +     * but if it is not set we will fallback to LIBVIRTD_PATH
> > > +     * for previous behaviour
> > > +     */
> > > +    if (virGetEnvBlockSUID("VIRTD_PATH") != NULL) {
> > > +        return "VIRTD_PATH";
> > > +    } else {
> > > +        return "LIBVIRTD_PATH";
> > > +    }
> > > +}
> > > +#endif /* WIN32 */
> > 
> > I don't think this function needs to be guarded by 'ifndef WIN32':
> > we already do so at the call site, and AFAICT there's nothing in the
> > helper itself that warrants compiling it out on Windows.
> 
> It is a static function, so will trigger an unused  function warning.

You've convinced me, so

  Reviewed-by: Andrea Bolognani <abologna at redhat.com>

if you address the style issues and most importantly switch the
default mode to legacy.

-- 
Andrea Bolognani / Red Hat / Virtualization