Refactor ShellManager process spawning

src/event_manager.hh

@@ -34,7 +34,7 @@ public:
     void run(EventMode mode);
 
     void close_fd();
-    bool closed() const { return m_fd == -1; }
+    void disable() { m_fd = -1; }
 private:
 
     int       m_fd;

src/shell_manager.cc

@@ -13,8 +13,6 @@
 namespace Kakoune
 {
 
-static const Regex env_var_regex(R"(\bkak_(\w+)\b)");
-
 ShellManager::ShellManager()
 {
     const char* path = getenv("PATH");
@@ -22,103 +20,35 @@ ShellManager::ShellManager()
     setenv("PATH", new_path.c_str(), 1);
 }
 
-std::pair<String, int> ShellManager::eval(
+namespace
-    StringView cmdline, const Context& context, StringView input,
-    Flags flags, ConstArrayView<String> params, const EnvVarMap& env_vars)
 {
-    int write_pipe[2]; // child stdin
-    int read_pipe[2];  // child stdout
-    int error_pipe[2]; // child stderr
 
-    ::pipe(write_pipe);
+struct Pipe
-    ::pipe(read_pipe);
-    ::pipe(error_pipe);
-
-    if (pid_t pid = fork())
 {
-        close(write_pipe[0]);
+    Pipe() { pipe(m_fd); }
-        close(read_pipe[1]);
+    ~Pipe() { close_read_fd(); close_write_fd(); }
-        close(error_pipe[1]);
 
-        write(write_pipe[1], input.data(), (int)input.length());
+    int read_fd() const { return m_fd[0]; }
-        close(write_pipe[1]);
+    int write_fd() const { return m_fd[1]; }
 
-        String child_stdout, child_stderr;
+    void close_read_fd() { close_fd(m_fd[0]); }
-        int status = 0;
+    void close_write_fd() { close_fd(m_fd[1]); }
-        {
+
-            auto pipe_reader = [](String& output) {
+private:
-                return [&output](FDWatcher& watcher, EventMode) {
+    void close_fd(int& fd) { if (fd != -1) { close(fd); fd = -1; } }
-                    char buffer[1024];
+    int m_fd[2];
-                    size_t size = read(watcher.fd(), buffer, 1024);
-                    if (size <= 0)
-                        watcher.close_fd();
-                    output += StringView{buffer, buffer+size};
-                };
 };
 
-            FDWatcher stdout_watcher{read_pipe[0], pipe_reader(child_stdout)};
+pid_t spawn_process(StringView cmdline, ConstArrayView<String> params, ConstArrayView<String> kak_env,
-            FDWatcher stderr_watcher{error_pipe[0], pipe_reader(child_stderr)};
+                    const Pipe& child_stdout, const Pipe& child_stdin, const Pipe& child_stderr)
-
-            // block SIGCHLD to make sure we wont receive it before
-            // our call to pselect, that will end up blocking indefinitly.
-            sigset_t mask, orig_mask;
-            sigemptyset(&mask);
-            sigaddset(&mask, SIGCHLD);
-            sigprocmask(SIG_BLOCK, &mask, &orig_mask);
-
-            // check for termination now that SIGCHLD is blocked
-            bool terminated = waitpid(pid, &status, WNOHANG);
-
-            while (not terminated or
-                   ((flags & Flags::WaitForStdout) and
-                    (not stdout_watcher.closed() or
-                     not stderr_watcher.closed())))
 {
-                EventManager::instance().handle_next_events(EventMode::Urgent, &orig_mask);
+    Vector<const char*> envptrs;
-                if (not terminated)
+    for (char** envp = environ; *envp; ++envp)
-                    terminated = waitpid(pid, &status, WNOHANG);
+        envptrs.push_back(*envp);
-            }
+    for (auto& env : kak_env)
+        envptrs.push_back(env.c_str());
+    envptrs.push_back(nullptr);
 
-            stdout_watcher.close_fd();
-            stderr_watcher.close_fd();
-
-            sigprocmask(SIG_SETMASK, &orig_mask, nullptr);
-        }
-
-        if (not child_stderr.empty())
-            write_to_debug_buffer(format("shell stderr: <<<\n{}>>>", child_stderr));
-
-        return { child_stdout, WIFEXITED(status) ? WEXITSTATUS(status) : - 1 };
-    }
-    else try
-    {
-        close(write_pipe[1]);
-        close(read_pipe[0]);
-        close(error_pipe[0]);
-
-        dup2(read_pipe[1], 1); close(read_pipe[1]);
-        dup2(error_pipe[1], 2); close(error_pipe[1]);
-        dup2(write_pipe[0], 0); close(write_pipe[0]);
-
-        using RegexIt = RegexIterator<StringView::iterator>;
-        for (RegexIt it{cmdline.begin(), cmdline.end(), env_var_regex}, end;
-             it != end; ++it)
-        {
-            auto& match = *it;
-
-            StringView name{match[1].first, match[1].second};
-            kak_assert(name.length() > 0);
-
-            auto local_var = env_vars.find(name);
-            if (local_var != env_vars.end())
-                setenv(("kak_" + name).c_str(), local_var->value.c_str(), 1);
-            else try
-            {
-                String value = get_val(name, context);
-                setenv(format("kak_{}", name).c_str(), value.c_str(), 1);
-            }
-            catch (runtime_error&) {}
-        }
     const char* shell = "/bin/sh";
     auto cmdlinezstr = cmdline.zstr();
     Vector<const char*> execparams = { shell, "-c", cmdlinezstr };
@@ -128,11 +58,114 @@ std::pair<String, int> ShellManager::eval(
         execparams.push_back(param.c_str());
     execparams.push_back(nullptr);
 
-        execvp(shell, (char* const*)execparams.data());
+    if (pid_t pid = fork())
+        return pid;
+
+    auto move = [](int oldfd, int newfd) { dup2(oldfd, newfd); close(oldfd); };
+
+    close(child_stdout.write_fd());
+    move(child_stdout.read_fd(), 0);
+
+    close(child_stdin.read_fd());
+    move(child_stdin.write_fd(), 1);
+
+    close(child_stderr.read_fd());
+    move(child_stderr.write_fd(), 2);
+
+    execve(shell, (char* const*)execparams.data(), (char* const*)envptrs.data());
     exit(-1);
+    return -1;
 }
-    catch (...) { exit(-1); }
+
-    return {};
+}
+
+std::pair<String, int> ShellManager::eval(
+    StringView cmdline, const Context& context, StringView input,
+    Flags flags, ConstArrayView<String> params, const EnvVarMap& env_vars)
+{
+    static const Regex re(R"(\bkak_(\w+)\b)");
+
+    Vector<String> kak_env;
+    for (RegexIterator<const char*> it{cmdline.begin(), cmdline.end(), re}, end;
+         it != end; ++it)
+    {
+        StringView name{(*it)[1].first, (*it)[1].second};
+
+        auto match_name = [&](const String& s) {
+            return s.length() > name.length()  and
+                   prefix_match(s, name) and s[name.length()] == '=';
+        };
+        if (find_if(kak_env, match_name) != kak_env.end())
+            continue;
+
+        auto var_it = env_vars.find(name);
+        try
+        {
+            const String& value = var_it != env_vars.end() ?
+                var_it->value : get_val(name, context);
+
+            kak_env.push_back(format("kak_{}={}", name, value));
+        } catch (runtime_error&) {}
+    }
+
+    Pipe child_stdin, child_stdout, child_stderr;
+    pid_t pid = spawn_process(cmdline, params, kak_env,
+                              child_stdin, child_stdout, child_stderr);
+
+    child_stdin.close_read_fd();
+    child_stdout.close_write_fd();
+    child_stderr.close_write_fd();
+
+    write(child_stdin.write_fd(), input.data(), (int)input.length());
+    child_stdin.close_write_fd();
+
+    struct PipeReader : FDWatcher
+    {
+        PipeReader(Pipe& pipe, String& contents)
+            : FDWatcher(pipe.read_fd(),
+                        [&contents, &pipe](FDWatcher& watcher, EventMode) {
+                            char buffer[1024];
+                            size_t size = read(pipe.read_fd(), buffer, 1024);
+                            if (size <= 0)
+                            {
+                                pipe.close_read_fd();
+                                watcher.disable();
+                                return;
+                            }
+                            contents += StringView{buffer, buffer+size};
+                        })
+        {}
+    };
+
+    String stdout_contents, stderr_contents;
+    PipeReader stdout_reader{child_stdout, stdout_contents};
+    PipeReader stderr_reader{child_stderr, stderr_contents};
+
+    // block SIGCHLD to make sure we wont receive it before
+    // our call to pselect, that will end up blocking indefinitly.
+    sigset_t mask, orig_mask;
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGCHLD);
+    sigprocmask(SIG_BLOCK, &mask, &orig_mask);
+    auto restore_mask = on_scope_end([&] { sigprocmask(SIG_SETMASK, &orig_mask, nullptr); });
+
+    int status = 0;
+    // check for termination now that SIGCHLD is blocked
+    bool terminated = waitpid(pid, &status, WNOHANG);
+
+    while (not terminated or
+           ((flags & Flags::WaitForStdout) and
+            (child_stdout.read_fd() != -1 or child_stderr.read_fd() != -1)))
+    {
+        EventManager::instance().handle_next_events(EventMode::Urgent, &orig_mask);
+        if (not terminated)
+            terminated = waitpid(pid, &status, WNOHANG);
+    }
+
+    if (not stderr_contents.empty())
+        write_to_debug_buffer(format("shell stderr: <<<\n{}>>>", stderr_contents));
+
+    return { stdout_contents, WIFEXITED(status) ? WEXITSTATUS(status) : -1 };
 }
 
 void ShellManager::register_env_var(StringView str, bool prefix,