From 1a4bb50cd5ad15e31581d50df2f62fddce35b197 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 25 Feb 2019 01:21:32 -0800 Subject: [PATCH] Combine status and pipestatus into statuses_t In most places where we set one, we want to set both. Make this less error-prone by combining them into a single type statuses_t. --- src/env.cpp | 6 +++--- src/event.cpp | 6 ++---- src/exec.cpp | 40 ++++++++++++++++------------------ src/fish.cpp | 2 +- src/input.cpp | 6 ++---- src/parse_execution.cpp | 18 ++++++++-------- src/proc.cpp | 48 ++++++++++++++++------------------------- src/proc.h | 30 +++++++++++++++++++------- 8 files changed, 76 insertions(+), 80 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index eecec7ff7..56da4dd20 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -1392,10 +1392,10 @@ maybe_t env_stack_t::get(const wcstring &key, env_mode_flags_t mode) if (history) history->get_history(result); return env_var_t(L"history", result); } else if (key == L"pipestatus") { - const auto js = proc_get_last_job_statuses(); + const auto js = proc_get_last_statuses(); wcstring_list_t result; - result.reserve(js.size()); - for (int i : js) { + result.reserve(js.pipestatus.size()); + for (int i : js.pipestatus) { result.push_back(to_string(i)); } return env_var_t(L"pipestatus", std::move(result)); diff --git a/src/event.cpp b/src/event.cpp index bdf002301..dfe991a53 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -274,16 +274,14 @@ static void event_fire_internal(const event_t &event) { // Event handlers are not part of the main flow of code, so they are marked as // non-interactive. proc_push_interactive(0); - int prev_status = proc_get_last_status(); - auto saved_job_statuses = proc_get_last_job_statuses(); + auto prev_statuses = proc_get_last_statuses(); parser_t &parser = parser_t::principal_parser(); event_block_t *b = parser.push_block(event); parser.eval(buffer, io_chain_t(), TOP); parser.pop_block(b); proc_pop_interactive(); - proc_set_last_status(prev_status); - proc_set_last_job_statuses(std::move(saved_job_statuses)); + proc_set_last_statuses(std::move(prev_statuses)); } } diff --git a/src/exec.cpp b/src/exec.cpp index 0a8281e00..7d55ca99e 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -278,7 +278,7 @@ void internal_exec_helper(parser_t &parser, parsed_source_ref_t parsed_source, t // Did the transmogrification fail - if so, set error status and return. if (!transmorgrified) { - proc_set_last_status(STATUS_EXEC_FAIL); + proc_set_last_statuses(statuses_t::just(STATUS_EXEC_FAIL)); return; } @@ -662,9 +662,7 @@ static bool handle_builtin_output(const std::shared_ptr &j, process_t *p, if (p->is_last_in_job) { debug(4, L"Set status of job %d (%ls) to %d using short circuit", j->job_id, j->preview().c_str(), p->status); - - int status_value = p->status.status_value(); - proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status_value) : status_value); + proc_set_last_statuses(j->get_statuses()); } return true; } else { @@ -838,10 +836,10 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr if (!block_output_bufferfill) { // No buffer, so we exit directly. This means we have to manually set the exit // status. - if (p->is_last_in_job) { - proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status) : status); - } p->completed = 1; + if (p->is_last_in_job) { + proc_set_last_statuses(j->get_statuses()); + } return true; } assert(block_output_bufferfill && "Must have a block output bufferfiller"); @@ -856,7 +854,7 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr return run_internal_process(p, std::move(buffer_contents), {} /*errdata*/, io_chain); } else { if (p->is_last_in_job) { - proc_set_last_status(j->get_flag(job_flag_t::NEGATE) ? (!status) : status); + proc_set_last_statuses(j->get_statuses()); } p->completed = 1; } @@ -1026,7 +1024,7 @@ bool exec_job(parser_t &parser, shared_ptr j) { // internal_exec only returns if it failed to set up redirections. // In case of an successful exec, this code is not reached. bool status = j->get_flag(job_flag_t::NEGATE) ? 0 : 1; - proc_set_last_status(status); + proc_set_last_statuses(statuses_t::just(status)); return false; } @@ -1065,7 +1063,6 @@ bool exec_job(parser_t &parser, shared_ptr j) { } j->continue_job(false); - proc_set_last_job_statuses(*j); return true; } @@ -1073,8 +1070,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin bool apply_exit_status, bool is_subcmd) { ASSERT_IS_MAIN_THREAD(); bool prev_subshell = is_subshell; - const int prev_status = proc_get_last_status(); - auto prev_job_statuses = proc_get_last_job_statuses(); + auto prev_statuses = proc_get_last_statuses(); bool split_output = false; const auto ifs = parser.vars().get(L"IFS"); @@ -1083,7 +1079,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin } is_subshell = true; - int subcommand_status = -1; // assume the worst + auto subcommand_statuses = statuses_t::just(-1); // assume the worst // IO buffer creation may fail (e.g. if we have too many open files to make a pipe), so this may // be null. @@ -1092,27 +1088,27 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin if (auto bufferfill = io_bufferfill_t::create(io_chain_t{}, read_limit)) { parser_t &parser = parser_t::principal_parser(); if (parser.eval(cmd, io_chain_t{bufferfill}, SUBST) == 0) { - subcommand_status = proc_get_last_status(); + subcommand_statuses = proc_get_last_statuses(); } buffer = io_bufferfill_t::finish(std::move(bufferfill)); } - if (buffer && buffer->buffer().discarded()) subcommand_status = STATUS_READ_TOO_MUCH; + if (buffer && buffer->buffer().discarded()) { + subcommand_statuses = statuses_t::just(STATUS_READ_TOO_MUCH); + } // If the caller asked us to preserve the exit status, restore the old status. Otherwise set the // status of the subcommand. if (apply_exit_status) { - proc_set_last_status(subcommand_status); - } - else { - proc_set_last_job_statuses(std::move(prev_job_statuses)); - proc_set_last_status(prev_status); + proc_set_last_statuses(subcommand_statuses); + } else { + proc_set_last_statuses(std::move(prev_statuses)); } is_subshell = prev_subshell; if (lst == NULL || !buffer) { - return subcommand_status; + return subcommand_statuses.status; } // Walk over all the elements. for (const auto &elem : buffer->buffer().elements()) { @@ -1151,7 +1147,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin } } - return subcommand_status; + return subcommand_statuses.status; } int exec_subshell(const wcstring &cmd, parser_t &parser, std::vector &outputs, diff --git a/src/fish.cpp b/src/fish.cpp index 1c8a0aaff..236c28a1b 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -380,7 +380,7 @@ int main(int argc, char **argv) { if (read_init(paths)) { // Stomp the exit status of any initialization commands (issue #635). - proc_set_last_status(STATUS_CMD_OK); + proc_set_last_statuses(statuses_t::just(STATUS_CMD_OK)); // Run post-config commands specified as arguments, if any. if (!opts.postconfig_cmds.empty()) { diff --git a/src/input.cpp b/src/input.cpp index ca2c1ae46..364c6eb70 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -366,13 +366,11 @@ static void input_mapping_execute(const input_mapping_t &m, bool allow_commands) // // FIXME(snnw): if commands add stuff to input queue (e.g. commandline -f execute), we won't // see that until all other commands have also been run. - int last_status = proc_get_last_status(); - auto last_job_statuses = proc_get_last_job_statuses(); + auto last_statuses = proc_get_last_statuses(); for (const wcstring &cmd : m.commands) { parser_t::principal_parser().eval(cmd, io_chain_t(), TOP); } - proc_set_last_status(last_status); - proc_set_last_job_statuses(std::move(last_job_statuses)); + proc_set_last_statuses(std::move(last_statuses)); input_common_next_ch(R_NULL); } else { // Invalid binding, mixed commands and functions. We would need to execute these one by diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 5db47a55a..6e8d2af36 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -261,7 +261,7 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( auto else_cont = else_clause.try_get_child(); if (!else_cont) { // 'if' condition failed, no else clause, return 0, we're done. - proc_set_last_status(STATUS_CMD_OK); + proc_set_last_statuses(statuses_t::just(STATUS_CMD_OK)); break; } else { // We have an 'else continuation' (either else-if or else). @@ -289,7 +289,7 @@ parse_execution_result_t parse_execution_context_t::run_if_statement( parser->pop_block(ib); } else { // No job list means no sucessful conditions, so return 0 (issue #1443). - proc_set_last_status(STATUS_CMD_OK); + proc_set_last_statuses(statuses_t::just(STATUS_CMD_OK)); } // It's possible there's a last-minute cancellation (issue #1297). @@ -325,7 +325,7 @@ parse_execution_result_t parse_execution_context_t::run_function_statement( } io_streams_t streams(0); // no limit on the amount of output from builtin_function() int err = builtin_function(*parser, streams, arguments, pstree, body); - proc_set_last_status(err); + proc_set_last_statuses(statuses_t::just(err)); if (!streams.err.empty()) { this->report_error(header, L"%ls", streams.err.contents().c_str()); @@ -543,7 +543,8 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( for (;;) { // Save off the exit status if it came from the loop body. We'll restore it if the condition // is false. - int cond_saved_status = first_cond_check ? EXIT_SUCCESS : proc_get_last_status(); + auto cond_saved_status = + first_cond_check ? statuses_t::just(EXIT_SUCCESS) : proc_get_last_statuses(); first_cond_check = false; // Check the condition. @@ -559,7 +560,7 @@ parse_execution_result_t parse_execution_context_t::run_while_statement( if (cond_ret != parse_execution_success) { break; } else if (proc_get_last_status() != EXIT_SUCCESS) { - proc_set_last_status(cond_saved_status); + proc_set_last_statuses(cond_saved_status); break; } @@ -636,7 +637,7 @@ parse_execution_result_t parse_execution_context_t::report_errors( /// Reports an unmatched wildcard error and returns parse_execution_errored. parse_execution_result_t parse_execution_context_t::report_unmatched_wildcard_error( const parse_node_t &unmatched_wildcard) const { - proc_set_last_status(STATUS_UNMATCHED_WILDCARD); + proc_set_last_statuses(statuses_t::just(STATUS_UNMATCHED_WILDCARD)); report_error(unmatched_wildcard, WILDCARD_ERR_MSG, get_source(unmatched_wildcard).c_str()); return parse_execution_errored; } @@ -718,8 +719,7 @@ parse_execution_result_t parse_execution_context_t::handle_command_not_found( // Set the last proc status appropriately. int status = err_code == ENOENT ? STATUS_CMD_UNKNOWN : STATUS_NOT_EXECUTABLE; - proc_set_last_status(status); - proc_set_last_job_statuses({status}); + proc_set_last_statuses(statuses_t::just(status)); return parse_execution_errored; } @@ -741,7 +741,7 @@ parse_execution_result_t parse_execution_context_t::expand_command( expand_error_t expand_err = expand_to_command_and_args(unexp_cmd, parser->vars(), out_cmd, out_args, &errors); if (expand_err == EXPAND_ERROR) { - proc_set_last_status(STATUS_ILLEGAL_CMD); + proc_set_last_statuses(statuses_t::just(STATUS_ILLEGAL_CMD)); return report_errors(errors); } else if (expand_err == EXPAND_WILDCARD_NO_MATCH) { return report_unmatched_wildcard_error(statement); diff --git a/src/proc.cpp b/src/proc.cpp index f0c9cb482..72f6fd3bd 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -49,11 +49,8 @@ #include "util.h" #include "wutil.h" // IWYU pragma: keep -/// Status of last process to exit. -static int last_status = 0; - /// Statuses of last job's processes to exit - ensure we start off with one entry of 0. -static owning_lock> last_job_statuses{std::vector(1u, 0)}; +static owning_lock last_statuses{statuses_t::just(0)}; /// The signals that signify crashes to us. static const int crashsignals[] = {SIGABRT, SIGBUS, SIGFPE, SIGILL, SIGSEGV, SIGSYS}; @@ -134,30 +131,14 @@ void proc_destroy() { } } -void proc_set_last_status(int s) { +void proc_set_last_statuses(statuses_t s) { ASSERT_IS_MAIN_THREAD(); - last_status = s; + *last_statuses.acquire() = std::move(s); } -int proc_get_last_status() { return last_status; } +int proc_get_last_status() { return last_statuses.acquire()->status; } -void proc_set_last_job_statuses(const job_t &last_job) { - ASSERT_IS_MAIN_THREAD(); - std::vector ljs; - ljs.reserve(last_job.processes.size()); - for (const auto &p : last_job.processes) { - ljs.push_back(p->status.status_value()); - } - proc_set_last_job_statuses(std::move(ljs)); -} - -void proc_set_last_job_statuses(std::vector statuses) { - ASSERT_IS_MAIN_THREAD(); - auto vals = last_job_statuses.acquire(); - *vals = std::move(statuses); -} - -std::vector proc_get_last_job_statuses() { return *last_job_statuses.acquire(); } +statuses_t proc_get_last_statuses() { return *last_statuses.acquire(); } // Basic thread safe job IDs. The vector consumed_job_ids has a true value wherever the job ID // corresponding to that slot is in use. The job ID corresponding to slot 0 is 1. @@ -264,6 +245,17 @@ bool job_t::signal(int signal) { return true; } +statuses_t job_t::get_statuses() const { + statuses_t st{}; + st.pipestatus.reserve(processes.size()); + for (const auto &p : processes) { + st.pipestatus.push_back(p->status.status_value()); + } + int laststatus = st.pipestatus.back(); + st.status = (get_flag(job_flag_t::NEGATE) ? !laststatus : laststatus); + return st; +} + void internal_proc_t::mark_exited(proc_status_t status) { assert(!exited() && "Process is already exited"); status_.store(status, std::memory_order_relaxed); @@ -651,12 +643,12 @@ bool job_reap(bool allow_interactive) { process_mark_finished_children(false); // Preserve the exit status. - const int saved_status = proc_get_last_status(); + auto saved_statuses = proc_get_last_statuses(); found = process_clean_after_marking(allow_interactive); // Restore the exit status. - proc_set_last_status(saved_status); + proc_set_last_statuses(std::move(saved_statuses)); return found; } @@ -930,11 +922,9 @@ void job_t::continue_job(bool send_sigcont) { if (is_foreground() && is_completed()) { // Set $status only if we are in the foreground and the last process in the job has // finished and is not a short-circuited builtin. - bool negate = get_flag(job_flag_t::NEGATE); auto &p = processes.back(); if (p->status.normal_exited() || p->status.signal_exited()) { - int status_code = p->status.status_value(); - proc_set_last_status(negate ? !status_code : status_code); + proc_set_last_statuses(get_statuses()); } } } diff --git a/src/proc.h b/src/proc.h index 9bd302f9e..9da35777d 100644 --- a/src/proc.h +++ b/src/proc.h @@ -261,6 +261,23 @@ enum class job_flag_t { JOB_FLAG_COUNT }; +/// A collection of status and pipestatus. +struct statuses_t { + /// Status of the last job to exit. + int status{0}; + + /// Pipestatus value. + std::vector pipestatus{}; + + /// Return a statuses for a single process status. + static statuses_t just(int s) { + statuses_t result{}; + result.status = s; + result.pipestatus.push_back(s); + return result; + } +}; + template <> struct enum_info_t { static constexpr auto count = job_flag_t::JOB_FLAG_COUNT; @@ -401,6 +418,9 @@ class job_t { /// \return true on success, false on failure. bool signal(int signal); + /// \returns the statuses for this job. + statuses_t get_statuses() const; + /// Return the job instance matching this unique job id. /// If id is 0 or less, return the last job used. static job_t *from_job_id(job_id_t id); @@ -478,17 +498,11 @@ extern int job_control_mode; extern int no_exec; /// Sets the status of the last process to exit. -void proc_set_last_status(int s); +void proc_set_last_statuses(statuses_t s); /// Returns the status of the last process to exit. int proc_get_last_status(); - -/// Sets the status of the last job's processes to exit from last_job. -void proc_set_last_job_statuses(const job_t &last_job); -void proc_set_last_job_statuses(std::vector statuses); - -/// Returns the statuses of the last job's processes to exit. -std::vector proc_get_last_job_statuses(); +statuses_t proc_get_last_statuses(); /// Notify the user about stopped or terminated jobs. Delete terminated jobs from the job list. ///