From b5716e97cc11087b8ce518a9676618e7810d688e Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Fri, 5 Feb 2021 18:14:50 -0800 Subject: [PATCH] Remove fd_set_t Now that we no longer need to worry about pipes conflicting with user-specified redirections, we can remove fd_set_t. --- src/builtin_eval.cpp | 6 ++---- src/exec.cpp | 28 +++++++++------------------- src/fds.h | 20 -------------------- src/fish_tests.cpp | 18 +----------------- src/io.cpp | 12 +----------- src/io.h | 8 +------- 6 files changed, 14 insertions(+), 78 deletions(-) diff --git a/src/builtin_eval.cpp b/src/builtin_eval.cpp index 73941e89e..19a09d2b3 100644 --- a/src/builtin_eval.cpp +++ b/src/builtin_eval.cpp @@ -38,8 +38,7 @@ maybe_t builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **arg // buffer in that case. shared_ptr stdout_fill{}; if (streams.out_is_piped) { - stdout_fill = - io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO); + stdout_fill = io_bufferfill_t::create(parser.libdata().read_limit, STDOUT_FILENO); if (!stdout_fill) { // We were unable to create a pipe, probably fd exhaustion. return STATUS_CMD_ERROR; @@ -50,8 +49,7 @@ maybe_t builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **arg // Of course the same applies to stderr. shared_ptr stderr_fill{}; if (streams.err_is_piped) { - stderr_fill = - io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO); + stderr_fill = io_bufferfill_t::create(parser.libdata().read_limit, STDERR_FILENO); if (!stderr_fill) { return STATUS_CMD_ERROR; } diff --git a/src/exec.cpp b/src/exec.cpp index bf87e694f..90fd31a19 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -658,16 +658,15 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job, } /// Execute a block node or function "process". -/// \p conflicts contains the list of fds which pipes should avoid. /// \p allow_buffering if true, permit buffering the output. static launch_result_t exec_block_or_func_process(parser_t &parser, const std::shared_ptr &j, - process_t *p, const fd_set_t &conflicts, - io_chain_t io_chain, bool allow_buffering) { + process_t *p, io_chain_t io_chain, + bool allow_buffering) { // Create an output buffer if we're piping to another process. shared_ptr block_output_bufferfill{}; if (!p->is_last_in_job && allow_buffering) { // Be careful to handle failure, e.g. too many open fds. - block_output_bufferfill = io_bufferfill_t::create(conflicts); + block_output_bufferfill = io_bufferfill_t::create(); if (!block_output_bufferfill) { return launch_result_t::failed; } @@ -709,7 +708,6 @@ static launch_result_t exec_block_or_func_process(parser_t &parser, const std::s static launch_result_t exec_process_in_job(parser_t &parser, process_t *p, const std::shared_ptr &j, const io_chain_t &block_io, autoclose_pipes_t pipes, - const fd_set_t &conflicts, const autoclose_pipes_t &deferred_pipes, bool is_deferred_run = false) { // The write pipe (destined for stdout) needs to occur before redirections. For example, @@ -800,8 +798,8 @@ static launch_result_t exec_process_in_job(parser_t &parser, process_t *p, // Allow buffering unless this is a deferred run. If deferred, then processes after us // were already launched, so they are ready to receive (or reject) our output. bool allow_buffering = !is_deferred_run; - if (exec_block_or_func_process(parser, j, p, conflicts, process_net_io_chain, - allow_buffering) == launch_result_t::failed) { + if (exec_block_or_func_process(parser, j, p, process_net_io_chain, allow_buffering) == + launch_result_t::failed) { return launch_result_t::failed; } break; @@ -907,14 +905,6 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const io_chain_t &bl return true; } - // Get the list of all FDs so we can ensure our pipes do not conflict. - fd_set_t conflicts = block_io.fd_set(); - for (const auto &p : j->processes) { - for (const auto &spec : p->redirection_specs()) { - conflicts.add(spec.fd); - } - } - // Handle an exec call. if (j->processes.front()->type == process_type_t::exec) { // If we are interactive, perhaps disallow exec if there are background jobs. @@ -987,8 +977,8 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const io_chain_t &bl } // Regular process. - if (exec_process_in_job(parser, p, j, block_io, std::move(proc_pipes), conflicts, - deferred_pipes) == launch_result_t::failed) { + if (exec_process_in_job(parser, p, j, block_io, std::move(proc_pipes), deferred_pipes) == + launch_result_t::failed) { aborted_pipeline = true; abort_pipeline_from(j, p); break; @@ -1012,7 +1002,7 @@ bool exec_job(parser_t &parser, const shared_ptr &j, const io_chain_t &bl // Some other process already aborted our pipeline. deferred_process->mark_aborted_before_launch(); } else if (exec_process_in_job(parser, deferred_process, j, block_io, - std::move(deferred_pipes), conflicts, {}, + std::move(deferred_pipes), {}, true) == launch_result_t::failed) { // The deferred proc itself failed to launch. deferred_process->mark_aborted_before_launch(); @@ -1104,7 +1094,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may // be null. - auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit); + auto bufferfill = io_bufferfill_t::create(ld.read_limit); if (!bufferfill) { *break_expand = true; return STATUS_CMD_ERROR; diff --git a/src/fds.h b/src/fds.h index 21dcd9c99..29858906d 100644 --- a/src/fds.h +++ b/src/fds.h @@ -71,26 +71,6 @@ struct autoclose_pipes_t { : read(std::move(r)), write(std::move(w)) {} }; -/// A simple set of FDs. -struct fd_set_t { - std::vector fds; - - void add(int fd) { - assert(fd >= 0 && "Invalid fd"); - if (static_cast(fd) >= fds.size()) { - fds.resize(fd + 1); - } - fds[fd] = true; - } - - bool contains(int fd) const { - assert(fd >= 0 && "Invalid fd"); - return static_cast(fd) < fds.size() && fds[fd]; - } - - bool empty() const { return fds.empty(); } -}; - /// Call pipe(), populating autoclose fds. /// The pipes are marked CLO_EXEC and are placed in the high fd range. /// \return pipes on success, none() on error. diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 789aeafd4..d3edba1a4 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1293,7 +1293,7 @@ static void test_parser() { } static void test_1_cancellation(const wchar_t *src) { - auto filler = io_bufferfill_t::create(fd_set_t{}); + auto filler = io_bufferfill_t::create(); pthread_t thread = pthread_self(); double delay = 0.50 /* seconds */; iothread_perform([=]() { @@ -3541,21 +3541,6 @@ static void test_input() { } } -static void test_fd_set() { - say(L"Testing fd_set"); - fd_set_t fds; - do_test(!fds.contains(0)); - do_test(!fds.contains(100)); - fds.add(1); - do_test(!fds.contains(0)); - do_test(!fds.contains(100)); - do_test(fds.contains(1)); - fds.add(1); - do_test(!fds.contains(0)); - do_test(!fds.contains(100)); - do_test(fds.contains(1)); -} - static void test_line_iterator() { say(L"Testing line iterator"); @@ -6335,7 +6320,6 @@ int main(int argc, char **argv) { if (should_test_function("complete")) test_complete(); if (should_test_function("autoload")) test_autoload(); if (should_test_function("input")) test_input(); - if (should_test_function("io")) test_fd_set(); if (should_test_function("line_iterator")) test_line_iterator(); if (should_test_function("undo")) test_undo(); if (should_test_function("universal")) test_universal(); diff --git a/src/io.cpp b/src/io.cpp index cc5642c23..742f6bf41 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -156,9 +156,7 @@ separated_buffer_t io_buffer_t::complete_background_fillthread_and_take_buffer() return result; } -shared_ptr io_bufferfill_t::create(const fd_set_t &conflicts, size_t buffer_limit, - int target) { - (void)conflicts; +shared_ptr io_bufferfill_t::create(size_t buffer_limit, int target) { assert(target >= 0 && "Invalid target fd"); // Construct our pipes. @@ -277,14 +275,6 @@ void io_chain_t::print() const { } } -fd_set_t io_chain_t::fd_set() const { - fd_set_t result; - for (const auto &io : *this) { - result.add(io->fd); - } - return result; -} - shared_ptr io_chain_t::io_for_fd(int fd) const { for (auto iter = rbegin(); iter != rend(); ++iter) { const auto &data = *iter; diff --git a/src/io.h b/src/io.h index 7a7681130..bb8d38e2d 100644 --- a/src/io.h +++ b/src/io.h @@ -276,10 +276,7 @@ class io_bufferfill_t final : public io_data_t { /// \returns nullptr on failure, e.g. too many open fds. /// /// \param target the fd which this will be dup2'd to - typically stdout. - /// \param conflicts A set of fds. The function ensures that any pipe it makes does - /// not conflict with an fd redirection in this list. - static shared_ptr create(const fd_set_t &conflicts, size_t buffer_limit = 0, - int target = STDOUT_FILENO); + static shared_ptr create(size_t buffer_limit = 0, int target = STDOUT_FILENO); /// Reset the receiver (possibly closing the write end of the pipe), and complete the fillthread /// of the buffer. \return the buffer. @@ -357,9 +354,6 @@ class io_chain_t : public std::vector { /// Output debugging information to stderr. void print() const; - - /// \return the set of redirected FDs. - fd_set_t fd_set() const; }; /// Base class representing the output that a builtin can generate.