virCommand: Actually acquire pidfile instead of just writing it

Our virCommand module allows us to set a pidfile for commands we
want to spawn. The caller constructs the string of pidfile path
and then uses virCommandSetPidFile() to tell the module to write
the pidfile once the command is ran. This usually works, but has
two flaws:

1) the child process does not hold the pidfile open & locked.
Therefore, the caller (or anybody else) can't use our fancy
virPidFileForceCleanupPath() function to kill the command
afterwards. Also, for everybody else on the system it's
needlessly harder to check if the pid from the pidfile is still
alive or not.

2) if the caller ever makes a mistake and passes the same pidfile
path for two different commands, the start of the second command
will overwrite the pidfile even though the first command might
still be running.

NOTE that this temporarily renders some command spawning
unusable, specifically those code patterns where both
virCommandSetPidFile() is used together with instructing spawned
command to acquire pidfile itself. Fortunately, there is only one
occurrence of such pattern and it is in
qemuProcessStartManagedPRDaemon(). This is fixed in next commit.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 8a9061e70f2b8533f6bbf93887ea1ea50f5c7af4..823d89cc718121cd6c29c44ac1a935231bc11c04 100644 (file)
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -226,7 +226,9 @@ virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid");
 
     <p>
       This PID file is guaranteed to be written before
-      the intermediate process exits.
+      the intermediate process exits. Moreover, the daemonized
+      process will inherit the FD of the opened and locked PID
+      file.
     </p>
 
     <h3><a id="privs">Reducing command privileges</a></h3>

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 9ffa0cf49b6348519107d247f3f64f91b467db2a..77078d09fbb4ee8aa101259a7a80c0309d9d41c6 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -612,6 +612,7 @@ virExec(virCommandPtr cmd)
     int null = -1;
     int pipeout[2] = {-1, -1};
     int pipeerr[2] = {-1, -1};
+    int pipesync[2] = {-1, -1};
     int childin = cmd->infd;
     int childout = -1;
     int childerr = -1;
@@ -745,9 +746,15 @@ virExec(virCommandPtr cmd)
     /* Initialize full logging for a while */
     virLogSetFromEnv();
 
+    if (cmd->pidfile &&
+        virPipe(pipesync) < 0)
+        goto fork_error;
+
     /* Daemonize as late as possible, so the parent process can detect
      * the above errors with wait* */
     if (cmd->flags & VIR_EXEC_DAEMON) {
+        char c;
+
         if (setsid() < 0) {
             virReportSystemError(errno,
                                  "%s", _("cannot become session leader"));
@@ -768,12 +775,13 @@ virExec(virCommandPtr cmd)
         }
 
         if (pid > 0) {
-            if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid) < 0)) {
-                if (virProcessKillPainfully(pid, true) >= 0)
-                    virReportSystemError(errno,
-                                         _("could not write pidfile %s for %d"),
-                                         cmd->pidfile, pid);
-                goto fork_error;
+            /* The parent expect us to have written the pid file before
+             * exiting. Wait here for the child to write it and signal us. */
+            if (cmd->pidfile &&
+                saferead(pipesync[0], &c, sizeof(c)) != sizeof(c)) {
+                virReportSystemError(errno, "%s",
+                                     _("Unable to wait for child process"));
+                _exit(EXIT_FAILURE);
             }
             _exit(EXIT_SUCCESS);
         }
@@ -788,6 +796,37 @@ virExec(virCommandPtr cmd)
     if (cmd->setMaxCore &&
         virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
         goto fork_error;
+    if (cmd->pidfile) {
+        VIR_AUTOCLOSE pidfilefd = -1;
+        int newpidfilefd = -1;
+        char c;
+
+        pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, getpid());
+        if (pidfilefd < 0)
+            goto fork_error;
+        if (virSetInherit(pidfilefd, true) < 0) {
+            virReportSystemError(errno, "%s",
+                                 _("Cannot disable close-on-exec flag"));
+            goto fork_error;
+        }
+
+        c = '1';
+        if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) {
+            virReportSystemError(errno, "%s", _("Unable to notify child process"));
+            goto fork_error;
+        }
+        VIR_FORCE_CLOSE(pipesync[0]);
+        VIR_FORCE_CLOSE(pipesync[1]);
+
+        /* This is here only to move the pidfilefd
+         * to the lowest possible number. */
+        if ((newpidfilefd = dup(pidfilefd)) < 0) {
+            virReportSystemError(errno, "%s", _("Unable to dup FD"));
+            goto fork_error;
+        }
+
+        /* newpidfilefd is intentionally leaked. */
+    }
 
     if (cmd->hook) {
         VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque);
@@ -1080,8 +1119,9 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
  * @cmd: the command to modify
  * @pidfile: filename to use
  *
- * Save the child PID in a pidfile.  The pidfile will be populated
- * before the exec of the child.
+ * Save the child PID in a pidfile. The pidfile will be populated before the
+ * exec of the child and the child will inherit opened and locked FD to the
+ * pidfile.
  */
 void
 virCommandSetPidFile(virCommandPtr cmd, const char *pidfile)

diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
index f170be204eaa8553afb66779f686629f680950fe..5820f28307051bf3300115f3b343d29a5d05e729 100644 (file)
--- a/tests/commanddata/test4.log
+++ b/tests/commanddata/test4.log
@@ -9,6 +9,7 @@ ENV:USER=test
 FD:0
 FD:1
 FD:2
+FD:3
 DAEMON:yes
 CWD:/
 UMASK:0022