From ab189a75abeeeaf5cbddc2ffc7e865567fb39bc6 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 23 Jan 2017 09:28:34 -0800 Subject: [PATCH] Switch a job's process list from a linked list to a vector of pointers Clarifies and simplifies the memory management around process handling. --- src/builtin_jobs.cpp | 10 +++--- src/exec.cpp | 44 +++++++++++++-------------- src/expand.cpp | 3 +- src/parse_execution.cpp | 28 +++++++---------- src/parser.cpp | 2 +- src/proc.cpp | 67 ++++++++++++++++++----------------------- src/proc.h | 18 ++++++----- 7 files changed, 79 insertions(+), 93 deletions(-) diff --git a/src/builtin_jobs.cpp b/src/builtin_jobs.cpp index 456385672..8e8c8679c 100644 --- a/src/builtin_jobs.cpp +++ b/src/builtin_jobs.cpp @@ -28,13 +28,12 @@ enum { /// Calculates the cpu usage (in percent) of the specified job. static int cpu_use(const job_t *j) { double u = 0; - process_t *p; - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { struct timeval t; int jiffies; gettimeofday(&t, 0); - jiffies = proc_get_jiffies(p); + jiffies = proc_get_jiffies(p.get()); double t1 = 1000000.0 * p->last_time.tv_sec + p->last_time.tv_usec; double t2 = 1000000.0 * t.tv_sec + t.tv_usec; @@ -48,7 +47,6 @@ static int cpu_use(const job_t *j) { /// Print information about the specified job. static void builtin_jobs_print(const job_t *j, int mode, int header, io_streams_t &streams) { - process_t *p; switch (mode) { case JOBS_DEFAULT: { if (header) { @@ -85,7 +83,7 @@ static void builtin_jobs_print(const job_t *j, int mode, int header, io_streams_ streams.out.append(_(L"Process\n")); } - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { streams.out.append_format(L"%d\n", p->pid); } break; @@ -96,7 +94,7 @@ static void builtin_jobs_print(const job_t *j, int mode, int header, io_streams_ streams.out.append(_(L"Command\n")); } - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { streams.out.append_format(L"%ls\n", p->argv0()); } break; diff --git a/src/exec.cpp b/src/exec.cpp index 4dd046488..b9948644d 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -397,7 +397,7 @@ void exec_job(parser_t &parser, job_t *j) { } } - if (j->first_process->type == INTERNAL_EXEC) { + if (j->processes.front()->type == INTERNAL_EXEC) { // Do a regular launch - but without forking first... signal_block(); @@ -421,10 +421,10 @@ void exec_job(parser_t &parser, job_t *j) { env_set(L"SHLVL", nshlvl_str.c_str(), ENV_GLOBAL | ENV_EXPORT); // launch_process _never_ returns. - launch_process_nofork(j->first_process); + launch_process_nofork(j->processes.front().get()); } else { job_set_flag(j, JOB_CONSTRUCTED, 1); - j->first_process->completed = 1; + j->processes.front()->completed = 1; return; } DIE("this should be unreachable"); @@ -440,7 +440,7 @@ void exec_job(parser_t &parser, job_t *j) { if (!io_buffer->avoid_conflicts_with_io_chain(all_ios)) { // We could not avoid conflicts, probably due to fd exhaustion. Mark an error. exec_error = true; - job_mark_process_as_failed(j, j->first_process); + job_mark_process_as_failed(j, j->processes.front().get()); break; } } @@ -453,13 +453,9 @@ void exec_job(parser_t &parser, job_t *j) { // builtin/block/function is inside a pipeline, since that usually means we have to wait for one // program to exit before continuing in the pipeline, causing the group leader to exit. if (job_get_flag(j, JOB_CONTROL) && !exec_error) { - for (const process_t *p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (p->type != EXTERNAL) { - if (p->next) { - needs_keepalive = true; - break; - } - if (p != j->first_process) { + if (! p->is_last_in_job || ! p->is_first_in_job) { needs_keepalive = true; break; } @@ -498,7 +494,11 @@ void exec_job(parser_t &parser, job_t *j) { // We are careful to set these to -1 when closed, so if we exit the loop abruptly, we can still // close them. int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; - for (process_t *p = j->first_process; p != NULL && !exec_error; p = p->next) { + for (std::unique_ptr &unique_p : j->processes) { + if (exec_error) { + break; + } + process_t * const p = unique_p.get(); // The IO chain for this process. It starts with the block IO, then pipes, and then gets any // from the process. io_chain_t process_net_io_chain = j->block_io_chain(); @@ -509,7 +509,7 @@ void exec_job(parser_t &parser, job_t *j) { pipe_next_read = -1; // See if we need a pipe. - const bool pipes_to_next_command = (p->next != NULL); + const bool pipes_to_next_command = ! p->is_last_in_job; // The pipes the current process write to and read from. Unfortunately these can't be just // allocated on the stack, since j->io wants shared_ptr. @@ -545,7 +545,7 @@ void exec_job(parser_t &parser, job_t *j) { shared_ptr pipe_read; // Write pipe goes first. - if (p->next) { + if (pipes_to_next_command) { pipe_write.reset(new io_pipe_t(p->pipe_write_fd, false)); process_net_io_chain.push_back(pipe_write); } @@ -554,7 +554,7 @@ void exec_job(parser_t &parser, job_t *j) { process_net_io_chain.append(p->io_chain()); // Read pipe goes last. - if (p != j->first_process) { + if (! p->is_first_in_job) { pipe_read.reset(new io_pipe_t(p->pipe_read_fd, true)); // Record the current read in pipe_read. pipe_read->pipe_fd[0] = pipe_current_read; @@ -640,7 +640,7 @@ void exec_job(parser_t &parser, job_t *j) { parser.forbid_function(func_name); - if (p->next) { + if (! p->is_last_in_job) { // Be careful to handle failure, e.g. too many open fds. block_output_io_buffer = io_buffer_t::create(STDOUT_FILENO, all_ios); if (block_output_io_buffer.get() == NULL) { @@ -667,7 +667,7 @@ void exec_job(parser_t &parser, job_t *j) { } case INTERNAL_BLOCK_NODE: { - if (p->next) { + if (! p->is_last_in_job) { block_output_io_buffer = io_buffer_t::create(STDOUT_FILENO, all_ios); if (block_output_io_buffer.get() == NULL) { // We failed (e.g. no more fds could be created). @@ -693,7 +693,7 @@ void exec_job(parser_t &parser, job_t *j) { // If this is the first process, check the io redirections and see where we should // be reading from. - if (p == j->first_process) { + if (p->is_first_in_job) { const shared_ptr in = process_net_io_chain.get_io_for_fd(STDIN_FILENO); @@ -757,7 +757,7 @@ void exec_job(parser_t &parser, job_t *j) { } else { // Determine if we have a "direct" redirection for stdin. bool stdin_is_directly_redirected; - if (p != j->first_process) { + if (! p->is_first_in_job) { // We must have a pipe stdin_is_directly_redirected = true; } else { @@ -831,7 +831,7 @@ void exec_job(parser_t &parser, job_t *j) { if (!block_output_io_buffer.get()) { // No buffer, so we exit directly. This means we have to manually set the exit // status. - if (p->next == NULL) { + if (p->is_last_in_job) { proc_set_last_status(job_get_flag(j, JOB_NEGATE) ? (!status) : status); } p->completed = 1; @@ -866,7 +866,7 @@ void exec_job(parser_t &parser, job_t *j) { } } else { - if (p->next == 0) { + if (p->is_last_in_job) { proc_set_last_status(job_get_flag(j, JOB_NEGATE) ? (!status) : status); } p->completed = 1; @@ -896,7 +896,7 @@ void exec_job(parser_t &parser, job_t *j) { // output, so that we can truncate the file. Does not apply to /dev/null. bool must_fork = redirection_is_to_real_file(stdout_io.get()) || redirection_is_to_real_file(stderr_io.get()); - if (!must_fork && p->next == NULL) { + if (!must_fork && p->is_last_in_job) { const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER; const bool no_stdout_output = stdout_buffer.empty(); const bool no_stderr_output = stderr_buffer.empty(); @@ -939,7 +939,7 @@ void exec_job(parser_t &parser, job_t *j) { if (fork_was_skipped) { p->completed = 1; - if (p->next == 0) { + if (p->is_last_in_job) { debug(3, L"Set status of %ls to %d using short circuit", j->command_wcstr(), p->status); diff --git a/src/expand.cpp b/src/expand.cpp index 2ea9459db..225c94b80 100644 --- a/src/expand.cpp +++ b/src/expand.cpp @@ -519,9 +519,8 @@ static int find_job(const struct find_job_data_t *info) { jobs.reset(); while ((j = jobs.next())) { - process_t *p; if (j->command_is_empty()) continue; - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (p->actual_cmd.empty()) continue; size_t offset; diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 1073bd44b..5e312d5a0 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1146,9 +1146,9 @@ parse_execution_result_t parse_execution_context_t::populate_job_from_job_node( parse_execution_result_t result = parse_execution_success; // Create processes. Each one may fail. - std::vector processes; - processes.push_back(new process_t()); - result = this->populate_job_process(j, processes.back(), *statement_node); + process_list_t processes; + processes.emplace_back(new process_t()); + result = this->populate_job_process(j, processes.back().get(), *statement_node); // Construct process_ts for job continuations (pipelines), by walking the list until we hit the // terminal (empty) job continuation. @@ -1171,29 +1171,23 @@ parse_execution_result_t parse_execution_context_t::populate_job_from_job_node( assert(statement_node != NULL); // Store the new process (and maybe with an error). - processes.push_back(new process_t()); - result = this->populate_job_process(j, processes.back(), *statement_node); + processes.emplace_back(new process_t()); + result = this->populate_job_process(j, processes.back().get(), *statement_node); // Get the next continuation. job_cont = get_child(*job_cont, 2, symbol_job_continuation); assert(job_cont != NULL); } + // Inform our processes of who is first and last + processes.front()->is_first_in_job = true; + processes.back()->is_last_in_job = true; + // Return what happened. if (result == parse_execution_success) { // Link up the processes. assert(!processes.empty()); //!OCLINT(multiple unary operator) - j->first_process = processes.at(0); - for (size_t i = 1; i < processes.size(); i++) { - processes.at(i - 1)->next = processes.at(i); - } - } else { - // Clean up processes. - for (size_t i = 0; i < processes.size(); i++) { - const process_t *proc = processes.at(i); - processes.at(i) = NULL; - delete proc; - } + j->processes = std::move(processes); } return result; } @@ -1313,7 +1307,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(const parse_node_t // Check to see if this contained any external commands. bool job_contained_external_command = false; - for (const process_t *proc = j->first_process; proc != NULL; proc = proc->next) { + for (const auto &proc : j->processes) { if (proc->type == EXTERNAL) { job_contained_external_command = true; break; diff --git a/src/parser.cpp b/src/parser.cpp index 3e36aa872..0f2c56035 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -525,7 +525,7 @@ wcstring parser_t::current_line() { void parser_t::job_add(job_t *job) { assert(job != NULL); - assert(job->first_process != NULL); + assert(! job->processes.empty()); this->my_job_list.push_front(job); } diff --git a/src/proc.cpp b/src/proc.cpp index 082c70af7..9918bb510 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -214,9 +214,7 @@ job_t *job_get_from_pid(int pid) { /// /// \param j the job to test int job_is_stopped(const job_t *j) { - process_t *p; - - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (!p->completed && !p->stopped) { return 0; } @@ -228,9 +226,9 @@ int job_is_stopped(const job_t *j) { /// /// \param j the job to test bool job_is_completed(const job_t *j) { - assert(j->first_process != NULL); + assert(! j->processes.empty()); bool result = true; - for (process_t *p = j->first_process; p != NULL; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (!p->completed) { result = false; break; @@ -256,7 +254,7 @@ int job_signal(job_t *j, int signal) { if (j->pgid != my_pid) { res = killpg(j->pgid, signal); } else { - for (process_t *p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (!p->completed && p->pid && kill(p->pid, signal)) { res = -1; break; @@ -284,12 +282,15 @@ static void mark_process_status(process_t *p, int status) { } } -void job_mark_process_as_failed(const job_t *job, process_t *p) { +void job_mark_process_as_failed(job_t *job, const process_t *failed_proc) { // The given process failed to even lift off (e.g. posix_spawn failed) and so doesn't have a - // valid pid. Mark it as dead. - UNUSED(job); - for (process_t *cursor = p; cursor != NULL; cursor = cursor->next) { - cursor->completed = 1; + // valid pid. Mark it and everything after it as dead. + bool found = false; + for (process_ptr_t &p : job->processes) { + found = found || (p.get() == failed_proc); + if (found) { + p->completed = true; + } } } @@ -299,22 +300,22 @@ void job_mark_process_as_failed(const job_t *job, process_t *p) { /// \param status the status as returned by wait static void handle_child_status(pid_t pid, int status) { bool found_proc = false; - const job_t *j = NULL; + job_t *j = NULL; process_t *p = NULL; job_iterator_t jobs; while (!found_proc && (j = jobs.next())) { - process_t *prev = 0; - for (p = j->first_process; p; p = p->next) { + process_t *prev = NULL; + for (process_ptr_t &p : j->processes) { if (pid == p->pid) { - mark_process_status(p, status); + mark_process_status(p.get(), status); if (p->completed && prev && !prev->completed && prev->pid) { kill(prev->pid, SIGPIPE); } found_proc = true; break; } - prev = p; + prev = p.get(); } } @@ -350,7 +351,9 @@ static void handle_child_status(pid_t pid, int status) { } process_t::process_t() - : type(), // gets set later + : is_first_in_job(), + is_last_in_job(), + type(), // gets set later internal_block_node(NODE_OFFSET_INVALID), pid(0), pipe_write_fd(0), @@ -358,8 +361,7 @@ process_t::process_t() completed(0), stopped(0), status(0), - count_help_magic(0), - next(NULL) + count_help_magic(0) #ifdef HAVE__PROC_SELF_STAT , last_time(), @@ -368,20 +370,17 @@ process_t::process_t() { } -process_t::~process_t() { delete this->next; } - job_t::job_t(job_id_t jobid, const io_chain_t &bio) - : block_io(bio), first_process(NULL), pgid(0), tmodes(), job_id(jobid), flags(0) {} + : block_io(bio), pgid(0), tmodes(), job_id(jobid), flags(0) {} job_t::~job_t() { - delete first_process; release_job_id(job_id); } /// Return all the IO redirections. Start with the block IO, then walk over the processes. io_chain_t job_t::all_io_redirections() const { io_chain_t result = this->block_io; - for (process_t *p = this->first_process; p != NULL; p = p->next) { + for (const process_ptr_t &p : this->processes) { result.append(p->io_chain()); } return result; @@ -557,7 +556,7 @@ int job_reap(bool allow_interactive) { continue; } - for (process_t *p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { int s; if (!p->completed) continue; @@ -575,7 +574,7 @@ int job_reap(bool allow_interactive) { } // Handle signals other than SIGPIPE. - int proc_is_job = ((p == j->first_process) && (p->next == 0)); + int proc_is_job = (p->is_first_in_job && p->is_last_in_job); if (proc_is_job) job_set_flag(j, JOB_NOTIFIED, 1); if (job_get_flag(j, JOB_SKIP_NOTIFICATION)) { continue; @@ -884,9 +883,7 @@ void job_continue(job_t *j, bool cont) { // Send the job a continue signal, if necessary. if (cont) { - process_t *p; - - for (p = j->first_process; p; p = p->next) p->stopped = 0; + for (process_ptr_t &p : j->processes) p->stopped = false; if (job_get_flag(j, JOB_CONTROL)) { if (killpg(j->pgid, SIGCONT)) { @@ -894,7 +891,7 @@ void job_continue(job_t *j, bool cont) { return; } } else { - for (p = j->first_process; p; p = p->next) { + for (const process_ptr_t &p : j->processes) { if (kill(p->pid, SIGCONT) < 0) { wperror(L"kill (SIGCONT)"); return; @@ -951,8 +948,7 @@ void job_continue(job_t *j, bool cont) { // order! read_try(j); - process_t *p = j->first_process; - while (p->next) p = p->next; + const std::unique_ptr &p = j->processes.back(); // Mark process status only if we are in the foreground and the last process in a pipe, // and it is not a short circuited builtin. @@ -994,11 +990,9 @@ void proc_sanity_check() { job_iterator_t jobs; while ((j = jobs.next())) { - process_t *p; if (!job_get_flag(j, JOB_CONSTRUCTED)) continue; - validate_pointer(j->first_process, _(L"Process list pointer"), 0); // More than one foreground job? if (job_get_flag(j, JOB_FOREGROUND) && !(job_is_stopped(j) || job_is_completed(j))) { @@ -1010,13 +1004,11 @@ void proc_sanity_check() { fg_job = j; } - p = j->first_process; - while (p) { + for (const process_ptr_t &p : j->processes) { // Internal block nodes do not have argv - see issue #1545. bool null_ok = (p->type == INTERNAL_BLOCK_NODE); validate_pointer(p->get_argv(), _(L"Process argument list"), null_ok); validate_pointer(p->argv0(), _(L"Process name"), null_ok); - validate_pointer(p->next, _(L"Process list pointer"), true); if ((p->stopped & (~0x00000001)) != 0) { debug(0, _(L"Job '%ls', process '%ls' has inconsistent state \'stopped\'=%d"), @@ -1030,7 +1022,6 @@ void proc_sanity_check() { sanity_lose(); } - p = p->next; } } } diff --git a/src/proc.h b/src/proc.h index 7990174d0..f3f59c74c 100644 --- a/src/proc.h +++ b/src/proc.h @@ -90,7 +90,10 @@ class process_t { public: process_t(); - ~process_t(); + + // Note whether we are the first and/or last in the job + bool is_first_in_job; + bool is_last_in_job; /// Type of process. Can be one of \c EXTERNAL, \c INTERNAL_BUILTIN, \c INTERNAL_FUNCTION, \c /// INTERNAL_EXEC. @@ -140,8 +143,6 @@ class process_t { volatile int status; /// Special flag to tell the evaluation function for count to print the help information. int count_help_magic; - /// Next process in pipeline. We own this and we are responsible for deleting it. - process_t *next; #ifdef HAVE__PROC_SELF_STAT /// Last time of cpu time check. struct timeval last_time; @@ -150,6 +151,9 @@ class process_t { #endif }; +typedef std::unique_ptr process_ptr_t; +typedef std::vector process_list_t; + /// Constants for the flag variable in the job struct. enum { /// Whether the user has been told about stopped job. @@ -204,9 +208,9 @@ class job_t { /// Sets the command. void set_command(const wcstring &cmd) { command_str = cmd; } - /// A linked list of all the processes in this job. We are responsible for deleting this when we - /// are deallocated. - process_t *first_process; + /// All the processes in this job. + process_list_t processes; + /// Process group ID for the process group that this job is running in. pid_t pgid; /// The saved terminal modes of this job. This needs to be saved so that we can restore the @@ -342,7 +346,7 @@ void job_handle_signal(int signal, siginfo_t *info, void *con); int job_signal(job_t *j, int signal); /// Mark a process as failed to execute (and therefore completed). -void job_mark_process_as_failed(const job_t *job, process_t *p); +void job_mark_process_as_failed(job_t *job, const process_t *p); #ifdef HAVE__PROC_SELF_STAT /// Use the procfs filesystem to look up how many jiffies of cpu time was used by this process. This