The remoteGetUNIXSocketHelper method always returns a non-NULL string. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/remote_sockets.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index 43f78023fe..e9d865213f 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -155,12 +155,10 @@ remoteGetUNIXSocket(remoteDriverTransport transport, legacy_daemon = g_strdup("libvirtd"); - if (driver && - !(direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, ro, session))) - return NULL; + if (driver) + direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, ro, session); - if (!(legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", ro, session))) - return NULL; + legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", ro, session); if (mode == REMOTE_DRIVER_MODE_AUTO) { if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) { -- 2.31.1

We have helper methods that return boolans for ro/user/autostart properties. We then pack them into a flags parameter, and later unpack them again. This makes te code consistently use flags throughout. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/remote_driver.c | 16 ++++----------- src/remote/remote_sockets.c | 37 ++++++++++++++++------------------ src/remote/remote_sockets.h | 6 ++---- src/remote/remote_ssh_helper.c | 13 ++++++------ 4 files changed, 30 insertions(+), 42 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 6b615b9286..d03369b168 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -917,9 +917,7 @@ doRemoteOpen(virConnectPtr conn, case REMOTE_DRIVER_TRANSPORT_LIBSSH2: if (!sockname && !(sockname = remoteGetUNIXSocket(transport, mode, driver_str, - flags & REMOTE_DRIVER_OPEN_RO, - flags & REMOTE_DRIVER_OPEN_USER, - &daemon_name))) + flags, &daemon_name))) goto failed; break; @@ -1232,9 +1230,7 @@ remoteConnectOpen(virConnectPtr conn, { struct private_data *priv; int ret = VIR_DRV_OPEN_ERROR; - int rflags = 0; - bool user; - bool autostart; + unsigned int rflags = 0; char *driver = NULL; remoteDriverTransport transport; @@ -1270,15 +1266,11 @@ remoteConnectOpen(virConnectPtr conn, if (!(priv = remoteAllocPrivateData())) goto cleanup; + + remoteGetURIDaemonInfo(conn->uri, transport, &rflags); if (flags & VIR_CONNECT_RO) rflags |= REMOTE_DRIVER_OPEN_RO; - remoteGetURIDaemonInfo(conn->uri, transport, &user, &autostart); - if (user) - rflags |= REMOTE_DRIVER_OPEN_USER; - if (autostart) - rflags |= REMOTE_DRIVER_OPEN_AUTOSTART; - ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags); if (ret != VIR_DRV_OPEN_SUCCESS) { conn->privateData = NULL; diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index e9d865213f..06315fe9c0 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -101,13 +101,12 @@ remoteSplitURIScheme(virURI *uri, static char * remoteGetUNIXSocketHelper(remoteDriverTransport transport, const char *sock_prefix, - bool ro, - bool session) + unsigned int flags) { char *sockname = NULL; g_autofree char *userdir = NULL; - if (session) { + if (flags & REMOTE_DRIVER_OPEN_USER) { userdir = virGetUserRuntimeDirectory(); sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix); @@ -120,13 +119,14 @@ remoteGetUNIXSocketHelper(remoteDriverTransport transport, */ sockname = g_strdup_printf("%s/run/libvirt/%s-%s", LOCALSTATEDIR, sock_prefix, - ro ? "sock-ro" : "sock"); + flags & REMOTE_DRIVER_OPEN_RO ? + "sock-ro" : "sock"); } VIR_DEBUG("Built UNIX sockname=%s for transport=%s " - "prefix=%s ro=%d session=%d", + "prefix=%s flags=0x%x", sockname, remoteDriverTransportTypeToString(transport), - sock_prefix, ro, session); + sock_prefix, flags); return sockname; } @@ -135,8 +135,7 @@ char * remoteGetUNIXSocket(remoteDriverTransport transport, remoteDriverMode mode, const char *driver, - bool ro, - bool session, + unsigned int flags, char **daemon) { char *sock_name = NULL; @@ -145,10 +144,10 @@ remoteGetUNIXSocket(remoteDriverTransport transport, g_autofree char *direct_sock_name = NULL; g_autofree char *legacy_sock_name = NULL; - VIR_DEBUG("Choosing remote socket for transport=%s mode=%s driver=%s ro=%d session=%d", + VIR_DEBUG("Choosing remote socket for transport=%s mode=%s driver=%s flags=0x%x", remoteDriverTransportTypeToString(transport), remoteDriverModeTypeToString(mode), - driver, ro, session); + driver, flags); if (driver) direct_daemon = g_strdup_printf("virt%sd", driver); @@ -156,9 +155,9 @@ remoteGetUNIXSocket(remoteDriverTransport transport, legacy_daemon = g_strdup("libvirtd"); if (driver) - direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, ro, session); + direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, flags); - legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", ro, session); + legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", flags); if (mode == REMOTE_DRIVER_MODE_AUTO) { if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) { @@ -221,13 +220,11 @@ remoteGetUNIXSocket(remoteDriverTransport transport, void remoteGetURIDaemonInfo(virURI *uri, remoteDriverTransport transport, - bool *session, - bool *autostart) + unsigned int *flags) { const char *autostart_str = getenv("LIBVIRT_AUTOSTART"); - *session = false; - *autostart = false; + *flags = 0; /* * User session daemon is used for @@ -244,7 +241,7 @@ remoteGetURIDaemonInfo(virURI *uri, STRPREFIX(uri->scheme, "test+")) && geteuid() > 0) { VIR_DEBUG("User session daemon required"); - *session = true; + *flags |= REMOTE_DRIVER_OPEN_USER; /* * Furthermore if no servername is given, @@ -256,7 +253,7 @@ remoteGetURIDaemonInfo(virURI *uri, (!autostart_str || STRNEQ(autostart_str, "0"))) { VIR_DEBUG("Try daemon autostart"); - *autostart = true; + *flags |= REMOTE_DRIVER_OPEN_AUTOSTART; } } @@ -268,10 +265,10 @@ remoteGetURIDaemonInfo(virURI *uri, VIR_DEBUG("Auto-probe remote URI"); if (geteuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); - *session = true; + *flags |= REMOTE_DRIVER_OPEN_USER; if (!autostart_str || STRNEQ(autostart_str, "0")) - *autostart = true; + *flags |= REMOTE_DRIVER_OPEN_AUTOSTART; } } } diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h index 749389700e..2331f81425 100644 --- a/src/remote/remote_sockets.h +++ b/src/remote/remote_sockets.h @@ -66,12 +66,10 @@ char * remoteGetUNIXSocket(remoteDriverTransport transport, remoteDriverMode mode, const char *driver, - bool ro, - bool session, + unsigned int flags, /* remoteDriverOpenFlags */ char **daemon); void remoteGetURIDaemonInfo(virURI *uri, remoteDriverTransport transport, - bool *session, - bool *autostart); + unsigned int *flags); /* remoteDriverOpenFlags */ diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index 7934e14509..7123fc6d00 100644 --- a/src/remote/remote_ssh_helper.c +++ b/src/remote/remote_ssh_helper.c @@ -355,8 +355,6 @@ int main(int argc, char **argv) g_autoptr(virURI) uri = NULL; g_autofree char *driver = NULL; remoteDriverTransport transport; - bool user = false; - bool autostart = false; gboolean version = false; gboolean readonly = false; g_autofree char *sock_path = NULL; @@ -369,6 +367,7 @@ int main(int argc, char **argv) { "version", 'V', 0, G_OPTION_ARG_NONE, &version, "Display version information", NULL }, { NULL, '\0', 0, 0, NULL, NULL, NULL } }; + unsigned int flags; context = g_option_context_new("- libvirt socket proxy"); g_option_context_add_main_entries(context, entries, PACKAGE); @@ -422,16 +421,18 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } - remoteGetURIDaemonInfo(uri, transport, &user, &autostart); + remoteGetURIDaemonInfo(uri, transport, &flags); + if (readonly) + flags |= REMOTE_DRIVER_OPEN_RO; sock_path = remoteGetUNIXSocket(transport, REMOTE_DRIVER_MODE_AUTO, driver, - !!readonly, - user, + flags, &daemon_name); - if (virNetSocketNewConnectUNIX(sock_path, autostart, daemon_name, &sock) < 0) { + if (virNetSocketNewConnectUNIX(sock_path, flags & REMOTE_DRIVER_OPEN_AUTOSTART, + daemon_name, &sock) < 0) { g_printerr(_("%s: cannot connect to '%s': %s\n"), argv[0], sock_path, virGetLastErrorMessage()); exit(EXIT_FAILURE); -- 2.31.1

On a Wednesday in 2021, Daniel P. Berrangé wrote: This removes the last usage of daemonPath in this function: src/remote/remote_driver.c:733:22: error: unused variable 'daemonPath' [-Werror,-Wunused-variable] g_autofree char *daemonPath = NULL; Jano

The "spawnDaemon" and "binary" parameters are co-dependant, with the latter non-NULL, if-and-only-if the former is true. Getting rid of the "spawnDaemon" parameter simplifies life for the callers and eliminates an error checking scenario. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/admin/admin_remote.c | 2 +- src/locking/lock_driver_lockd.c | 1 - src/logging/log_manager.c | 1 - src/lxc/lxc_monitor.c | 2 +- src/qemu/qemu_migration.c | 3 +-- src/remote/remote_driver.c | 1 - src/remote/remote_ssh_helper.c | 3 +-- src/rpc/virnetclient.c | 5 ++--- src/rpc/virnetclient.h | 3 +-- src/rpc/virnetsocket.c | 26 +++++++------------------- src/rpc/virnetsocket.h | 3 +-- tests/virnetsockettest.c | 4 ++-- 12 files changed, 17 insertions(+), 37 deletions(-) diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index 55ac81c5de..83a6be2b97 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -222,7 +222,7 @@ remoteAdminPrivNew(const char *sock_path) if (!(priv = virObjectLockableNew(remoteAdminPrivClass))) goto error; - if (!(priv->client = virNetClientNewUNIX(sock_path, false, NULL))) + if (!(priv->client = virNetClientNewUNIX(sock_path, NULL))) goto error; if (!(priv->program = virNetClientProgramNew(ADMIN_PROGRAM, diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 823b918db3..3a7386af30 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -208,7 +208,6 @@ static virNetClient *virLockManagerLockDaemonConnectionNew(bool privileged, goto error; if (!(client = virNetClientNewUNIX(lockdpath, - daemonPath != NULL, daemonPath))) goto error; diff --git a/src/logging/log_manager.c b/src/logging/log_manager.c index f66ebda495..e605ed9930 100644 --- a/src/logging/log_manager.c +++ b/src/logging/log_manager.c @@ -79,7 +79,6 @@ virLogManagerConnect(bool privileged, goto error; if (!(client = virNetClientNewUNIX(logdpath, - daemonPath != NULL, daemonPath))) goto error; diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c index 445bea281d..d8ef489a01 100644 --- a/src/lxc/lxc_monitor.c +++ b/src/lxc/lxc_monitor.c @@ -151,7 +151,7 @@ virLXCMonitor *virLXCMonitorNew(virDomainObj *vm, sockpath = g_strdup_printf("%s/%s.sock", socketdir, vm->def->name); - if (!(mon->client = virNetClientNewUNIX(sockpath, false, NULL))) + if (!(mon->client = virNetClientNewUNIX(sockpath, NULL))) goto error; if (virNetClientRegisterAsyncIO(mon->client) < 0) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3b0026a28e..a199758feb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3761,8 +3761,7 @@ qemuMigrationSrcConnect(virQEMUDriver *driver, break; case MIGRATION_DEST_CONNECT_SOCKET: if (virNetSocketNewConnectUNIX(spec->dest.socket.path, - false, NULL, - &sock) == 0) { + NULL, &sock) == 0) { fd_qemu = virNetSocketDupFD(sock, true); virObjectUnref(sock); } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e4e412dd01..9ca4348f6f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -997,7 +997,6 @@ doRemoteOpen(virConnectPtr conn, #ifndef WIN32 case REMOTE_DRIVER_TRANSPORT_UNIX: if (!(priv->client = virNetClientNewUNIX(sockname, - flags & REMOTE_DRIVER_OPEN_AUTOSTART, daemon_path))) goto failed; diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c index 2a24f2df96..0945b90331 100644 --- a/src/remote/remote_ssh_helper.c +++ b/src/remote/remote_ssh_helper.c @@ -431,8 +431,7 @@ int main(int argc, char **argv) flags, &daemon_path); - if (virNetSocketNewConnectUNIX(sock_path, flags & REMOTE_DRIVER_OPEN_AUTOSTART, - daemon_path, &sock) < 0) { + if (virNetSocketNewConnectUNIX(sock_path, daemon_path, &sock) < 0) { g_printerr(_("%s: cannot connect to '%s': %s\n"), argv[0], sock_path, virGetLastErrorMessage()); exit(EXIT_FAILURE); diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 3797a6097b..ffe2f343f9 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -366,12 +366,11 @@ virNetClientFindDefaultSshKey(const char *homedir, char **retPath) virNetClient *virNetClientNewUNIX(const char *path, - bool spawnDaemon, - const char *binary) + const char *spawnDaemonPath) { virNetSocket *sock; - if (virNetSocketNewConnectUNIX(path, spawnDaemon, binary, &sock) < 0) + if (virNetSocketNewConnectUNIX(path, spawnDaemonPath, &sock) < 0) return NULL; return virNetClientNew(sock, NULL); diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index c6ad59cf0d..5048033325 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -48,8 +48,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, bool readonly); virNetClient *virNetClientNewUNIX(const char *path, - bool spawnDaemon, - const char *binary); + const char *spawnDaemonPath); virNetClient *virNetClientNewTCP(const char *nodename, const char *service, diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c762b605a5..c3fae8b626 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -664,8 +664,7 @@ int virNetSocketNewConnectTCP(const char *nodename, #ifndef WIN32 int virNetSocketNewConnectUNIX(const char *path, - bool spawnDaemon, - const char *binary, + const char *spawnDaemonPath, virNetSocket **retsock) { char *lockpath = NULL; @@ -678,25 +677,15 @@ int virNetSocketNewConnectUNIX(const char *path, int ret = -1; bool daemonLaunched = false; - VIR_DEBUG("path=%s spawnDaemon=%d binary=%s", path, spawnDaemon, - NULLSTR(binary)); + VIR_DEBUG("path=%s spawnDaemonPath=%s", path, NULLSTR(spawnDaemonPath)); memset(&localAddr, 0, sizeof(localAddr)); memset(&remoteAddr, 0, sizeof(remoteAddr)); remoteAddr.len = sizeof(remoteAddr.data.un); - if (spawnDaemon) { - g_autofree char *binname = NULL; - - if (!binary) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Auto-spawn of daemon requested, " - "but no binary specified")); - goto cleanup; - } - - binname = g_path_get_basename(binary); + if (spawnDaemonPath) { + g_autofree char *binname = g_path_get_basename(spawnDaemonPath); rundir = virGetUserRuntimeDirectory(); if (g_mkdir_with_parents(rundir, 0700) < 0) { @@ -741,7 +730,7 @@ int virNetSocketNewConnectUNIX(const char *path, VIR_DEBUG("connect() failed: retries=%d errno=%d", retries, errno); retries--; - if (!spawnDaemon || + if (!spawnDaemonPath || retries == 0 || (errno != ENOENT && errno != ECONNREFUSED)) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), @@ -750,7 +739,7 @@ int virNetSocketNewConnectUNIX(const char *path, } if (!daemonLaunched) { - if (virNetSocketForkDaemon(binary) < 0) + if (virNetSocketForkDaemon(spawnDaemonPath) < 0) goto cleanup; daemonLaunched = true; @@ -785,8 +774,7 @@ int virNetSocketNewConnectUNIX(const char *path, } #else int virNetSocketNewConnectUNIX(const char *path G_GNUC_UNUSED, - bool spawnDaemon G_GNUC_UNUSED, - const char *binary G_GNUC_UNUSED, + const char *spawnDaemonPath, virNetSocket **retsock G_GNUC_UNUSED) { virReportSystemError(ENOSYS, "%s", diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 2dc4d06f42..6fef8d30b3 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -65,8 +65,7 @@ int virNetSocketNewConnectTCP(const char *nodename, virNetSocket **addr); int virNetSocketNewConnectUNIX(const char *path, - bool spawnDaemon, - const char *binary, + const char *spawnDaemonPath, virNetSocket **addr); int virNetSocketNewConnectCommand(virCommand *cmd, diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 6afdcbdbc7..8059c6cbb0 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -131,7 +131,7 @@ testSocketClient(void *opaque) virNetSocket *csock = NULL; if (data->path) { - if (virNetSocketNewConnectUNIX(data->path, false, + if (virNetSocketNewConnectUNIX(data->path, NULL, &csock) < 0) return; } else { @@ -339,7 +339,7 @@ static int testSocketUNIXAddrs(const void *data G_GNUC_UNUSED) if (virNetSocketListen(lsock, 0) < 0) goto cleanup; - if (virNetSocketNewConnectUNIX(path, false, NULL, &csock) < 0) + if (virNetSocketNewConnectUNIX(path, NULL, &csock) < 0) goto cleanup; if (STRNEQ(virNetSocketLocalAddrStringSASL(csock), "127.0.0.1;0")) { -- 2.31.1

While we couldn't historically connect to the remote session daemon automatically, we do allow the user to set an explicit socket path to enable the connections to work. This ability was accidentally lost in commit f8ec7c842df9e40c6607eae9b0223766cb226336 Author: Daniel P. Berrangé <berrange(a)redhat.com&gt; Date: Wed Jul 8 17:03:38 2020 +0100 rpc: use new virt-ssh-helper binary for remote tunnelling We need to force use of 'netcat' when a 'socket' path is given in the URI parameters. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/remote/remote_driver.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 6a881fff28..da672b0d00 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -868,10 +868,33 @@ doRemoteOpen(virConnectPtr conn, goto failed; } } else { - /* Historically we didn't allow ssh tunnel with session mode, - * since we can't construct the accurate path remotely, - * so we can default to modern virt-ssh-helper */ - if (flags & REMOTE_DRIVER_OPEN_USER) + /* + * Goal is to maximise usage of virt-ssh-helper + * + * Historically tunnelling access for the session mode + * daemon did not automatically work, since we can't + * construct the accurate path remotely. Users could, + * however, specify the 'socket' URI parameter explicitly. + * + * If we see a 'socket' path we must always use netcat, + * since virt-ssh-helper won't handle an explicit socket. + * Autostart won't work for session mode, so we assume + * user started it manually on the remote host in this + * case. + * + * If we have a 'session' URI without explicit socket, + * we can just assume the use of virt-ssh-helper, since + * logic for constructing socket paths relies on env + * envs whose values have no guarantee of matching those + * on the remote host. It was explicitly blocked with an + * error check before virt-ssh-helper was introduced. + * + * For 'system' URIs, we need to try virt-ssh-helper but + * with fallback to netcat for back compat. + */ + if (sockname) + proxy = VIR_NET_CLIENT_PROXY_NETCAT; + else if (flags & REMOTE_DRIVER_OPEN_USER) proxy = VIR_NET_CLIENT_PROXY_NATIVE; else proxy = VIR_NET_CLIENT_PROXY_AUTO; -- 2.31.1