[libvirt] [PATCH 0/3] Use virFileFindResource for libvirtd/virtlockd autostart
This small series converts the two places where we autostart libvirtd/virtlockd over to use virFileFindResource so that ./tools/virsh -c qemu:///session will use the daemons from the source tree instead of /usr/sbin Daniel P. Berrange (3): Use virFileFindResource to locate libvirtd daemon Use virFileFindResource to locate virtlockd daemon Make autostart of virtlockd actually work src/locking/lock_daemon.c | 20 +++++++++++++++++++ src/locking/lock_driver_lockd.c | 25 +++++++---------------- src/remote/remote_driver.c | 44 ++++++----------------------------------- 3 files changed, 33 insertions(+), 56 deletions(-) -- 1.9.0
Make the remote driver use virFileFindResource to find the libvirt daemon path, so that it executes the in-builddir daemon if run from source tree. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/remote/remote_driver.c | 44 ++++++-------------------------------------- 1 file changed, 6 insertions(+), 38 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ed7dde6..d9be756 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -174,37 +174,6 @@ remoteStateInitialize(bool privileged ATTRIBUTE_UNUSED, } #endif -#ifndef WIN32 -/** - * remoteFindDaemonPath: - * - * Tries to find the path to the libvirtd binary. - * - * Returns path on success or NULL in case of error. - */ -static const char * -remoteFindDaemonPath(void) -{ - static const char *serverPaths[] = { - SBINDIR "/libvirtd", - SBINDIR "/libvirtd_dbg", - NULL - }; - size_t i; - const char *customDaemon = virGetEnvBlockSUID("LIBVIRTD_PATH"); - - if (customDaemon) - return customDaemon; - - for (i = 0; serverPaths[i]; i++) { - if (virFileIsExecutable(serverPaths[i])) { - return serverPaths[i]; - } - } - return NULL; -} -#endif - static void remoteDomainBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, @@ -887,14 +856,13 @@ doRemoteOpen(virConnectPtr conn, } if ((flags & VIR_DRV_OPEN_REMOTE_AUTOSTART) && - !(daemonPath = remoteFindDaemonPath())) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to locate libvirtd daemon in %s " - "(to override, set $LIBVIRTD_PATH to the " - "name of the libvirtd binary)"), - SBINDIR); + !(daemonPath = virFileFindResourceFull("libvirtd", + NULL, NULL, + "daemon", + SBINDIR, + "LIBVIRTD_PATH"))) goto failed; - } + if (!(priv->client = virNetClientNewUNIX(sockname, flags & VIR_DRV_OPEN_REMOTE_AUTOSTART, daemonPath))) -- 1.9.0
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
Make the remote driver use virFileFindResource to find the libvirt daemon path, so that it executes the in-builddir daemon if run from source tree.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/remote/remote_driver.c | 44 ++++++-------------------------------------- 1 file changed, 6 insertions(+), 38 deletions(-)
Nice diffstat - proof that we made a good factorization.
-static const char * -remoteFindDaemonPath(void) -{ - static const char *serverPaths[] = { - SBINDIR "/libvirtd", - SBINDIR "/libvirtd_dbg", - NULL - }; - size_t i; - const char *customDaemon = virGetEnvBlockSUID("LIBVIRTD_PATH"); - - if (customDaemon) - return customDaemon;
The old code assumes LIBVIRTD_PATH provides "/path/to/libvirtd"
- virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to locate libvirtd daemon in %s " - "(to override, set $LIBVIRTD_PATH to the " - "name of the libvirtd binary)"), - SBINDIR); + !(daemonPath = virFileFindResourceFull("libvirtd", + NULL, NULL, + "daemon", + SBINDIR, + "LIBVIRTD_PATH")))
the new code assumes LIBVIRTD_PATH provides "/path/to" which contains libvirtd. Don't know how many users this will impact, but the only former documentation of LIBVIRTD_PATH was in the error message. I can live with the change in semantics, since it is not formally documented on the web page as something users would normally fiddle with. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Apr 25, 2014 at 08:01:58AM -0600, Eric Blake wrote:
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
Make the remote driver use virFileFindResource to find the libvirt daemon path, so that it executes the in-builddir daemon if run from source tree.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/remote/remote_driver.c | 44 ++++++-------------------------------------- 1 file changed, 6 insertions(+), 38 deletions(-)
Nice diffstat - proof that we made a good factorization.
-static const char * -remoteFindDaemonPath(void) -{ - static const char *serverPaths[] = { - SBINDIR "/libvirtd", - SBINDIR "/libvirtd_dbg", - NULL - }; - size_t i; - const char *customDaemon = virGetEnvBlockSUID("LIBVIRTD_PATH"); - - if (customDaemon) - return customDaemon;
The old code assumes LIBVIRTD_PATH provides "/path/to/libvirtd"
- virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to locate libvirtd daemon in %s " - "(to override, set $LIBVIRTD_PATH to the " - "name of the libvirtd binary)"), - SBINDIR); + !(daemonPath = virFileFindResourceFull("libvirtd", + NULL, NULL, + "daemon", + SBINDIR, + "LIBVIRTD_PATH")))
the new code assumes LIBVIRTD_PATH provides "/path/to" which contains libvirtd.
Don't know how many users this will impact, but the only former documentation of LIBVIRTD_PATH was in the error message. I can live with the change in semantics, since it is not formally documented on the web page as something users would normally fiddle with.
The 'run' script uses this, but we can just delete those lines from the 'run' script now, since we'll "do the right thing" automatically Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/25/2014 08:25 AM, Daniel P. Berrange wrote:
On Fri, Apr 25, 2014 at 08:01:58AM -0600, Eric Blake wrote:
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
Make the remote driver use virFileFindResource to find the libvirt daemon path, so that it executes the in-builddir daemon if run from source tree.
the new code assumes LIBVIRTD_PATH provides "/path/to" which contains libvirtd.
Don't know how many users this will impact, but the only former documentation of LIBVIRTD_PATH was in the error message. I can live with the change in semantics, since it is not formally documented on the web page as something users would normally fiddle with.
The 'run' script uses this, but we can just delete those lines from the 'run' script now, since we'll "do the right thing" automatically
In fact, you HAVE to fix the run script, or using it will break :) (I just tested it now, on a virgin VM with no libvirt installed and just an in-tree build: 'tools/virsh list' works now where it previously didn't, but without a tweak to run.in, './run tools/virsh list' started failing. So amendment to my earlier posting: my ACK is conditional on this being squashed in (and similarly for 2/3): diff --git i/run.in w/run.in index 2211f24..c8f12a5 100644 --- i/run.in +++ w/run.in @@ -1,6 +1,6 @@ #!/bin/sh # libvirt 'run' programs locally script -# Copyright (C) 2012-2013 Red Hat, Inc. +# Copyright (C) 2012-2014 Red Hat, Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -56,7 +56,6 @@ export LD_LIBRARY_PATH export LIBVIRT_DRIVER_DIR="$b/src/.libs" export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs" export VIRTLOCKD_PATH="$b/src/virtlockd" -export LIBVIRTD_PATH="$b/daemon/libvirtd" # This is a cheap way to find some use-after-free and uninitialized # read problems when using glibc. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Apr 25, 2014 at 10:52:23AM -0600, Eric Blake wrote:
On 04/25/2014 08:25 AM, Daniel P. Berrange wrote:
On Fri, Apr 25, 2014 at 08:01:58AM -0600, Eric Blake wrote:
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
Make the remote driver use virFileFindResource to find the libvirt daemon path, so that it executes the in-builddir daemon if run from source tree.
the new code assumes LIBVIRTD_PATH provides "/path/to" which contains libvirtd.
Don't know how many users this will impact, but the only former documentation of LIBVIRTD_PATH was in the error message. I can live with the change in semantics, since it is not formally documented on the web page as something users would normally fiddle with.
The 'run' script uses this, but we can just delete those lines from the 'run' script now, since we'll "do the right thing" automatically
In fact, you HAVE to fix the run script, or using it will break :) (I just tested it now, on a virgin VM with no libvirt installed and just an in-tree build: 'tools/virsh list' works now where it previously didn't, but without a tweak to run.in, './run tools/virsh list' started failing.
So amendment to my earlier posting: my ACK is conditional on this being squashed in (and similarly for 2/3):
diff --git i/run.in w/run.in index 2211f24..c8f12a5 100644 --- i/run.in +++ w/run.in @@ -1,6 +1,6 @@ #!/bin/sh # libvirt 'run' programs locally script -# Copyright (C) 2012-2013 Red Hat, Inc. +# Copyright (C) 2012-2014 Red Hat, Inc. # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -56,7 +56,6 @@ export LD_LIBRARY_PATH export LIBVIRT_DRIVER_DIR="$b/src/.libs" export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs" export VIRTLOCKD_PATH="$b/src/virtlockd" -export LIBVIRTD_PATH="$b/daemon/libvirtd"
We actually don't want to delete it because this script serves dual purposes. It is supposed to be usable for running apps outside libvirt. ie ./run virt-manager Such apps won't trigger the automatic path change, so we must just just alter the env var, not delete it Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/25/2014 10:54 AM, Daniel P. Berrange wrote:
# This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -56,7 +56,6 @@ export LD_LIBRARY_PATH export LIBVIRT_DRIVER_DIR="$b/src/.libs" export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs" export VIRTLOCKD_PATH="$b/src/virtlockd" -export LIBVIRTD_PATH="$b/daemon/libvirtd"
We actually don't want to delete it because this script serves dual purposes. It is supposed to be usable for running apps outside libvirt. ie
./run virt-manager
Such apps won't trigger the automatic path change, so we must just just alter the env var, not delete it
Fair enough. The alteration is thus: export LIBVIRTD_PATH="$b/daemon" -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Make the lock plugin use virFileFindResource to find the virtlockd daemon path, so that it executes the in-builddir daemon if run from source tree. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_driver_lockd.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index afa3bac..c67bda6 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -81,22 +81,6 @@ struct _virLockManagerLockDaemonDriver { static virLockManagerLockDaemonDriverPtr driver = NULL; -#define VIRTLOCKD_PATH SBINDIR "/virtlockd" - -static const char * -virLockManagerLockDaemonFindDaemon(void) -{ - const char *customDaemon = virGetEnvBlockSUID("VIRTLOCKD_PATH"); - - if (customDaemon) - return customDaemon; - - if (virFileIsExecutable(VIRTLOCKD_PATH)) - return VIRTLOCKD_PATH; - - return NULL; -} - static int virLockManagerLockDaemonLoadConfig(const char *configFile) { virConfPtr conf; @@ -266,8 +250,13 @@ static virNetClientPtr virLockManagerLockDaemonConnectionNew(bool privileged, if (!(lockdpath = virLockManagerLockDaemonPath(privileged))) goto error; - if (!privileged) - daemonPath = virLockManagerLockDaemonFindDaemon(); + if (!privileged && + !(daemonPath = virFileFindResourceFull("virtlockd", + NULL, NULL, + "src", + SBINDIR, + "VIRTLOCKD_PATH"))) + goto error; if (!(client = virNetClientNewUNIX(lockdpath, daemonPath != NULL, -- 1.9.0
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
Make the lock plugin use virFileFindResource to find the virtlockd daemon path, so that it executes the in-builddir daemon if run from source tree.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_driver_lockd.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)
-static const char * -virLockManagerLockDaemonFindDaemon(void) -{ - const char *customDaemon = virGetEnvBlockSUID("VIRTLOCKD_PATH"); - - if (customDaemon) - return customDaemon; -
As in 1/3, this is a change in semantics, where VIRTLOCKD_PATH used to be "/path/to/virtlockd" and is now "/path/to"; but was otherwise undocumented. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The virnetsocket.c API is hardcoded to pass --timeout=30 to any daemon it auto-starts. For inexplicable reasons the virtlockd daemon did not implement the --timeout option, so it would immediately exit on autostart with an error. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_daemon.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 969f901..3379f29 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1143,6 +1143,7 @@ virLockDaemonUsage(const char *argv0, bool privileged) " -h | --help Display program help:\n" " -v | --verbose Verbose messages.\n" " -d | --daemon Run as a daemon & write PID file.\n" + " -t | --timeout <secs> Exit after timeout period.\n" " -f | --config <file> Configuration file.\n" " -V | --version Display version information.\n" " -p | --pid-file <file> Change name of PID file.\n" @@ -1195,6 +1196,7 @@ int main(int argc, char **argv) { char *pid_file = NULL; int pid_file_fd = -1; char *sock_file = NULL; + int timeout = -1; /* -t: Shutdown timeout */ char *state_file = NULL; bool implicit_conf = false; mode_t old_umask; @@ -1206,6 +1208,7 @@ int main(int argc, char **argv) { { "verbose", no_argument, &verbose, 'v'}, { "daemon", no_argument, &godaemon, 'd'}, { "config", required_argument, NULL, 'f'}, + { "timeout", required_argument, NULL, 't'}, { "pid-file", required_argument, NULL, 'p'}, { "version", no_argument, NULL, 'V' }, { "help", no_argument, NULL, 'h' }, @@ -1226,6 +1229,7 @@ int main(int argc, char **argv) { while (1) { int optidx = 0; int c; + char *tmp; c = getopt_long(argc, argv, "ldf:p:t:vVh", opts, &optidx); @@ -1244,6 +1248,16 @@ int main(int argc, char **argv) { godaemon = 1; break; + case 't': + if (virStrToLong_i(optarg, &tmp, 10, &timeout) != 0 + || timeout <= 0 + /* Ensure that we can multiply by 1000 without overflowing. */ + || timeout > INT_MAX / 1000) { + VIR_ERROR(_("Invalid value for timeout")); + exit(EXIT_FAILURE); + } + break; + case 'p': VIR_FREE(pid_file); if (VIR_STRDUP_QUIET(pid_file, optarg) < 0) @@ -1407,6 +1421,12 @@ int main(int argc, char **argv) { } } + if (timeout != -1) { + VIR_DEBUG("Registering shutdown timeout %d", timeout); + virNetServerAutoShutdown(lockDaemon->srv, + timeout); + } + if ((virLockDaemonSetupSignals(lockDaemon->srv)) < 0) { ret = VIR_LOCK_DAEMON_ERR_SIGNAL; goto cleanup; -- 1.9.0
On 04/25/2014 05:28 AM, Daniel P. Berrange wrote:
The virnetsocket.c API is hardcoded to pass --timeout=30 to any daemon it auto-starts. For inexplicable reasons the virtlockd daemon did not implement the --timeout option, so it would immediately exit on autostart with an error.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/locking/lock_daemon.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
@@ -1226,6 +1229,7 @@ int main(int argc, char **argv) { while (1) { int optidx = 0; int c; + char *tmp;
c = getopt_long(argc, argv, "ldf:p:t:vVh", opts, &optidx);
Oh my - we were previously accepting -t (but not --timeout) then doing nothing with it.
@@ -1244,6 +1248,16 @@ int main(int argc, char **argv) { godaemon = 1; break;
+ case 't':
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake