From 8e9140cfdef9dbd1eb61e4c75c91d452ab5e4a74 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 10 Jul 2014 16:50:51 +0200 Subject: [PATCH] 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 | 109 +++++++++++------------ src/libutil/util.hh | 7 ++ src/nix-daemon/nix-daemon.cc | 49 ++++------ 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 6cbcd9891..6834634f3 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 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 70a3effb2..d3184507c 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(); + pid = startProcess([&]() { - switch (pid) { + commonChildInit(logPipe); - case -1: - throw SysError("unable to fork"); + if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1) + throw SysError("cannot dup output pipe into stdout"); - case 0: - try { /* child */ + execv(sub.c_str(), (char * *) argArr); - commonChildInit(logPipe); + throw SysError(format("executing `%1%'") % sub); + }); - if (dup2(outPipe.writeSide, STDOUT_FILENO) == -1) - throw SysError("cannot dup output pipe into stdout"); - - 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 08ab269b3..e66042c57 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 5f6203bc2..faa2b83c3 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 #include #include @@ -16,8 +19,6 @@ #include #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) { + Pid pid = startProcess([&]() { - case -1: - throw SysError("unable to fork"); + if (setuid(uid) == -1) + throw SysError("setting uid"); - case 0: - try { /* child */ - - 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 */ + _exit(0); + }); + 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 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(); + Pid pid = startProcess([&]() { + if (dup2(pipe.writeSide, STDOUT_FILENO) == -1) + throw SysError("dupping stdout"); - switch (pid) { + if (searchPath) + execvp(program.c_str(), (char * *) &cargs[0]); + else + execv(program.c_str(), (char * *) &cargs[0]); - 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); - } - - /* Parent. */ + throw SysError(format("executing `%1%'") % program); + }); pipe.writeSide.close(); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 07c027a1f..ad0d377a4 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -7,6 +7,7 @@ #include #include #include +#include #include @@ -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 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 0464ac965..265131c61 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(); + startProcess([&]() { + /* Background the daemon. */ + if (setsid() == -1) + throw SysError(format("creating a new session")); - switch (child) { + /* Restore normal handling of SIGCHLD. */ + setSigChldAction(false); - 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])); - } - - /* Handle the connection. */ - from.fd = remote; - to.fd = remote; - processConnection(trusted); - - } catch (std::exception & e) { - writeToStderr("unexpected Nix daemon error: " + string(e.what()) + "\n"); + /* 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])); } - exit(0); - } + + /* Handle the connection. */ + from.fd = remote; + to.fd = remote; + processConnection(trusted); + + _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 bb5a9e2e0..28b205b1f 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();