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

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.

> > + 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.

> > +#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.