Re: [libvirt] [PATCH v2] virtlockd: make re-exec more robust
On 11.12.2013 09:07, Michael Chapman wrote:
> - Use $XDG_RUNTIME_DIR for re-exec state file when running unprivileged.
>
> - 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 on re-exec.
>
> - As per list discussion [1], ignore --daemon on re-exec.
>
> [1] https://www.redhat.com/archives/libvir-list/2013-December/msg00514.html
>
> Signed-off-by: Michael Chapman
> ---
> src/locking/lock_daemon.c | 128
> ++
> 1 file changed, 94 insertions(+), 34 deletions(-)
>
> diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
> index a6be43c..b405e3a 100644
> --- a/src/locking/lock_daemon.c
> +++ b/src/locking/lock_daemon.c
> @@ -925,7 +925,41 @@ error:
> }
>
>
> -#define VIR_LOCK_DAEMON_RESTART_EXEC_FILE LOCALSTATEDIR
> "/run/virtlockd-restart-exec.json"
> +static int
> +virLockDaemonExecRestartStatePath(bool privileged,
> + char **state_file)
> +{
> +if (privileged) {
> +if (VIR_STRDUP(*state_file, LOCALSTATEDIR
> "/run/virtlockd-restart-exec.json") < 0)
> +goto error;
> +} else {
> +char *rundir = NULL;
> +mode_t old_umask;
> +
> +if (!(rundir = virGetUserRuntimeDirectory()))
> +goto error;
> +
> +old_umask = umask(077);
> +if (virFileMakePath(rundir) < 0) {
> +umask(old_umask);
> +goto error;
If we fail, when the control continues at the 'error' label ..
> +}
> +umask(old_umask);
> +
> +if (virAsprintf(state_file, "%s/virtlockd-restart-exec.json",
> rundir) < 0) {
> +VIR_FREE(rundir);
> +goto error;
> +}
> +
> +VIR_FREE(rundir);
> +}
> +
> +return 0;
> +
> +error:
.. here, where the @rundir is leaked.
> +return -1;
> +}
> +
>
> static char *
> virLockDaemonGetExecRestartMagic(void)
> @@ -938,7 +972,10 @@ virLockDaemonGetExecRestartMagic(void)
>
>
> static int
> -virLockDaemonPostExecRestart(bool privileged)
> +virLockDaemonPostExecRestart(const char *state_file,
> + const char *pid_file,
> + int *pid_file_fd,
> + bool privileged)
> {
> const char *gotmagic;
> char *wantmagic = NULL;
> @@ -948,14 +985,14 @@ virLockDaemonPostExecRestart(bool privileged)
>
> VIR_DEBUG("Running post-restart exec");
>
> -if (!virFileExists(VIR_LOCK_DAEMON_RESTART_EXEC_FILE)) {
> -VIR_DEBUG("No restart file %s present",
> - VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> +if (!virFileExists(state_file)) {
> +VIR_DEBUG("No restart state file %s present",
> + state_file);
> ret = 0;
> goto cleanup;
> }
>
> -if (virFileReadAll(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
> +if (virFileReadAll(state_file,
> 1024 * 1024 * 10, /* 10 MB */
> &state) < 0)
> goto cleanup;
> @@ -982,13 +1019,18 @@ virLockDaemonPostExecRestart(bool privileged)
> goto cleanup;
> }
>
> +/* Re-claim PID file now as we will not be daemonizing */
> +if (pid_file &&
> +(*pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0)
> +goto cleanup;
> +
> if (!(lockDaemon = virLockDaemonNewPostExecRestart(object, privileged)))
> goto cleanup;
>
> ret = 1;
>
> cleanup:
> -unlink(VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> +unlink(state_file);
> VIR_FREE(wantmagic);
> VIR_FREE(state);
> virJSONValueFree(object);
> @@ -997,7 +1039,8 @@ cleanup:
>
>
> static int
> -virLockDaemonPreExecRestart(virNetServerPtr srv,
> +virLockDaemonPreExecRestart(const char *state_file,
> +virNetServerPtr srv,
> char **argv)
> {
> virJSONValuePtr child;
> @@ -1065,15 +1108,15 @@ virLockDaemonPreExecRestart(virNetServerPtr srv,
>
> VIR_DEBUG("Saving state %s", state);
>
> -if (virFileWriteStr(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
> +if (virFileWriteStr(state_file,
> state, 0700) < 0) {
> virReportSystemError(errno,
> _("Unable to save state file %s"),
> - VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
> + state_file);
> goto cleanup;
> }
>
> -if (execv(argv[0], argv) < 0) {
> +if (execvp(argv[0], argv) < 0) {
> virReportSystemError(errno, "%s",
> _("Unable to restart self"));
> goto cleanup;
> @@ -1153,6 +1196,7 @@ int main(int argc, char **argv) {
> char *pid_file = NULL;
> int pid_file_fd = -1;
> char *sock_file = NULL;
> +char *state_file = NULL;
> bool implicit_conf = false;
> mode_t old_umask;
>
Re: [libvirt] [PATCH v2] virtlockd: make re-exec more robust
On Wed, 11 Dec 2013, Michael Chapman wrote: - Use $XDG_RUNTIME_DIR for re-exec state file when running unprivileged. - 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 on re-exec. - As per list discussion [1], ignore --daemon on re-exec. [1] https://www.redhat.com/archives/libvir-list/2013-December/msg00514.html Signed-off-by: Michael Chapman --- src/locking/lock_daemon.c | 128 ++ 1 file changed, 94 insertions(+), 34 deletions(-) Any comments on this patch? - Michael -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] virtlockd: make re-exec more robust
- Use $XDG_RUNTIME_DIR for re-exec state file when running unprivileged.
- 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 on re-exec.
- As per list discussion [1], ignore --daemon on re-exec.
[1] https://www.redhat.com/archives/libvir-list/2013-December/msg00514.html
Signed-off-by: Michael Chapman
---
src/locking/lock_daemon.c | 128 ++
1 file changed, 94 insertions(+), 34 deletions(-)
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index a6be43c..b405e3a 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -925,7 +925,41 @@ error:
}
-#define VIR_LOCK_DAEMON_RESTART_EXEC_FILE LOCALSTATEDIR
"/run/virtlockd-restart-exec.json"
+static int
+virLockDaemonExecRestartStatePath(bool privileged,
+ char **state_file)
+{
+if (privileged) {
+if (VIR_STRDUP(*state_file, LOCALSTATEDIR
"/run/virtlockd-restart-exec.json") < 0)
+goto error;
+} else {
+char *rundir = NULL;
+mode_t old_umask;
+
+if (!(rundir = virGetUserRuntimeDirectory()))
+goto error;
+
+old_umask = umask(077);
+if (virFileMakePath(rundir) < 0) {
+umask(old_umask);
+goto error;
+}
+umask(old_umask);
+
+if (virAsprintf(state_file, "%s/virtlockd-restart-exec.json", rundir)
< 0) {
+VIR_FREE(rundir);
+goto error;
+}
+
+VIR_FREE(rundir);
+}
+
+return 0;
+
+error:
+return -1;
+}
+
static char *
virLockDaemonGetExecRestartMagic(void)
@@ -938,7 +972,10 @@ virLockDaemonGetExecRestartMagic(void)
static int
-virLockDaemonPostExecRestart(bool privileged)
+virLockDaemonPostExecRestart(const char *state_file,
+ const char *pid_file,
+ int *pid_file_fd,
+ bool privileged)
{
const char *gotmagic;
char *wantmagic = NULL;
@@ -948,14 +985,14 @@ virLockDaemonPostExecRestart(bool privileged)
VIR_DEBUG("Running post-restart exec");
-if (!virFileExists(VIR_LOCK_DAEMON_RESTART_EXEC_FILE)) {
-VIR_DEBUG("No restart file %s present",
- VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
+if (!virFileExists(state_file)) {
+VIR_DEBUG("No restart state file %s present",
+ state_file);
ret = 0;
goto cleanup;
}
-if (virFileReadAll(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
+if (virFileReadAll(state_file,
1024 * 1024 * 10, /* 10 MB */
&state) < 0)
goto cleanup;
@@ -982,13 +1019,18 @@ virLockDaemonPostExecRestart(bool privileged)
goto cleanup;
}
+/* Re-claim PID file now as we will not be daemonizing */
+if (pid_file &&
+(*pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0)
+goto cleanup;
+
if (!(lockDaemon = virLockDaemonNewPostExecRestart(object, privileged)))
goto cleanup;
ret = 1;
cleanup:
-unlink(VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
+unlink(state_file);
VIR_FREE(wantmagic);
VIR_FREE(state);
virJSONValueFree(object);
@@ -997,7 +1039,8 @@ cleanup:
static int
-virLockDaemonPreExecRestart(virNetServerPtr srv,
+virLockDaemonPreExecRestart(const char *state_file,
+virNetServerPtr srv,
char **argv)
{
virJSONValuePtr child;
@@ -1065,15 +1108,15 @@ virLockDaemonPreExecRestart(virNetServerPtr srv,
VIR_DEBUG("Saving state %s", state);
-if (virFileWriteStr(VIR_LOCK_DAEMON_RESTART_EXEC_FILE,
+if (virFileWriteStr(state_file,
state, 0700) < 0) {
virReportSystemError(errno,
_("Unable to save state file %s"),
- VIR_LOCK_DAEMON_RESTART_EXEC_FILE);
+ state_file);
goto cleanup;
}
-if (execv(argv[0], argv) < 0) {
+if (execvp(argv[0], argv) < 0) {
virReportSystemError(errno, "%s",
_("Unable to restart self"));
goto cleanup;
@@ -1153,6 +1196,7 @@ int main(int argc, char **argv) {
char *pid_file = NULL;
int pid_file_fd = -1;
char *sock_file = NULL;
+char *state_file = NULL;
bool implicit_conf = false;
mode_t old_umask;
bool privileged = false;
@@ -1276,21 +1320,13 @@ int main(int argc, char **argv) {
VIR_DEBUG("Decided on socket paths '%s'",
sock_file);
-if (godaemon) {
-char ebuf[1024];
-
-if (chdir("/") < 0) {
-VIR_ERROR(_("cannot change to root directory: %s"),
- virStrerror(errno, ebuf, sizeof(ebuf)));
-goto cleanup;
-}
-
-if ((statuswrite = virLockDaemonFor
