On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
[...]
> If connecting to a remote host over any kind of ssh tunnel, for now we
> must assume only the legacy socket exists. A future patch will introduce
> a netcat replacement that is tailored for libvirt to make remote
> tunnelling easier.
>
> The configure arg '--with-remote-default-mode=legacy|direct' allows
> packagers to set a default at build time. If not given, it will default
> to direct mode.
>
> In RPM builds this is overriden, because before we can default to the
> new daemons, we must get SELinux policy written & the timeframe for that
> is unclear at this stage.
If direct mode is not ready to be the default for RPM builds, then
it's not ready to be the default for any build. Let's stick with
legacy mode as the default until we have the missing pieces you
mention and also the code has undergone more testing.
> +++ b/src/libvirt.c
> @@ -601,6 +601,30 @@ virRegisterConnectDriver(virConnectDriverPtr driver,
> +/**
> + * virHasDriverForURIScheme:
> + * @scheme: the URI scheme
> + *
> + * Determine if there is a driver registered that explicitly
> + * handles URIs with the scheme @scheme.
> + *
> + * Returns: true if a driver is registered
> + */
> +bool virHasDriverForURIScheme(const char *scheme)
Return type on a separate line.
> +{
> + size_t i, j;
One variable declaration per line. Also, leave an empty line between
variable declarations and the rest of the function.
[...]
> +++ b/src/remote/remote_driver.c
> +typedef enum {
> + /* Prefer per-driver virt*d daemons, but fallback to legacy libvirtd */
> + REMOTE_DRIVER_MODE_AUTO,
I mean, even with --with-remote-default-mode=direct this comment is
not really accurate, since the algorithm is more nuanced than this.
Please use a more neutral language.
[...]
> +VIR_ENUM_IMPL(remoteDriverMode,
> + REMOTE_DRIVER_MODE_LAST,
> + "auto", "legacy", "direct");
One enum value per line.
[...]
> @@ -92,6 +108,7 @@ VIR_ENUM_IMPL(remoteDriverTransport,
> static bool inside_daemon;
>
> +
> struct private_data {
> virMutex lock;
Unrelated whitespace change.
[...]
> +remoteGetUNIXSocketHelper(remoteDriverTransport transport,
> + const char *sock_prefix,
> + unsigned int flags)
> {
> char *sockname = NULL;
> - VIR_AUTOFREE(char *userdir);
> + VIR_AUTOFREE(char *) userdir = NULL;
Once you declare userdir correctly in the first place, this hunk
will go away :)
[...]
> @@ -758,21 +776,126 @@ remoteGetUNIXSocket(remoteDriverTransport transport,
> if (!(userdir = virGetUserRuntimeDirectory()))
> return NULL;
>
> - 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?
[...]
> +static char *
> +remoteGetUNIXSocket(remoteDriverTransport transport,
> + remoteDriverMode mode,
> + const char *driver,
> + char **daemon,
> + unsigned int flags)
> +{
[...]
> + if (mode == REMOTE_DRIVER_MODE_LEGACY) {
> + sock_name = legacy_sock_name;
> + legacy_sock_name = NULL;
> + *daemon = legacy_daemon;
> + legacy_daemon = NULL;
This is
VIR_STEAL_PTR(sock_name, legacy_sock_name);
VIR_STEAL_PTR(*daemon, legacy_daemon);
> + } else if (mode == REMOTE_DRIVER_MODE_DIRECT) {
> + if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("Cannot use direct socket mode for %s transport"),
> + remoteDriverTransportTypeToString(transport));
> + return NULL;
> + }
> +
> + 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.
> + sock_name = direct_sock_name;
> + direct_sock_name = NULL;
> + *daemon = direct_daemon;
> + direct_daemon = NULL;
This is
VIR_STEAL_PTR(sock_name, direct_sock_name);
VIR_STEAL_PTR(*daemon, direct_daemon);
> + } else {
> + virReportEnumRangeError(remoteDriverMode, mode);
> + return NULL;
> + }
See, I was going to suggest you turn this into a switch statement
anyway, but the fact that you have used virReportEnumRangeError()
here definitely seals the deal :)
[...]
> +#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.
[...]
> @@ -819,11 +942,20 @@ doRemoteOpen(virConnectPtr conn,
> VIR_AUTOFREE(char *) sshauth = NULL;
> VIR_AUTOFREE(char *) knownHostsVerify = NULL;
> VIR_AUTOFREE(char *) knownHosts = NULL;
> + VIR_AUTOFREE(char *) mode_str = NULL;
> + VIR_AUTOFREE(char *) daemon_name = NULL;
> bool sanity = true;
> bool verify = true;
> #ifndef WIN32
> bool tty = true;
> #endif
> + int mode;
This could be remoteDriverNode.
[...]
> @@ -955,6 +1087,21 @@ doRemoteOpen(virConnectPtr conn,
> goto failed;
> }
>
> + if (conf && !mode_str &&
> + virConfGetValueString(conf, "remote_mode", &mode_str) < 0)
> + goto failed;
We definitely need to document the "remote_mode" daemon configuration
knob properly, along with the "mode" URI parameter...
The rest looks good, even though I don't think I can confidently
claim that I have a clear mental picture of all the nuances involved
in routing connections to the appropriate endpoints, and I will feel
much better about the changes once they've been subjected to
extensive testing.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
