42/118: Refactoring: Move all fork handling into a higher-order function

[Ludovic Courtès]

civodul pushed a commit to branch nix
in repository guix.

commit 8e9140cfdef9dbd1eb61e4c75c91d452ab5e4a74
Author: Eelco Dolstra <address@hidden>
Date:   Thu Jul 10 16:50:51 2014 +0200

    Refactoring: Move all fork handling into a higher-order function
    
    C++11 lambdas ftw.
---
 src/download-via-ssh/download-via-ssh.cc |   32 ++-------
 src/libstore/build.cc                    |   73 +++++++--------------
 src/libstore/local-store.cc              |   35 +++-------
 src/libutil/util.cc                      |  107 +++++++++++++++---------------
 src/libutil/util.hh                      |    7 ++
 src/nix-daemon/nix-daemon.cc             |   51 +++++---------
 src/nix-store/nix-store.cc               |   29 ++------
 7 files changed, 128 insertions(+), 206 deletions(-)

diff --git a/src/download-via-ssh/download-via-ssh.cc 
b/src/download-via-ssh/download-via-ssh.cc
index 6cbcd98..6834634 100644
--- a/src/download-via-ssh/download-via-ssh.cc
+++ b/src/download-via-ssh/download-via-ssh.cc
@@ -24,30 +24,14 @@ static std::pair<FdSink, FdSource> connect(const string & 
conn)
     Pipe to, from;
     to.create();
     from.create();
-    pid_t child = fork();
-    switch (child) {
-        case -1:
-            throw SysError("unable to fork");
-        case 0:
-            try {
-                restoreAffinity();
-                if (dup2(to.readSide, STDIN_FILENO) == -1)
-                    throw SysError("dupping stdin");
-                if (dup2(from.writeSide, STDOUT_FILENO) == -1)
-                    throw SysError("dupping stdout");
-                execlp("ssh"
-                      , "ssh"
-                      , "-x"
-                      , "-T"
-                      , conn.c_str()
-                      , "nix-store --serve"
-                      , NULL);
-                throw SysError("executing ssh");
-            } catch (std::exception & e) {
-                std::cerr << "error: " << e.what() << std::endl;
-            }
-            _exit(1);
-    }
+    startProcess([&]() {
+        if (dup2(to.readSide, STDIN_FILENO) == -1)
+            throw SysError("dupping stdin");
+        if (dup2(from.writeSide, STDOUT_FILENO) == -1)
+            throw SysError("dupping stdout");
+        execlp("ssh", "ssh", "-x", "-T", conn.c_str(), "nix-store --serve", 
NULL);
+        throw SysError("executing ssh");
+    });
     // If child exits unexpectedly, we'll EPIPE or EOF early.
     // If we exit unexpectedly, child will EPIPE or EOF early.
     // So no need to keep track of it.
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 70a3eff..d318450 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -602,42 +602,29 @@ HookInstance::HookInstance()
     builderOut.create();
 
     /* Fork the hook. */
-    pid = fork();
-    switch (pid) {
+    pid = startProcess([&]() {
 
-    case -1:
-        throw SysError("unable to fork");
+        commonChildInit(fromHook);
 
-    case 0:
-        try { /* child */
+        if (chdir("/") == -1) throw SysError("changing into `/");
 
-            commonChildInit(fromHook);
+        /* Dup the communication pipes. */
+        if (dup2(toHook.readSide, STDIN_FILENO) == -1)
+            throw SysError("dupping to-hook read side");
 
-            if (chdir("/") == -1) throw SysError("changing into `/");
+        /* Use fd 4 for the builder's stdout/stderr. */
+        if (dup2(builderOut.writeSide, 4) == -1)
+            throw SysError("dupping builder's stdout/stderr");
 
-            /* Dup the communication pipes. */
-            if (dup2(toHook.readSide, STDIN_FILENO) == -1)
-                throw SysError("dupping to-hook read side");
+        execl(buildHook.c_str(), buildHook.c_str(), 
settings.thisSystem.c_str(),
+            (format("%1%") % settings.maxSilentTime).str().c_str(),
+            (format("%1%") % settings.printBuildTrace).str().c_str(),
+            (format("%1%") % settings.buildTimeout).str().c_str(),
+            NULL);
 
-            /* Use fd 4 for the builder's stdout/stderr. */
-            if (dup2(builderOut.writeSide, 4) == -1)
-                throw SysError("dupping builder's stdout/stderr");
+        throw SysError(format("executing `%1%'") % buildHook);
+    });
 
-            execl(buildHook.c_str(), buildHook.c_str(), 
settings.thisSystem.c_str(),
-                (format("%1%") % settings.maxSilentTime).str().c_str(),
-                (format("%1%") % settings.printBuildTrace).str().c_str(),
-                (format("%1%") % settings.buildTimeout).str().c_str(),
-                NULL);
-
-            throw SysError(format("executing `%1%'") % buildHook);
-
-        } catch (std::exception & e) {
-            writeToStderr("build hook error: " + string(e.what()) + "\n");
-        }
-        _exit(1);
-    }
-
-    /* parent */
     pid.setSeparatePG(true);
     pid.setKillSignal(SIGTERM);
     fromHook.writeSide.close();
@@ -2781,32 +2768,18 @@ void SubstitutionGoal::tryToRun()
     const char * * argArr = strings2CharPtrs(args);
 
     /* Fork the substitute program. */
-    pid = fork();
-
-    switch (pid) {
+    pid = startProcess([&]() {
 
-    case -1:
-        throw SysError("unable to fork");
+        commonChildInit(logPipe);
 
-    case 0:
-        try { /* child */
+        if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1)
+            throw SysError("cannot dup output pipe into stdout");
 
-            commonChildInit(logPipe);
+        execv(sub.c_str(), (char * *) argArr);
 
-            if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1)
-                throw SysError("cannot dup output pipe into stdout");
+        throw SysError(format("executing `%1%'") % sub);
+    });
 
-            execv(sub.c_str(), (char * *) argArr);
-
-            throw SysError(format("executing `%1%'") % sub);
-
-        } catch (std::exception & e) {
-            writeToStderr("substitute error: " + string(e.what()) + "\n");
-        }
-        _exit(1);
-    }
-
-    /* parent */
     pid.setSeparatePG(true);
     pid.setKillSignal(SIGTERM);
     outPipe.writeSide.close();
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 08ab269..e66042c 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -1083,31 +1083,16 @@ void LocalStore::startSubstituter(const Path & 
substituter, RunningSubstituter &
 
     setSubstituterEnv();
 
-    run.pid = fork();
-
-    switch (run.pid) {
-
-    case -1:
-        throw SysError("unable to fork");
-
-    case 0: /* child */
-        try {
-            restoreAffinity();
-            if (dup2(toPipe.readSide, STDIN_FILENO) == -1)
-                throw SysError("dupping stdin");
-            if (dup2(fromPipe.writeSide, STDOUT_FILENO) == -1)
-                throw SysError("dupping stdout");
-            if (dup2(errorPipe.writeSide, STDERR_FILENO) == -1)
-                throw SysError("dupping stderr");
-            execl(substituter.c_str(), substituter.c_str(), "--query", NULL);
-            throw SysError(format("executing `%1%'") % substituter);
-        } catch (std::exception & e) {
-            std::cerr << "error: " << e.what() << std::endl;
-        }
-        _exit(1);
-    }
-
-    /* Parent. */
+    run.pid = startProcess([&]() {
+        if (dup2(toPipe.readSide, STDIN_FILENO) == -1)
+            throw SysError("dupping stdin");
+        if (dup2(fromPipe.writeSide, STDOUT_FILENO) == -1)
+            throw SysError("dupping stdout");
+        if (dup2(errorPipe.writeSide, STDERR_FILENO) == -1)
+            throw SysError("dupping stderr");
+        execl(substituter.c_str(), substituter.c_str(), "--query", NULL);
+        throw SysError(format("executing `%1%'") % substituter);
+    });
 
     run.program = baseNameOf(substituter);
     run.to = toPipe.writeSide.borrow();
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 5f6203b..faa2b83 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -1,5 +1,8 @@
 #include "config.h"
 
+#include "util.hh"
+#include "affinity.hh"
+
 #include <iostream>
 #include <cerrno>
 #include <cstdio>
@@ -16,8 +19,6 @@
 #include <sys/syscall.h>
 #endif
 
-#include "util.hh"
-
 
 extern char * * environ;
 
@@ -714,6 +715,13 @@ Pid::Pid()
 }
 
 
+Pid::Pid(pid_t pid)
+{
+    Pid();
+    *this = pid;
+}
+
+
 Pid::~Pid()
 {
     kill();
@@ -801,43 +809,30 @@ void killUser(uid_t uid)
        users to which the current process can send signals.  So we
        fork a process, switch to uid, and send a mass kill. */
 
-    Pid pid;
-    pid = fork();
-    switch (pid) {
-
-    case -1:
-        throw SysError("unable to fork");
-
-    case 0:
-        try { /* child */
+    Pid pid = startProcess([&]() {
 
-            if (setuid(uid) == -1)
-                throw SysError("setting uid");
+        if (setuid(uid) == -1)
+            throw SysError("setting uid");
 
-            while (true) {
+        while (true) {
 #ifdef __APPLE__
-                /* OSX's kill syscall takes a third parameter that, among other
-                   things, determines if kill(-1, signo) affects the calling
-                   process. In the OSX libc, it's set to true, which means
-                   "follow POSIX", which we don't want here
+            /* OSX's kill syscall takes a third parameter that, among
+               other things, determines if kill(-1, signo) affects the
+               calling process. In the OSX libc, it's set to true,
+               which means "follow POSIX", which we don't want here
                  */
-                if (syscall(SYS_kill, -1, SIGKILL, false) == 0) break;
+            if (syscall(SYS_kill, -1, SIGKILL, false) == 0) break;
 #else
-                if (kill(-1, SIGKILL) == 0) break;
+            if (kill(-1, SIGKILL) == 0) break;
 #endif
-                if (errno == ESRCH) break; /* no more processes */
-                if (errno != EINTR)
-                    throw SysError(format("cannot kill processes for uid 
`%1%'") % uid);
-            }
-
-        } catch (std::exception & e) {
-            writeToStderr((format("killing processes belonging to uid `%1%': 
%2%\n") % uid % e.what()).str());
-            _exit(1);
+            if (errno == ESRCH) break; /* no more processes */
+            if (errno != EINTR)
+                throw SysError(format("cannot kill processes for uid `%1%'") % 
uid);
         }
+
         _exit(0);
-    }
+    });
 
-    /* parent */
     int status = pid.wait(true);
     if (status != 0)
         throw Error(format("cannot kill processes for uid `%1%': %2%") % uid % 
statusToString(status));
@@ -852,6 +847,25 @@ void killUser(uid_t uid)
 //////////////////////////////////////////////////////////////////////
 
 
+pid_t startProcess(std::function<void()> fun, const string & errorPrefix)
+{
+    pid_t pid = fork();
+    if (pid == -1) throw SysError("unable to fork");
+
+    if (pid == 0) {
+        try {
+            restoreAffinity();
+            fun();
+        } catch (std::exception & e) {
+            writeToStderr(errorPrefix + string(e.what()) + "\n");
+        }
+        _exit(1);
+    }
+
+    return pid;
+}
+
+
 string runProgram(Path program, bool searchPath, const Strings & args)
 {
     checkInterrupt();
@@ -867,32 +881,17 @@ string runProgram(Path program, bool searchPath, const 
Strings & args)
     pipe.create();
 
     /* Fork. */
-    Pid pid;
-    pid = fork();
-
-    switch (pid) {
+    Pid pid = startProcess([&]() {
+        if (dup2(pipe.writeSide, STDOUT_FILENO) == -1)
+            throw SysError("dupping stdout");
 
-    case -1:
-        throw SysError("unable to fork");
-
-    case 0: /* child */
-        try {
-            if (dup2(pipe.writeSide, STDOUT_FILENO) == -1)
-                throw SysError("dupping stdout");
-
-            if (searchPath)
-                execvp(program.c_str(), (char * *) &cargs[0]);
-            else
-                execv(program.c_str(), (char * *) &cargs[0]);
-            throw SysError(format("executing `%1%'") % program);
-
-        } catch (std::exception & e) {
-            writeToStderr("error: " + string(e.what()) + "\n");
-        }
-        _exit(1);
-    }
+        if (searchPath)
+            execvp(program.c_str(), (char * *) &cargs[0]);
+        else
+            execv(program.c_str(), (char * *) &cargs[0]);
 
-    /* Parent. */
+        throw SysError(format("executing `%1%'") % program);
+    });
 
     pipe.writeSide.close();
 
diff --git a/src/libutil/util.hh b/src/libutil/util.hh
index 07c027a..ad0d377 100644
--- a/src/libutil/util.hh
+++ b/src/libutil/util.hh
@@ -7,6 +7,7 @@
 #include <dirent.h>
 #include <unistd.h>
 #include <signal.h>
+#include <functional>
 
 #include <cstdio>
 
@@ -237,6 +238,7 @@ class Pid
     int killSignal;
 public:
     Pid();
+    Pid(pid_t pid);
     ~Pid();
     void operator =(pid_t pid);
     operator pid_t();
@@ -252,6 +254,11 @@ public:
 void killUser(uid_t uid);
 
 
+/* Fork a process that runs the given function, and return the child
+   pid to the caller. */
+pid_t startProcess(std::function<void()> fun, const string & errorPrefix = 
"error: ");
+
+
 /* Run a program and return its stdout in a string (i.e., like the
    shell backtick operator). */
 string runProgram(Path program, bool searchPath = false,
diff --git a/src/nix-daemon/nix-daemon.cc b/src/nix-daemon/nix-daemon.cc
index 0464ac9..265131c 100644
--- a/src/nix-daemon/nix-daemon.cc
+++ b/src/nix-daemon/nix-daemon.cc
@@ -872,40 +872,27 @@ static void daemonLoop()
             printMsg(lvlInfo, format("accepted connection from pid %1%, uid 
%2%") % clientPid % clientUid);
 
             /* Fork a child to handle the connection. */
-            pid_t child;
-            child = fork();
-
-            switch (child) {
-
-            case -1:
-                throw SysError("unable to fork");
-
-            case 0:
-                try { /* child */
-
-                    /* Background the daemon. */
-                    if (setsid() == -1)
-                        throw SysError(format("creating a new session"));
-
-                    /* Restore normal handling of SIGCHLD. */
-                    setSigChldAction(false);
-
-                    /* For debugging, stuff the pid into argv[1]. */
-                    if (clientPid != -1 && argvSaved[1]) {
-                        string processName = int2String(clientPid);
-                        strncpy(argvSaved[1], processName.c_str(), 
strlen(argvSaved[1]));
-                    }
+            startProcess([&]() {
+                /* Background the daemon. */
+                if (setsid() == -1)
+                    throw SysError(format("creating a new session"));
+
+                /* Restore normal handling of SIGCHLD. */
+                setSigChldAction(false);
+
+                /* For debugging, stuff the pid into argv[1]. */
+                if (clientPid != -1 && argvSaved[1]) {
+                    string processName = int2String(clientPid);
+                    strncpy(argvSaved[1], processName.c_str(), 
strlen(argvSaved[1]));
+                }
 
-                    /* Handle the connection. */
-                    from.fd = remote;
-                    to.fd = remote;
-                    processConnection(trusted);
+                /* Handle the connection. */
+                from.fd = remote;
+                to.fd = remote;
+                processConnection(trusted);
 
-                } catch (std::exception & e) {
-                    writeToStderr("unexpected Nix daemon error: " + 
string(e.what()) + "\n");
-                }
-                exit(0);
-            }
+                _exit(0);
+            }, "unexpected Nix daemon error: ");
 
         } catch (Interrupted & e) {
             throw;
diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc
index bb5a9e2..28b205b 100644
--- a/src/nix-store/nix-store.cc
+++ b/src/nix-store/nix-store.cc
@@ -939,27 +939,14 @@ static void opServe(Strings opFlags, Strings opArgs)
                     Pipe fromDecompressor;
                     fromDecompressor.create();
 
-                    Pid pid;
-                    pid = fork();
-
-                    switch (pid) {
-
-                        case -1:
-                            throw SysError("unable to fork");
-
-                        case 0: /* child */
-                            try {
-                                fromDecompressor.readSide.close();
-                                if (dup2(fromDecompressor.writeSide, 
STDOUT_FILENO) == -1)
-                                    throw SysError("dupping stdout");
-                                // FIXME: use absolute path.
-                                execlp(compression.c_str(), 
compression.c_str(), "-d", NULL);
-                                throw SysError(format("executing `%1%'") % 
compression);
-                            } catch (std::exception & e) {
-                                std::cerr << "error: " << e.what() << 
std::endl;
-                            }
-                            _exit(1);
-                    }
+                    Pid pid = startProcess([&]() {
+                        fromDecompressor.readSide.close();
+                        if (dup2(fromDecompressor.writeSide, STDOUT_FILENO) == 
-1)
+                            throw SysError("dupping stdout");
+                        // FIXME: use absolute path.
+                        execlp(compression.c_str(), compression.c_str(), "-d", 
NULL);
+                        throw SysError(format("executing `%1%'") % 
compression);
+                    });
 
                     fromDecompressor.writeSide.close();