[libvirt] [PATCH 0/6] Some virtlockd improvements
Hi, This patch series fixes up a few problems encountered while testing out virtlockd. The biggest problem was that reloading virtlockd through systemd or its initscript would send it a SIGHUP, not a SIGUSR1 (which causes it to re-exec itself). This would simply kill it. Patches 1-3 clean this up by having SIGHUP behave the same as SIGUSR1. The systemd unit and initscript have to use SIGUSR1, though, since they may be called upon to signal an old virtlockd binary. When run from the initscript graceful re-exec didn't work properly anyway, as virtlockd assumed argv[0] was a full path to its binary. The initscript didn't provide a full path. Patches 4-5 make virtlockd use execvp() to re-exec itself, which does a PATH lookup if necessary. Patch 6 isn't really virtlockd-specific. It moves the reloading of virtlockd and libvirtd via initscripts into the libvirt-daemon package's %postun scriptlet (which is already where reloading is done with systemd). The problem with reloading libvirt-daemon in %post is that it restarts *before* all of the libvirt-daemon-driver-* packages have been updated in the same RPM transaction, which means it keeps using the old libraries (or fails to load them due to missing symbols). Michael Chapman (6): virtlockd: improve systemd units virtlockd: improve initscripts virtlockd: treat SIGHUP like SIGUSR1 virtlockd: use common exit path when out-of-memory virtlockd: make re-exec more robust spec: clean up libvirtd and virtlockd service mgmt daemon/libvirtd.init.in | 2 ++ libvirt.spec.in | 43 +++++++++++----------- src/locking/lock_daemon.c | 77 ++++++++++++++++++++++++++++++---------- src/locking/virtlockd.init.in | 11 +++--- src/locking/virtlockd.service.in | 7 ++-- src/locking/virtlockd.socket.in | 2 +- 6 files changed, 95 insertions(+), 47 deletions(-) -- 1.8.4.2
- Pass VIRTLOCKD_ARGS through to virtlockd. - Use SIGUSR1, not SIGHUP, in ExecReload. At present, virtlockd only responds to the former. - Have "systemctl enable virtlockd.service" enable virtlockd.socket, rather than throw an error. - Make virtlockd.socket wanted by sockets.target, rather than multi-user.target. This is consistent with other socket units in Fedora, and it ensures that the socket is available before libvirtd is started. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/virtlockd.service.in | 7 +++++-- src/locking/virtlockd.socket.in | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in index 0ef9923..a1298a3 100644 --- a/src/locking/virtlockd.service.in +++ b/src/locking/virtlockd.service.in @@ -4,9 +4,12 @@ Requires=virtlockd.socket [Service] EnvironmentFile=-/etc/sysconfig/virtlockd -ExecStart=@sbindir@/virtlockd -ExecReload=/bin/kill -HUP $MAINPID +ExecStart=@sbindir@/virtlockd $VIRTLOCKD_ARGS +ExecReload=/bin/kill -USR1 $MAINPID # Loosing the locks is a really bad thing that will # cause the machine to be fenced (rebooted), so make # sure we discourage OOM killer OOMScoreAdjust=-900 + +[Install] +Also=virtlockd.socket diff --git a/src/locking/virtlockd.socket.in b/src/locking/virtlockd.socket.in index a38b1f4..9808bbb 100644 --- a/src/locking/virtlockd.socket.in +++ b/src/locking/virtlockd.socket.in @@ -5,4 +5,4 @@ Description=Virtual machine lock manager socket ListenStream=@localstatedir@/run/libvirt/virtlockd-sock [Install] -WantedBy=multi-user.target +WantedBy=sockets.target -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
- Pass VIRTLOCKD_ARGS through to virtlockd.
- Use SIGUSR1, not SIGHUP, in ExecReload. At present, virtlockd only responds to the former.
- Have "systemctl enable virtlockd.service" enable virtlockd.socket, rather than throw an error.
- Make virtlockd.socket wanted by sockets.target, rather than multi-user.target. This is consistent with other socket units in Fedora, and it ensures that the socket is available before libvirtd is started.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/virtlockd.service.in | 7 +++++-- src/locking/virtlockd.socket.in | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/locking/virtlockd.service.in b/src/locking/virtlockd.service.in index 0ef9923..a1298a3 100644 --- a/src/locking/virtlockd.service.in +++ b/src/locking/virtlockd.service.in @@ -4,9 +4,12 @@ Requires=virtlockd.socket
[Service] EnvironmentFile=-/etc/sysconfig/virtlockd -ExecStart=@sbindir@/virtlockd -ExecReload=/bin/kill -HUP $MAINPID +ExecStart=@sbindir@/virtlockd $VIRTLOCKD_ARGS +ExecReload=/bin/kill -USR1 $MAINPID # Loosing the locks is a really bad thing that will # cause the machine to be fenced (rebooted), so make # sure we discourage OOM killer OOMScoreAdjust=-900 + +[Install] +Also=virtlockd.socket diff --git a/src/locking/virtlockd.socket.in b/src/locking/virtlockd.socket.in index a38b1f4..9808bbb 100644 --- a/src/locking/virtlockd.socket.in +++ b/src/locking/virtlockd.socket.in @@ -5,4 +5,4 @@ Description=Virtual machine lock manager socket ListenStream=@localstatedir@/run/libvirt/virtlockd-sock
[Install] -WantedBy=multi-user.target +WantedBy=sockets.target
ACKed & pushed. Michal
- Use SIGUSR1, not SIGHUP, on reload. At present, virtlockd only responds to the former. - Fix PID file for virtlockd. - Do not start virtlockd in any runlevels by default. It needs to be explicitly selected in libvirt's qemu.conf anyway, so there is no need to have it running on all systems regardless. - Fix chkconfig priorities to ensure virtlockd is started before libvirtd is started, and stopped after libvirtd is stopped. - Add "Should-Start: virtlockd" to the libvirtd initscript's LSB header, for the same reason. - Add "Default-Stop" to both libvirtd and virtlockd initscripts. LSB does not guarantee that this defaults to the inverse of "Default-Start". Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- daemon/libvirtd.init.in | 2 ++ src/locking/virtlockd.init.in | 11 ++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in index f66ddad..ed42195 100644 --- a/daemon/libvirtd.init.in +++ b/daemon/libvirtd.init.in @@ -9,9 +9,11 @@ # Should-Start: $named # Should-Start: xend # Should-Start: avahi-daemon +# Should-Start: virtlockd # Required-Stop: $network messagebus # Should-Stop: $named # Default-Start: 3 4 5 +# Default-Stop: 0 1 2 6 # Short-Description: daemon for libvirt virtualization API # Description: This is a daemon for managing guest instances # and libvirt virtual networks diff --git a/src/locking/virtlockd.init.in b/src/locking/virtlockd.init.in index 1adea07..0913727 100644 --- a/src/locking/virtlockd.init.in +++ b/src/locking/virtlockd.init.in @@ -5,7 +5,8 @@ # ### BEGIN INIT INFO # Provides: virtlockd -# Default-Start: 3 4 5 +# Default-Start: +# Default-Stop: 0 1 2 3 4 5 6 # Short-Description: virtual machine lock manager # Description: This is a daemon for managing locks # on virtual machine disk images @@ -15,12 +16,12 @@ # # virtlockd: virtual machine lock manager # -# chkconfig: 345 97 03 +# chkconfig: - 96 04 # description: This is a daemon for managing locks \ # on virtual machine disk images # # processname: virtlockd -# pidfile: @localstatedir@/run/libvirt/virtlockd.pid +# pidfile: @localstatedir@/run/virtlockd.pid # # Source function library. @@ -28,7 +29,7 @@ SERVICE=virtlockd PROCESS=virtlockd -PIDFILE=@localstatedir@/run/libvirt/lockd/$SERVICE.pid +PIDFILE=@localstatedir@/run/$SERVICE.pid VIRTLOCKD_ARGS= @@ -64,7 +65,7 @@ restart() { reload() { echo -n $"Reloading $SERVICE configuration: " - killproc -p $PIDFILE $PROCESS -HUP + killproc -p $PIDFILE $PROCESS -USR1 RETVAL=$? echo return $RETVAL -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
- Use SIGUSR1, not SIGHUP, on reload. At present, virtlockd only responds to the former.
- Fix PID file for virtlockd.
- Do not start virtlockd in any runlevels by default. It needs to be explicitly selected in libvirt's qemu.conf anyway, so there is no need to have it running on all systems regardless.
- Fix chkconfig priorities to ensure virtlockd is started before libvirtd is started, and stopped after libvirtd is stopped.
- Add "Should-Start: virtlockd" to the libvirtd initscript's LSB header, for the same reason.
- Add "Default-Stop" to both libvirtd and virtlockd initscripts. LSB does not guarantee that this defaults to the inverse of "Default-Start".
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- daemon/libvirtd.init.in | 2 ++ src/locking/virtlockd.init.in | 11 ++++++----- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in index f66ddad..ed42195 100644 --- a/daemon/libvirtd.init.in +++ b/daemon/libvirtd.init.in @@ -9,9 +9,11 @@ # Should-Start: $named # Should-Start: xend # Should-Start: avahi-daemon +# Should-Start: virtlockd # Required-Stop: $network messagebus # Should-Stop: $named # Default-Start: 3 4 5 +# Default-Stop: 0 1 2 6 # Short-Description: daemon for libvirt virtualization API # Description: This is a daemon for managing guest instances # and libvirt virtual networks diff --git a/src/locking/virtlockd.init.in b/src/locking/virtlockd.init.in index 1adea07..0913727 100644 --- a/src/locking/virtlockd.init.in +++ b/src/locking/virtlockd.init.in @@ -5,7 +5,8 @@ # ### BEGIN INIT INFO # Provides: virtlockd -# Default-Start: 3 4 5 +# Default-Start:
Spurious space.
+# Default-Stop: 0 1 2 3 4 5 6 # Short-Description: virtual machine lock manager # Description: This is a daemon for managing locks # on virtual machine disk images @@ -15,12 +16,12 @@ # # virtlockd: virtual machine lock manager # -# chkconfig: 345 97 03 +# chkconfig: - 96 04 # description: This is a daemon for managing locks \ # on virtual machine disk images # # processname: virtlockd -# pidfile: @localstatedir@/run/libvirt/virtlockd.pid +# pidfile: @localstatedir@/run/virtlockd.pid #
# Source function library. @@ -28,7 +29,7 @@
SERVICE=virtlockd PROCESS=virtlockd -PIDFILE=@localstatedir@/run/libvirt/lockd/$SERVICE.pid +PIDFILE=@localstatedir@/run/$SERVICE.pid
VIRTLOCKD_ARGS=
@@ -64,7 +65,7 @@ restart() { reload() { echo -n $"Reloading $SERVICE configuration: "
- killproc -p $PIDFILE $PROCESS -HUP + killproc -p $PIDFILE $PROCESS -USR1 RETVAL=$? echo return $RETVAL
Fixed & pushed. Michal
SIGHUP is commonly used to instruct a daemon to reload its config. For now we should handle it in virtlockd just like SIGUSR1, rather than having it kill the process. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 35ccb4e..52d953a 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -590,6 +590,8 @@ virLockDaemonSetupSignals(virNetServerPtr srv) return -1; if (virNetServerAddSignalHandler(srv, SIGUSR1, virLockDaemonExecRestartHandler, NULL) < 0) return -1; + if (virNetServerAddSignalHandler(srv, SIGHUP, virLockDaemonExecRestartHandler, NULL) < 0) + return -1; return 0; } -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
SIGHUP is commonly used to instruct a daemon to reload its config. For now we should handle it in virtlockd just like SIGUSR1, rather than having it kill the process.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 35ccb4e..52d953a 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -590,6 +590,8 @@ virLockDaemonSetupSignals(virNetServerPtr srv) return -1; if (virNetServerAddSignalHandler(srv, SIGUSR1, virLockDaemonExecRestartHandler, NULL) < 0) return -1; + if (virNetServerAddSignalHandler(srv, SIGHUP, virLockDaemonExecRestartHandler, NULL) < 0) + return -1; return 0; }
Mm, okay. But if we go down this path I think we should adjust the man-page too: diff --git a/src/locking/virtlockd.pod.in b/src/locking/virtlockd.pod.in index 99612aa..022d67f 100644 --- a/src/locking/virtlockd.pod.in +++ b/src/locking/virtlockd.pod.in @@ -54,9 +54,9 @@ Display version information then exit. =head1 SIGNALS -On receipt of B<SIGUSR1> virtlockd will re-exec() its binary, while -maintaining all current locks and clients. This allows for live -upgrades of the virtlockd service. +On receipt of B<SIGUSR1> or B<SIGHUP> virtlockd will re-exec() its +binary, while maintaining all current locks and clients. This allows +for live upgrades of the virtlockd service. =head1 FILES Squashed in and pushed. Michal
On Mon, Dec 09, 2013 at 05:23:27PM +1100, Michael Chapman wrote:
SIGHUP is commonly used to instruct a daemon to reload its config. For now we should handle it in virtlockd just like SIGUSR1, rather than having it kill the process.
I don't think we should make SIGHUP do a re-exec - we should keep this signal available for the future when we may well want to support reload of the config without re-exec'ing at the same time. 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 12/10/2013 04:15 AM, Daniel P. Berrange wrote:
On Mon, Dec 09, 2013 at 05:23:27PM +1100, Michael Chapman wrote:
SIGHUP is commonly used to instruct a daemon to reload its config. For now we should handle it in virtlockd just like SIGUSR1, rather than having it kill the process.
I don't think we should make SIGHUP do a re-exec - we should keep this signal available for the future when we may well want to support reload of the config without re-exec'ing at the same time.
Fair point; should we go ahead and revert this patch, since it got pushed? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 10, 2013 at 06:28:06AM -0700, Eric Blake wrote:
On 12/10/2013 04:15 AM, Daniel P. Berrange wrote:
On Mon, Dec 09, 2013 at 05:23:27PM +1100, Michael Chapman wrote:
SIGHUP is commonly used to instruct a daemon to reload its config. For now we should handle it in virtlockd just like SIGUSR1, rather than having it kill the process.
I don't think we should make SIGHUP do a re-exec - we should keep this signal available for the future when we may well want to support reload of the config without re-exec'ing at the same time.
Fair point; should we go ahead and revert this patch, since it got pushed?
Yep, I think that would be best. 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 :|
Also use a distinct, valid exit status for daemon re-execution failure. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 52d953a..ae09ef8 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -77,6 +77,7 @@ enum { VIR_LOCK_DAEMON_ERR_NETWORK, VIR_LOCK_DAEMON_ERR_CONFIG, VIR_LOCK_DAEMON_ERR_HOOKS, + VIR_LOCK_DAEMON_ERR_REEXEC, VIR_LOCK_DAEMON_ERR_LAST }; @@ -91,7 +92,8 @@ VIR_ENUM_IMPL(virDaemonErr, VIR_LOCK_DAEMON_ERR_LAST, "Unable to drop privileges", "Unable to initialize network sockets", "Unable to load configuration file", - "Unable to look for hook scripts"); + "Unable to look for hook scripts", + "Unable to re-execute daemon"); static void * virLockDaemonClientNew(virNetServerClientPtr client, @@ -1203,18 +1205,14 @@ int main(int argc, char **argv) { case 'p': VIR_FREE(pid_file); - if (VIR_STRDUP_QUIET(pid_file, optarg) < 0) { - VIR_ERROR(_("Can't allocate memory")); - exit(EXIT_FAILURE); - } + if (VIR_STRDUP_QUIET(pid_file, optarg) < 0) + goto no_memory; break; case 'f': VIR_FREE(remote_config_file); - if (VIR_STRDUP_QUIET(remote_config_file, optarg) < 0) { - VIR_ERROR(_("Can't allocate memory")); - exit(EXIT_FAILURE); - } + if (VIR_STRDUP_QUIET(remote_config_file, optarg) < 0) + goto no_memory; break; case 'V': @@ -1298,10 +1296,8 @@ int main(int argc, char **argv) { /* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { - if (VIR_STRDUP_QUIET(run_dir, LOCALSTATEDIR "/run/libvirt") < 0) { - VIR_ERROR(_("Can't allocate memory")); - goto cleanup; - } + if (VIR_STRDUP_QUIET(run_dir, LOCALSTATEDIR "/run/libvirt") < 0) + goto no_memory; } else { if (!(run_dir = virGetUserRuntimeDirectory())) { VIR_ERROR(_("Can't determine user directory")); @@ -1395,7 +1391,7 @@ int main(int argc, char **argv) { if (execRestart && virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0) - ret = -1; + ret = VIR_LOCK_DAEMON_ERR_REEXEC; else ret = 0; @@ -1418,4 +1414,8 @@ cleanup: VIR_FREE(sock_file); VIR_FREE(run_dir); return ret; + +no_memory: + VIR_ERROR(_("Can't allocate memory")); + exit(EXIT_FAILURE); } -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
Also use a distinct, valid exit status for daemon re-execution failure.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
ACKed & pushed. Michal
virtlockd's argv[0] may not contain a full path to the binary, however it should contain something that can be looked up in the PATH. Use execvp() to do path lookup when re-executing ourselves. After re-execution, we must not attempt to daemonize again. It's not only unnecessary, but it also means we end up with the wrong PID and so we can't validate the state file. Instead, build a new argv for the new program that does not include --daemon. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 49 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index ae09ef8..32ca4d6 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1000,6 +1000,7 @@ cleanup: static int virLockDaemonPreExecRestart(virNetServerPtr srv, + const char *argv0, char **argv) { virJSONValuePtr child; @@ -1075,7 +1076,7 @@ virLockDaemonPreExecRestart(virNetServerPtr srv, goto cleanup; } - if (execv(argv[0], argv) < 0) { + if (execvp(argv0, argv) < 0) { virReportSystemError(errno, "%s", _("Unable to restart self")); goto cleanup; @@ -1389,11 +1390,47 @@ int main(int argc, char **argv) { virNetServerUpdateServices(lockDaemon->srv, true); virNetServerRun(lockDaemon->srv); - if (execRestart && - virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0) - ret = VIR_LOCK_DAEMON_ERR_REEXEC; - else - ret = 0; + ret = VIR_LOCK_DAEMON_ERR_NONE; + if (execRestart) { + char **restart_argv = NULL; + size_t count = 0; + + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], argv[0]) < 0) + goto no_memory; + if (verbose) { + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], "--verbose") < 0) + goto no_memory; + } + if (remote_config_file && !implicit_conf) { + if (VIR_EXPAND_N(restart_argv, count, 2) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 2], "--config") < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], remote_config_file) < 0) + goto no_memory; + } + if (pid_file) { + if (VIR_EXPAND_N(restart_argv, count, 2) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 2], "--pid-file") < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], pid_file) < 0) + goto no_memory; + } + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + + if (virLockDaemonPreExecRestart(lockDaemon->srv, argv[0], restart_argv) < 0) + ret = VIR_LOCK_DAEMON_ERR_REEXEC; + + while (count) + VIR_FREE(restart_argv[--count]); + VIR_FREE(restart_argv); + } cleanup: virObjectUnref(lockProgram); -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
virtlockd's argv[0] may not contain a full path to the binary, however it should contain something that can be looked up in the PATH. Use execvp() to do path lookup when re-executing ourselves.
After re-execution, we must not attempt to daemonize again. It's not only unnecessary, but it also means we end up with the wrong PID and so we can't validate the state file.
Instead, build a new argv for the new program that does not include --daemon.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- src/locking/lock_daemon.c | 49 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index ae09ef8..32ca4d6 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1000,6 +1000,7 @@ cleanup:
static int virLockDaemonPreExecRestart(virNetServerPtr srv, + const char *argv0, char **argv) { virJSONValuePtr child; @@ -1075,7 +1076,7 @@ virLockDaemonPreExecRestart(virNetServerPtr srv, goto cleanup; }
- if (execv(argv[0], argv) < 0) { + if (execvp(argv0, argv) < 0) { virReportSystemError(errno, "%s", _("Unable to restart self"));
While this chunk is okay,
goto cleanup; @@ -1389,11 +1390,47 @@ int main(int argc, char **argv) { virNetServerUpdateServices(lockDaemon->srv, true); virNetServerRun(lockDaemon->srv);
- if (execRestart && - virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0) - ret = VIR_LOCK_DAEMON_ERR_REEXEC; - else - ret = 0; + ret = VIR_LOCK_DAEMON_ERR_NONE; + if (execRestart) { + char **restart_argv = NULL; + size_t count = 0; + + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], argv[0]) < 0) + goto no_memory; + if (verbose) { + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], "--verbose") < 0) + goto no_memory; + } + if (remote_config_file && !implicit_conf) { + if (VIR_EXPAND_N(restart_argv, count, 2) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 2], "--config") < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], remote_config_file) < 0) + goto no_memory; + } + if (pid_file) { + if (VIR_EXPAND_N(restart_argv, count, 2) < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 2], "--pid-file") < 0) + goto no_memory; + if (VIR_STRDUP(restart_argv[count - 1], pid_file) < 0) + goto no_memory; + } + if (VIR_EXPAND_N(restart_argv, count, 1) < 0) + goto no_memory; + + if (virLockDaemonPreExecRestart(lockDaemon->srv, argv[0], restart_argv) < 0) + ret = VIR_LOCK_DAEMON_ERR_REEXEC; + + while (count) + VIR_FREE(restart_argv[--count]); + VIR_FREE(restart_argv); + }
cleanup: virObjectUnref(lockProgram);
This one isn't. It requires us to expand it whenever a new cmd line argument is invented. How about copying all arguments but "--daemon" or "-d"? (Just an explicit note - I'm not pushing this one now) Michal
On Mon, Dec 09, 2013 at 05:23:29PM +1100, Michael Chapman wrote:
virtlockd's argv[0] may not contain a full path to the binary, however it should contain something that can be looked up in the PATH. Use execvp() to do path lookup when re-executing ourselves.
After re-execution, we must not attempt to daemonize again. It's not only unnecessary, but it also means we end up with the wrong PID and so we can't validate the state file.
Instead, build a new argv for the new program that does not include --daemon.
It would seem to be simpler to instead change the main() method so that --daemon is ignored when re-exec'ing. 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 Tue, 10 Dec 2013, Daniel P. Berrange wrote:
It would seem to be simpler to instead change the main() method so that --daemon is ignored when re-exec'ing.
Yes, that does sound saner. I also noticed that virtlockd attempts to write out a re-exec state file under /var/ even when running unprivileged, when it probably should go under $XDG_RUNTIME_DIR. I'll roll in a fix for this as well. - Michael
- systemctl and the %systemd_* RPM macros can take multiple unit names in the one invocation. Make use of this to avoid repeated systemd daemon reloads. - virtlockd was only properly enabled and disabled when using systemd, but when systemd RPM macros were not available (e.g. on Fedora < 18). Make sure it's enabled when systemd RPM macros are present, or when using initscripts. - Always use "reload" on virtlockd, not "condrestart". This allows it to cleanly re-execute itself without losing running state. Ignore any error should the reload fail. - Move the reloading of virtlockd and libvirtd via their initscripts into the daemon package's %postun scriptlet. These services must be restarted after all of the libvirt-daemon-driver-* packages have been upgraded during the same RPM transaction. - Add a %triggerpostun executed only when upgrading an older libvirt-daemon. As an older package would only reload libvirtd during %post, and the newer package would only reload libvirtd during %postun, such an upgrade would not reload libvirtd at all without the trigger. Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- libvirt.spec.in | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index bd16eb3..9b34f3f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1591,12 +1591,11 @@ done %if %{with_systemd} %if %{with_systemd_macros} - %systemd_post libvirtd.service + %systemd_post virtlockd.socket libvirtd.service %else if [ $1 -eq 1 ] ; then # Initial installation - /bin/systemctl enable virtlockd.socket >/dev/null 2>&1 || : - /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : + /bin/systemctl enable virtlockd.socket libvirtd.service >/dev/null 2>&1 || : fi %endif %else @@ -1611,46 +1610,50 @@ fi %endif /sbin/chkconfig --add libvirtd -if [ "$1" -ge "1" ]; then - /sbin/service libvirtd condrestart > /dev/null 2>&1 -fi +/sbin/chkconfig --add virtlockd %endif %preun daemon %if %{with_systemd} %if %{with_systemd_macros} - %systemd_preun libvirtd.service + %systemd_preun libvirtd.service virtlockd.socket virtlockd.service %else if [ $1 -eq 0 ] ; then # Package removal, not upgrade - /bin/systemctl --no-reload disable virtlockd.socket > /dev/null 2>&1 || : - /bin/systemctl --no-reload disable libvirtd.service > /dev/null 2>&1 || : - /bin/systemctl stop libvirtd.service > /dev/null 2>&1 || : - /bin/systemctl stop virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl --no-reload disable libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : + /bin/systemctl stop libvirtd.service virtlockd.socket virtlockd.service > /dev/null 2>&1 || : fi %endif %else if [ $1 = 0 ]; then /sbin/service libvirtd stop 1>/dev/null 2>&1 /sbin/chkconfig --del libvirtd + /sbin/service virtlockd stop 1>/dev/null 2>&1 + /sbin/chkconfig --del virtlockd fi %endif %postun daemon %if %{with_systemd} - %if %{with_systemd_macros} - %systemd_postun_with_restart libvirtd.service - %else /bin/systemctl daemon-reload >/dev/null 2>&1 || : if [ $1 -ge 1 ] ; then - # Package upgrade, not uninstall - /bin/systemctl status virtlockd.service >/dev/null 2>&1 - if [ $? = 1 ] ; then - /bin/systemctl kill --signal=USR1 virtlockd.service >/dev/null 2>&1 || : - fi + /bin/systemctl reload-or-try-restart virtlockd.service >/dev/null 2>&1 || : /bin/systemctl try-restart libvirtd.service >/dev/null 2>&1 || : fi - %endif + %else +if [ $1 -ge 1 ]; then + /sbin/service virtlockd reload > /dev/null 2>&1 || : + /sbin/service libvirtd condrestart > /dev/null 2>&1 +fi + %endif + + %if %{with_systemd} + %else +%triggerpostun daemon -- libvirt-daemon < 1.2.1 +if [ "$1" -ge "1" ]; then + /sbin/service virtlockd reload > /dev/null 2>&1 || : + /sbin/service libvirtd condrestart > /dev/null 2>&1 +fi %endif %if %{with_network} -- 1.8.4.2
On 09.12.2013 07:23, Michael Chapman wrote:
- systemctl and the %systemd_* RPM macros can take multiple unit names in the one invocation. Make use of this to avoid repeated systemd daemon reloads.
- virtlockd was only properly enabled and disabled when using systemd, but when systemd RPM macros were not available (e.g. on Fedora < 18). Make sure it's enabled when systemd RPM macros are present, or when using initscripts.
- Always use "reload" on virtlockd, not "condrestart". This allows it to cleanly re-execute itself without losing running state. Ignore any error should the reload fail.
- Move the reloading of virtlockd and libvirtd via their initscripts into the daemon package's %postun scriptlet. These services must be restarted after all of the libvirt-daemon-driver-* packages have been upgraded during the same RPM transaction.
- Add a %triggerpostun executed only when upgrading an older libvirt-daemon. As an older package would only reload libvirtd during %post, and the newer package would only reload libvirtd during %postun, such an upgrade would not reload libvirtd at all without the trigger.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- libvirt.spec.in | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-)
ACKed & pushed. Michal
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michael Chapman -
Michal Privoznik