From b784a0caa3cf33f46298120e0fe4be280f1526b2 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 29 Dec 2019 14:49:05 -0800 Subject: [PATCH] dup2_list_t::resolve_chain to stop returning maybe It can no longer fail. --- src/exec.cpp | 14 +++++--------- src/fish_tests.cpp | 28 +++++++++++++--------------- src/redirection.cpp | 4 ++-- src/redirection.h | 2 +- 4 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/exec.cpp b/src/exec.cpp index 02b510bad..6cbbe49ab 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -201,8 +201,8 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_io) { } // child_setup_process makes sure signals are properly set up. - auto redirs = dup2_list_t::resolve_chain(all_ios); - if (redirs && !child_setup_process(INVALID_PID, false, *redirs)) { + dup2_list_t redirs = dup2_list_t::resolve_chain(all_ios); + if (!child_setup_process(INVALID_PID, false, redirs)) { // Decrement SHLVL as we're removing ourselves from the shell "stack". auto shlvl_var = vars.get(L"SHLVL", ENV_GLOBAL | ENV_EXPORT); wcstring shlvl_str = L"0"; @@ -282,9 +282,6 @@ static bool run_internal_process(process_t *p, std::string outdata, std::string // asked to truncate a file (e.g. `echo -n '' > /tmp/truncateme.txt'). The open() in the dup2 // list resolution will ensure this happens. f->dup2s = dup2_list_t::resolve_chain(ios); - if (!f->dup2s) { - return false; - } // Figure out which source fds to write to. If they are closed (unlikely) we just exit // successfully. @@ -565,7 +562,6 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr // Convert our IO chain to a dup2 sequence. auto dup2s = dup2_list_t::resolve_chain(proc_io_chain); - if (!dup2s) return false; // Ensure that stdin is blocking before we hand it off (see issue #176). It's a // little strange that we only do this with stdin and not with stdout or stderr. @@ -584,14 +580,14 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr #if FISH_USE_POSIX_SPAWN // Prefer to use posix_spawn, since it's faster on some systems like OS X. - bool use_posix_spawn = g_use_posix_spawn && can_use_posix_spawn_for_job(j, *dup2s); + bool use_posix_spawn = g_use_posix_spawn && can_use_posix_spawn_for_job(j, dup2s); if (use_posix_spawn) { s_fork_count++; // spawn counts as a fork+exec // Create posix spawn attributes and actions. pid_t pid = 0; posix_spawnattr_t attr = posix_spawnattr_t(); posix_spawn_file_actions_t actions = posix_spawn_file_actions_t(); - bool made_it = fork_actions_make_spawn_properties(&attr, &actions, j.get(), *dup2s); + bool made_it = fork_actions_make_spawn_properties(&attr, &actions, j.get(), dup2s); if (made_it) { // We successfully made the attributes and actions; actually call // posix_spawn. @@ -653,7 +649,7 @@ static bool exec_external_command(parser_t &parser, const std::shared_ptr } else #endif { - if (!fork_child_for_process(j, p, *dup2s, "external command", + if (!fork_child_for_process(j, p, dup2s, "external command", [&] { safe_launch_process(p, actual_cmd, argv, envv); })) { return false; } diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index 2ab4f7bc1..19776b426 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -2462,14 +2462,13 @@ static void test_dup2s() { chain.push_back(make_shared(17)); chain.push_back(make_shared(3, 19)); auto list = dup2_list_t::resolve_chain(chain); - do_test(list.has_value()); - do_test(list->get_actions().size() == 2); + do_test(list.get_actions().size() == 2); - auto act1 = list->get_actions().at(0); + auto act1 = list.get_actions().at(0); do_test(act1.src == 17); do_test(act1.target == -1); - auto act2 = list->get_actions().at(1); + auto act2 = list.get_actions().at(1); do_test(act2.src == 19); do_test(act2.target == 3); } @@ -2485,17 +2484,16 @@ static void test_dup2s_fd_for_target_fd() { chain.push_back(make_shared(3, 5)); auto list = dup2_list_t::resolve_chain(chain); - do_test(list.has_value()); - do_test(list->fd_for_target_fd(3) == 8); - do_test(list->fd_for_target_fd(5) == 8); - do_test(list->fd_for_target_fd(8) == 8); - do_test(list->fd_for_target_fd(1) == 4); - do_test(list->fd_for_target_fd(4) == 4); - do_test(list->fd_for_target_fd(100) == 100); - do_test(list->fd_for_target_fd(0) == 0); - do_test(list->fd_for_target_fd(-1) == -1); - do_test(list->fd_for_target_fd(9) == -1); - do_test(list->fd_for_target_fd(10) == -1); + do_test(list.fd_for_target_fd(3) == 8); + do_test(list.fd_for_target_fd(5) == 8); + do_test(list.fd_for_target_fd(8) == 8); + do_test(list.fd_for_target_fd(1) == 4); + do_test(list.fd_for_target_fd(4) == 4); + do_test(list.fd_for_target_fd(100) == 100); + do_test(list.fd_for_target_fd(0) == 0); + do_test(list.fd_for_target_fd(-1) == -1); + do_test(list.fd_for_target_fd(9) == -1); + do_test(list.fd_for_target_fd(10) == -1); } /// Testing colors. diff --git a/src/redirection.cpp b/src/redirection.cpp index 4ae858f74..7d991fff1 100644 --- a/src/redirection.cpp +++ b/src/redirection.cpp @@ -32,7 +32,7 @@ int redirection_spec_t::oflags() const { } } -maybe_t dup2_list_t::resolve_chain(const io_chain_t &io_chain) { +dup2_list_t dup2_list_t::resolve_chain(const io_chain_t &io_chain) { ASSERT_IS_NOT_FORKED_CHILD(); dup2_list_t result; for (const auto &io_ref : io_chain) { @@ -70,7 +70,7 @@ maybe_t dup2_list_t::resolve_chain(const io_chain_t &io_chain) { } } } - return {std::move(result)}; + return result; } int dup2_list_t::fd_for_target_fd(int target) const { diff --git a/src/redirection.h b/src/redirection.h index 7bde368c4..b5c1ba10d 100644 --- a/src/redirection.h +++ b/src/redirection.h @@ -95,7 +95,7 @@ class dup2_list_t { /// Produce a dup_fd_list_t from an io_chain. This may not be called before fork(). /// The result contains the list of fd actions (dup2 and close), as well as the list /// of fds opened. - static maybe_t resolve_chain(const io_chain_t &); + static dup2_list_t resolve_chain(const io_chain_t &); /// \return the fd ultimately dup'd to a target fd, or -1 if the target is closed. /// For example, if target fd is 1, and we have a dup2 chain 5->3 and 3->1, then we will