12:23 a.m.
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
12:23 a.m.
New subject: [libvirt] [PATCH 1/6] virtlockd: improve systemd units
- 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(a)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
5:06 a.m.
New subject: [libvirt] [PATCH 1/6] virtlockd: improve systemd units
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(a)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
12:23 a.m.
New subject: [libvirt] [PATCH 2/6] virtlockd: improve initscripts
- 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(a)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(a)/run/libvirt/virtlockd.pid
+# pidfile: @localstatedir(a)/run/virtlockd.pid
#
# Source function library.
@@ -28,7 +29,7 @@
SERVICE=virtlockd
PROCESS=virtlockd
-PIDFILE=@localstatedir(a)/run/libvirt/lockd/$SERVICE.pid
+PIDFILE=@localstatedir(a)/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
5:06 a.m.
New subject: [libvirt] [PATCH 2/6] virtlockd: improve initscripts
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(a)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(a)/run/libvirt/virtlockd.pid
+# pidfile: @localstatedir(a)/run/virtlockd.pid
#
# Source function library.
@@ -28,7 +29,7 @@
SERVICE=virtlockd
PROCESS=virtlockd
-PIDFILE=@localstatedir(a)/run/libvirt/lockd/$SERVICE.pid
+PIDFILE=@localstatedir(a)/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
12:23 a.m.
New subject: [libvirt] [PATCH 3/6] virtlockd: treat SIGHUP like SIGUSR1
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(a)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
5:06 a.m.
New subject: [libvirt] [PATCH 3/6] virtlockd: treat SIGHUP like SIGUSR1
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(a)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
5:15 a.m.
New subject: [libvirt] [PATCH 3/6] virtlockd: treat SIGHUP like SIGUSR1
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 :|
7:28 a.m.
New subject: [libvirt] [PATCH 3/6] virtlockd: treat SIGHUP like SIGUSR1
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
7:33 a.m.
New subject: [libvirt] [PATCH 3/6] virtlockd: treat SIGHUP like SIGUSR1
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 :|
12:23 a.m.
New subject: [libvirt] [PATCH 4/6] virtlockd: use common exit path when out-of-memory
Also use a distinct, valid exit status for daemon re-execution failure.
Signed-off-by: Michael Chapman <mike(a)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
5:06 a.m.
New subject: [libvirt] [PATCH 4/6] virtlockd: use common exit path when out-of-memory
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(a)very.puzzling.org>
---
src/locking/lock_daemon.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
ACKed & pushed.
Michal
12:23 a.m.
New subject: [libvirt] [PATCH 5/6] virtlockd: make re-exec more robust
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(a)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
5:06 a.m.
New subject: [libvirt] [PATCH 5/6] virtlockd: make re-exec more robust
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(a)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
5:14 a.m.
New subject: [libvirt] [PATCH 5/6] virtlockd: make re-exec more robust
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 :|
2:06 a.m.
New subject: [libvirt] [PATCH 5/6] virtlockd: make re-exec more robust
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
12:23 a.m.
New subject: [libvirt] [PATCH 6/6] spec: clean up libvirtd and virtlockd service mgmt
- 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(a)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
7:23 a.m.
New subject: [libvirt] [PATCH 6/6] spec: clean up libvirtd and virtlockd service mgmt
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(a)very.puzzling.org>
---
libvirt.spec.in | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
ACKed & pushed.
Michal
3971
days inactive
3973
days old
17 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Michael Chapman -
Michal Privoznik