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 <mike@xxxxxxxxxxxxxxxxx> > --- > 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; > 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 = virLockDaemonForkIntoBackground(argv[0])) < 0) { > - VIR_ERROR(_("Failed to fork as daemon: %s"), > - virStrerror(errno, ebuf, sizeof(ebuf))); > - goto cleanup; > - } > + if (virLockDaemonExecRestartStatePath(privileged, > + &state_file) < 0) { > + VIR_ERROR(_("Can't determine restart state file path")); > + exit(EXIT_FAILURE); > } > + VIR_DEBUG("Decided on restart state file path '%s'", > + state_file); > > /* Ensure the rundir exists (on tmpfs on some systems) */ > if (privileged) { > @@ -1317,20 +1353,41 @@ int main(int argc, char **argv) { > } > umask(old_umask); > > - /* If we have a pidfile set, claim it now, exiting if already taken */ > - if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) { > - ret = VIR_LOCK_DAEMON_ERR_PIDFILE; > - goto cleanup; > - } > - > - if ((rv = virLockDaemonPostExecRestart(privileged)) < 0) { > + if ((rv = virLockDaemonPostExecRestart(state_file, > + pid_file, > + &pid_file_fd, > + privileged)) < 0) { > ret = VIR_LOCK_DAEMON_ERR_INIT; > goto cleanup; > } > > /* rv == 1, means we setup everything from saved state, > - * so we only setup stuff from scratch if rv == 0 */ > + * so only (possibly) daemonize and setup stuff from > + * scratch if rv == 0 > + */ > if (rv == 0) { > + 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 = virLockDaemonForkIntoBackground(argv[0])) < 0) { > + VIR_ERROR(_("Failed to fork as daemon: %s"), > + virStrerror(errno, ebuf, sizeof(ebuf))); > + goto cleanup; > + } > + } > + > + /* If we have a pidfile set, claim it now, exiting if already taken */ > + if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) { > + ret = VIR_LOCK_DAEMON_ERR_PIDFILE; > + goto cleanup; > + } > + > if (!(lockDaemon = virLockDaemonNew(config, privileged))) { > ret = VIR_LOCK_DAEMON_ERR_INIT; > goto cleanup; > @@ -1388,7 +1445,9 @@ int main(int argc, char **argv) { > virNetServerRun(lockDaemon->srv); > > if (execRestart && > - virLockDaemonPreExecRestart(lockDaemon->srv, argv) < 0) > + virLockDaemonPreExecRestart(state_file, > + lockDaemon->srv, > + argv) < 0) > ret = VIR_LOCK_DAEMON_ERR_REEXEC; > else > ret = 0; > @@ -1410,6 +1469,7 @@ cleanup: > virPidFileReleasePath(pid_file, pid_file_fd); > VIR_FREE(pid_file); > VIR_FREE(sock_file); > + VIR_FREE(state_file); > VIR_FREE(run_dir); > return ret; > > Other than that, the patch looks good to me. I'm squashing this in prior to push: diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index adb1129..e047751 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -940,6 +940,7 @@ virLockDaemonExecRestartStatePath(bool privileged, old_umask = umask(077); if (virFileMakePath(rundir) < 0) { umask(old_umask); + VIR_FREE(rundir); goto error; } umask(old_umask); ACK and sorry for late review. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list