From acdb81bbca0090fd5f13d72fa1c936c2b645fc42 Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Sun, 6 Aug 2017 15:47:01 -0700 Subject: [PATCH] lint and style cleanups --- src/env.h | 2 +- src/exec.cpp | 82 +++++++++++++++++++++++++----------------------- src/expand.h | 2 -- src/postfork.cpp | 22 ++++++------- src/postfork.h | 1 - src/proc.cpp | 66 ++++++++++++++++++++------------------ 6 files changed, 89 insertions(+), 86 deletions(-) diff --git a/src/env.h b/src/env.h index b7f3c2158..b5c8131c5 100644 --- a/src/env.h +++ b/src/env.h @@ -71,8 +71,8 @@ void misc_init(); class env_var_t { private: - bool is_missing; wcstring val; + bool is_missing; public: static env_var_t missing_var() { diff --git a/src/exec.cpp b/src/exec.cpp index 0b18bb634..6f4e705e1 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -37,6 +38,7 @@ #include "postfork.h" #include "proc.h" #include "reader.h" +#include "signal.h" #include "wutil.h" // IWYU pragma: keep /// File descriptor redirection error message. @@ -493,8 +495,8 @@ 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. bool pgrp_set = false; - //this is static since processes can block on input/output across jobs - //the main exec_job loop is only ever run in a single thread, so this is OK + // This is static since processes can block on input/output across jobs the main exec_job loop + // is only ever run in a single thread, so this is OK. static pid_t blocked_pid = -1; int pipe_current_read = -1, pipe_current_write = -1, pipe_next_read = -1; for (std::unique_ptr &unique_p : j->processes) { @@ -513,7 +515,7 @@ void exec_job(parser_t &parser, job_t *j) { // See if we need a pipe. const bool pipes_to_next_command = !p->is_last_in_job; - //set to true if we end up forking for this process + // Set to true if we end up forking for this process. bool child_forked = false; bool child_spawned = false; bool block_child = true; @@ -622,12 +624,12 @@ void exec_job(parser_t &parser, job_t *j) { // a pipeline. shared_ptr block_output_io_buffer; - //used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job can read - //from its output. - auto unblock_previous = [&j] () { - //we've already called waitpid after forking the child, so we've guaranteed that they're - //already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call SIGCONT before - //they've actually stopped, and they'll remain suspended indefinitely. + // Used to SIGCONT the previously SIGSTOP'd process so the main loop or next command in job + // can read from its output. + auto unblock_previous = [&j]() { + // We've already called waitpid after forking the child, so we've guaranteed that + // they're already SIGSTOP'd. Otherwise we'd be risking a deadlock because we can call + // SIGCONT before they've actually stopped, and they'll remain suspended indefinitely. if (blocked_pid != -1) { debug(2, L"Unblocking process %d.\n", blocked_pid); kill(blocked_pid, SIGCONT); @@ -653,7 +655,7 @@ void exec_job(parser_t &parser, job_t *j) { if (block_child) { kill(p->pid, SIGSTOP); } - //the parent will wake us up when we're ready to execute + // The parent will wake us up when we're ready to execute. setup_child_process(p, process_net_io_chain); child_action(); DIE("Child process returned control to do_fork lambda!"); @@ -678,10 +680,9 @@ void exec_job(parser_t &parser, job_t *j) { return true; }; - - //helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure - //an output buffer exists in case there is another command in the job chain that will - //be reading from this command's output. + // Helper routine executed by INTERNAL_FUNCTION and INTERNAL_BLOCK_NODE to make sure an + // output buffer exists in case there is another command in the job chain that will be + // reading from this command's output. auto verify_buffer_output = [&] () { if (!p->is_last_in_job) { // Be careful to handle failure, e.g. too many open fds. @@ -842,8 +843,8 @@ void exec_job(parser_t &parser, job_t *j) { const int fg = j->get_flag(JOB_FOREGROUND); j->set_flag(JOB_FOREGROUND, false); - //main loop may need to perform a blocking read from previous command's output. - //make sure read source is not blocked + // Main loop may need to perform a blocking read from previous command's output. + // Make sure read source is not blocked. unblock_previous(); p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams); @@ -939,8 +940,8 @@ void exec_job(parser_t &parser, job_t *j) { bool must_fork = redirection_is_to_real_file(stdout_io.get()) || redirection_is_to_real_file(stderr_io.get()); if (!must_fork && p->is_last_in_job) { - //we are handling reads directly in the main loop. Make sure source is unblocked. - //Note that we may still end up forking. + // We are handling reads directly in the main loop. Make sure source is + // unblocked. Note that we may still end up forking. unblock_previous(); const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER; const bool no_stdout_output = stdout_buffer.empty(); @@ -1110,18 +1111,19 @@ void exec_job(parser_t &parser, job_t *j) { bool child_blocked = block_child && child_forked; if (child_blocked) { - //we have to wait to ensure the child has set their progress group and is in SIGSTOP state - //otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be blocked indefinitely. - //The child is SIGSTOP'd and is guaranteed to have called child_set_group() at this point. - //but we only need to call set_child_group for the first process in the group. - //If needs_keepalive is set, this has already been called for the keepalive process + // We have to wait to ensure the child has set their progress group and is in SIGSTOP + // state otherwise, we can later call SIGCONT before they call SIGSTOP and they'll be + // blocked indefinitely. The child is SIGSTOP'd and is guaranteed to have called + // child_set_group() at this point. but we only need to call set_child_group for the + // first process in the group. If needs_keepalive is set, this has already been called + // for the keepalive process. pid_t pid_status{}; int result; while ((result = waitpid(p->pid, &pid_status, WUNTRACED)) == -1 && errno == EINTR) { - //This could be a superfluous interrupt or Ctrl+C at the terminal - //In all cases, it is OK to retry since the forking code above is specifically designed - //to never, ever hang/block in a child process before the SIGSTOP call is reached. - continue; + // This could be a superfluous interrupt or Ctrl+C at the terminal In all cases, it + // is OK to retry since the forking code above is specifically designed to never, + // ever hang/block in a child process before the SIGSTOP call is reached. + ; // do nothing } if (result == -1) { exec_error = true; @@ -1129,26 +1131,26 @@ void exec_job(parser_t &parser, job_t *j) { wperror(L"waitpid"); break; } - //we are not unblocking the child via SIGCONT just yet to give the next process a chance to open - //the pipes and join the process group. We only unblock the last process in the job chain because - //no one awaits it. + // We are not unblocking the child via SIGCONT just yet to give the next process a + // chance to open the pipes and join the process group. We only unblock the last process + // in the job chain because no one awaits it. } - //regardless of whether the child blocked or not: - //only once per job, and only once we've actually executed an external command + // Regardless of whether the child blocked or not: only once per job, and only once we've + // actually executed an external command. if ((child_spawned || child_forked) && !pgrp_set) { - //this should be called after waitpid if child_forked && pipes_to_next_command - //it can be called at any time if child_spawned + // This should be called after waitpid if child_forked and pipes_to_next_command it can + // be called at any time if child_spawned. set_child_group(j, p->pid); - //we can't rely on p->is_first_in_job because a builtin may have been the first + // we can't rely on p->is_first_in_job because a builtin may have been the first. pgrp_set = true; } - //if the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock it now. - //this must be after the wait_pid on the process we just started, if any. + // If the command we ran _before_ this one was SIGSTOP'd to let this one catch up, unblock + // it now. this must be after the wait_pid on the process we just started, if any. unblock_previous(); if (child_blocked) { - //store the newly-blocked command's PID so that it can be SIGCONT'd once the next process - //in the chain is started. That may be in this job or in another job. + // Store the newly-blocked command's PID so that it can be SIGCONT'd once the next + // process in the chain is started. That may be in this job or in another job. blocked_pid = p->pid; } @@ -1164,7 +1166,7 @@ void exec_job(parser_t &parser, job_t *j) { pipe_current_write = -1; } - //unblock the last process because there's no need for it to stay SIGSTOP'd for anything + // Unblock the last process because there's no need for it to stay SIGSTOP'd for anything. if (p->is_last_in_job) { unblock_previous(); } diff --git a/src/expand.h b/src/expand.h index 2fd0423bd..6643b8742 100644 --- a/src/expand.h +++ b/src/expand.h @@ -122,8 +122,6 @@ bool expand_one(wcstring &inout_str, expand_flags_t flags, parse_error_list_t *e /// Convert the variable value to a human readable form, i.e. escape things, handle arrays, etc. /// Suitable for pretty-printing. -/// -/// \param in the value to escape wcstring expand_escape_variable(const env_var_t &var); /// Perform tilde expansion and nothing else on the specified string, which is modified in place. diff --git a/src/postfork.cpp b/src/postfork.cpp index 8b79c4f7f..bdf788607 100644 --- a/src/postfork.cpp +++ b/src/postfork.cpp @@ -78,9 +78,9 @@ bool child_set_group(job_t *j, process_t *p) { if (j->pgid == -2) { j->pgid = p->pid; } - //retry on EPERM because there's no way that a child cannot join an existing progress group - //because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few - //times to get the kernel to see the new group. (Linux 4.4.0) + // Retry on EPERM because there's no way that a child cannot join an existing progress group + // because we are SIGSTOPing the previous job in the chain. Sometimes we have to try a few + // times to get the kernel to see the new group. (Linux 4.4.0) int failure = setpgid(p->pid, j->pgid); while (failure == -1 && (errno == EPERM || errno == EINTR)) { debug_safe(4, "Retrying setpgid in child process"); @@ -112,7 +112,7 @@ bool child_set_group(job_t *j, process_t *p) { retval = false; } } else { - //this is probably stays unused in the child + // This is probably stays unused in the child. j->pgid = getpgrp(); } @@ -142,16 +142,16 @@ bool set_child_group(job_t *j, pid_t child_pid) { if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) { //!OCLINT(early exit) if (tcgetpgrp(STDIN_FILENO) == j->pgid) { - //we've already assigned the process group control of the terminal when the first process in the job - //was started. There's no need to do so again, and on some platforms this can cause an EPERM error. - //In addition, if we've given control of the terminal to a process group, attempting to call tcsetpgrp - //from the background will cause SIGTTOU to be sent to everything in our process group (unless we - //handle it).. + // We've already assigned the process group control of the terminal when the first + // process in the job was started. There's no need to do so again, and on some platforms + // this can cause an EPERM error. In addition, if we've given control of the terminal to + // a process group, attempting to call tcsetpgrp from the background will cause SIGTTOU + // to be sent to everything in our process group (unless we handle it). debug(4, L"Process group %d already has control of terminal\n", j->pgid); } else { - //no need to duplicate the code here, a function already exists that does just this + // No need to duplicate the code here, a function already exists that does just this. retval = terminal_give_to_job(j, false /*new job, so not continuing*/); } } @@ -264,7 +264,7 @@ int setup_child_process(process_t *p, const io_chain_t &io_chain) { bool ok = true; if (ok) { - //In the case of IO_FILE, this can hang until data is available to read/write! + // In the case of IO_FILE, this can hang until data is available to read/write! ok = (0 == handle_child_io(io_chain)); if (p != 0 && !ok) { debug_safe(4, "handle_child_io failed in setup_child_process"); diff --git a/src/postfork.h b/src/postfork.h index 4a3b47e4e..f29be4741 100644 --- a/src/postfork.h +++ b/src/postfork.h @@ -27,7 +27,6 @@ bool child_set_group(job_t *j, process_t *p); //called by child /// inside the exec function, which blocks all signals), and all IO redirections and other file /// descriptor actions are performed. /// -/// \param j the job to set up the IO for /// \param p the child process to set up /// \param io_chain the IO chain to use /// diff --git a/src/proc.cpp b/src/proc.cpp index b0ebdf893..3f005b34f 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -791,53 +791,56 @@ bool terminal_give_to_job(job_t *j, int cont) { signal_block(); - //It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no longer - //the controlling process group for the terminal and no longer have permission to set the process - //group that is in control, causing tcsetpgrp to return EPERM, even though that's not the documented - //behavior in tcsetpgrp(3), which instead says other bad things will happen (it says SIGTTOU will be - //sent to all members of the background *calling* process group, but it's more complicated than that, - //SIGTTOU may or may not be sent depending on the TTY configuration and whether or not signal handlers - //for SIGTTOU are installed. Read: http://curiousthing.org/sigttin-sigttou-deep-dive-linux - //In all cases, our goal here was just to hand over control of the terminal to this process group, - //which is a no-op if it's already been done. + // It may not be safe to call tcsetpgrp if we've already done so, as at that point we are no + // longer the controlling process group for the terminal and no longer have permission to set + // the process group that is in control, causing tcsetpgrp to return EPERM, even though that's + // not the documented behavior in tcsetpgrp(3), which instead says other bad things will happen + // (it says SIGTTOU will be sent to all members of the background *calling* process group, but + // it's more complicated than that, SIGTTOU may or may not be sent depending on the TTY + // configuration and whether or not signal handlers for SIGTTOU are installed. Read: + // http://curiousthing.org/sigttin-sigttou-deep-dive-linux In all cases, our goal here was just + // to hand over control of the terminal to this process group, which is a no-op if it's already + // been done. if (tcgetpgrp(STDIN_FILENO) == j->pgid) { debug(2, L"Process group %d already has control of terminal\n", j->pgid); } else { debug(4, L"Attempting to bring process group to foreground via tcsetpgrp for job->pgid %d\n", j->pgid); - //the tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but is not the - //process group ID of a process in the same session as the calling process." - //Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls SIGSTOP, and - //the child was created in the same session as us), it seems that EPERM is being thrown because of an - //caching issue - the call to tcsetpgrp isn't seeing the newly-created process group just yet. On this - //developer's test machine (WSL running Linux 4.4.0), EPERM does indeed disappear on retry. The important - //thing is that we can guarantee the process isn't going to exit while we wait (which would cause us to - //possibly block indefinitely). - + // The tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but + // is not the process group ID of a process in the same session as the calling process." + // Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls + // SIGSTOP, and the child was created in the same session as us), it seems that EPERM is + // being thrown because of an caching issue - the call to tcsetpgrp isn't seeing the + // newly-created process group just yet. On this developer's test machine (WSL running Linux + // 4.4.0), EPERM does indeed disappear on retry. The important thing is that we can + // guarantee the process isn't going to exit while we wait (which would cause us to possibly + // block indefinitely). while (tcsetpgrp(STDIN_FILENO, j->pgid) != 0) { bool pgroup_terminated = false; if (errno == EINTR) { - //always retry on EINTR, see comments in tcsetattr EINTR code below. + ; // Always retry on EINTR, see comments in tcsetattr EINTR code below. } else if (errno == EINVAL) { - //OS X returns EINVAL if the process group no longer lives. Probably other OSes, too. - //Unlike EPERM below, EINVAL can only happen if the process group has terminated + // OS X returns EINVAL if the process group no longer lives. Probably other OSes, + // too. Unlike EPERM below, EINVAL can only happen if the process group has + // terminated. pgroup_terminated = true; } else if (errno == EPERM) { - //retry so long as this isn't because the process group is dead + // Retry so long as this isn't because the process group is dead. int wait_result = waitpid(-1 * j->pgid, &wait_result, WNOHANG); if (wait_result == -1) { - //Note that -1 is technically an "error" for waitpid in the sense that an invalid argument was specified - //because no such process group exists any longer. This is the observed behavior on Linux 4.4.0. - //a "success" result would mean processes from the group still exist but is still running in some state - //or the other. + // Note that -1 is technically an "error" for waitpid in the sense that an + // invalid argument was specified because no such process group exists any + // longer. This is the observed behavior on Linux 4.4.0. a "success" result + // would mean processes from the group still exist but is still running in some + // state or the other. pgroup_terminated = true; } else { - //debug the original tcsetpgrp error (not the waitpid errno) to the log, and then retry until not EPERM - //or the process group has exited. + // Debug the original tcsetpgrp error (not the waitpid errno) to the log, and + // then retry until not EPERM or the process group has exited. debug(2, L"terminal_give_to_job(): EPERM.\n", j->pgid); } } @@ -850,9 +853,10 @@ bool terminal_give_to_job(job_t *j, int cont) { } if (pgroup_terminated) { - //all processes in the process group has exited. Since we force all child procs to SIGSTOP on startup, - //the only way that can happen is if the very last process in the group terminated, and didn't need - //to access the terminal, otherwise it would have hung waiting for terminal IO (SIGTTIN). We can ignore this. + // All processes in the process group has exited. Since we force all child procs to + // SIGSTOP on startup, the only way that can happen is if the very last process in + // the group terminated, and didn't need to access the terminal, otherwise it would + // have hung waiting for terminal IO (SIGTTIN). We can ignore this. debug(3, L"tcsetpgrp called but process group %d has terminated.\n", j->pgid); break; }