Mirrors/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell synced 2025-01-13 21:44:16 +00:00
Code Issues Projects Releases Packages Wiki Activity

Remove abstractions around job list

Browse source 
Directly access the job list without the intermediate job_iterator_t,
and remove functions that are ripe for abuse by modifying a local
enumeration of the same list instead of operating on the iterators
directly (e.g. proc.cpp iterates jobs, and mid-iteration calls
parser::job_remove(j) with the job (and not the iterator to the job),
causing an invisible invalidation of the pre-existing local iterators.
This commit is contained in:
Mahmoud Al-Qudsi 2018-12-30 21:25:16 -06:00
parent 0c5015d467
commit f8e0e0ef82
11 changed files with 87 additions and 150 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
src/builtin_bg.cpp
View file

@ -51,19 +51,19 @@ int builtin_bg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
if (optind == argc) {
// No jobs were specified so use the most recent (i.e., last) job.
job_t *j;
job_iterator_t jobs;
while ((j = jobs.next())) {
job_t *job = nullptr;
for (auto j : jobs()) {
if (j->is_stopped() && j->get_flag(job_flag_t::JOB_CONTROL) && (!j->is_completed())) {
job = j.get();
break;
}
}
if (!j) {
if (!job) {
streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd);
retval = STATUS_CMD_ERROR;
} else {
retval = send_to_bg(parser, streams, j);
retval = send_to_bg(parser, streams, job);
}
return retval;

24
src/builtin_disown.cpp
View file

@ -31,13 +31,17 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream
streams.err.append_format(fmt, cmd, j->job_id, j->command_wcstr());
}
pid_t pgid = j->pgid;
if (!parser.job_remove(j)) {
return STATUS_CMD_ERROR;
for (auto itr = jobs().begin(); itr != jobs().end(); ++itr) {
auto job = itr->get();
if (job == j) {
pid_t pgid = j->pgid;
add_disowned_pgid(pgid);
jobs().erase(itr);
return STATUS_CMD_OK;
}
}
add_disowned_pgid(pgid);
return STATUS_CMD_OK;
return STATUS_CMD_ERROR;
}
/// Builtin for removing jobs from the job list.
@ -56,20 +60,20 @@ int builtin_disown(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
}
if (argv[1] == 0) {
job_t *j;
// Select last constructed job (ie first job in the job queue) that is possible to disown.
// Stopped jobs can be disowned (they will be continued).
// Foreground jobs can be disowned.
// Even jobs that aren't under job control can be disowned!
job_iterator_t jobs;
while ((j = jobs.next())) {
job_t *job = nullptr;
for (auto j : jobs()) {
if (j->is_constructed() && (!j->is_completed())) {
job = j.get();
break;
}
}
if (j) {
retval = disown_job(cmd, parser, streams, j);
if (job) {
retval = disown_job(cmd, parser, streams, job);
} else {
streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd);
retval = STATUS_CMD_ERROR;

44
src/builtin_fg.cpp
View file

@ -33,20 +33,20 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
return STATUS_CMD_OK;
}
job_t *j = NULL;
job_t *job = nullptr;
if (optind == argc) {
// Select last constructed job (I.e. first job in the job que) that is possible to put in
// the foreground.
job_iterator_t jobs;
while ((j = jobs.next())) {
for (auto j : jobs()) {
if (j->is_constructed() && (!j->is_completed()) &&
((j->is_stopped() || (!j->is_foreground())) &&
j->get_flag(job_flag_t::JOB_CONTROL))) {
job = j.get();
break;
}
}
if (!j) {
if (!job) {
streams.err.append_format(_(L"%ls: There are no suitable jobs\n"), cmd);
}
} else if (optind + 1 < argc) {
@ -58,8 +58,8 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
pid = fish_wcstoi(argv[optind]);
if (!(errno || pid < 0)) {
j = job_t::from_pid(pid);
if (j) found_job = true;
job = job_t::from_pid(pid);
if (job) found_job = true;
}
if (found_job) {
@ -70,46 +70,46 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
builtin_print_error_trailer(parser, streams.err, cmd);
j = 0;
job = 0;
} else {
int pid = abs(fish_wcstoi(argv[optind]));
if (errno) {
streams.err.append_format(BUILTIN_ERR_NOT_NUMBER, cmd, argv[optind]);
builtin_print_error_trailer(parser, streams.err, cmd);
} else {
j = job_t::from_pid(pid);
if (!j || !j->is_constructed() || j->is_completed()) {
job = job_t::from_pid(pid);
if (!job || !job->is_constructed() || job->is_completed()) {
streams.err.append_format(_(L"%ls: No suitable job: %d\n"), cmd, pid);
j = 0;
} else if (!j->get_flag(job_flag_t::JOB_CONTROL)) {
job = nullptr;
} else if (!job->get_flag(job_flag_t::JOB_CONTROL)) {
streams.err.append_format(_(L"%ls: Can't put job %d, '%ls' to foreground because "
L"it is not under job control\n"),
cmd, pid, j->command_wcstr());
j = 0;
cmd, pid, job->command_wcstr());
job = 0;
}
}
}
if (!j) {
if (!job) {
return STATUS_INVALID_ARGS;
}
if (streams.err_is_redirected) {
streams.err.append_format(FG_MSG, j->job_id, j->command_wcstr());
streams.err.append_format(FG_MSG, job->job_id, job->command_wcstr());
} else {
// If we aren't redirecting, send output to real stderr, since stuff in sb_err won't get
// printed until the command finishes.
std::fwprintf(stderr, FG_MSG, j->job_id, j->command_wcstr());
std::fwprintf(stderr, FG_MSG, job->job_id, job->command_wcstr());
}
const wcstring ft = tok_first(j->command());
const wcstring ft = tok_first(job->command());
//For compatibility with fish 2.0's $_, now replaced with `status current-command`
if (!ft.empty()) parser.vars().set_one(L"_", ENV_EXPORT, ft);
reader_write_title(j->command());
reader_write_title(job->command());
j->promote();
j->set_flag(job_flag_t::FOREGROUND, true);
job->promote();
job->set_flag(job_flag_t::FOREGROUND, true);
j->continue_job(j->is_stopped());
job->continue_job(job->is_stopped());
return STATUS_CMD_OK;
}

12
src/builtin_jobs.cpp
View file

@ -174,11 +174,9 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
if (print_last) {
// Ignore unconstructed jobs, i.e. ourself.
job_iterator_t jobs;
const job_t *j;
while ((j = jobs.next())) {
for (auto j : jobs()) {
if (j->is_constructed() && !j->is_completed()) {
builtin_jobs_print(j, mode, !streams.out_is_redirected, streams);
builtin_jobs_print(j.get(), mode, !streams.out_is_redirected, streams);
return STATUS_CMD_ERROR;
}
}
@ -217,12 +215,10 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
}
}
} else {
job_iterator_t jobs;
const job_t *j;
while ((j = jobs.next())) {
for (auto j : jobs()) {
// Ignore unconstructed jobs, i.e. ourself.
if (j->is_constructed() && !j->is_completed()) {
builtin_jobs_print(j, mode, !found && !streams.out_is_redirected, streams);
builtin_jobs_print(j.get(), mode, !found && !streams.out_is_redirected, streams);
found = true;
}
}

20
src/builtin_wait.cpp
View file

@ -16,10 +16,7 @@
/// If a specified process has already finished but the job hasn't, parser_t::job_get_from_pid()
/// doesn't work properly, so use this function in wait command.
static job_id_t get_job_id_from_pid(pid_t pid) {
job_t *j;
job_iterator_t jobs;
while ((j = jobs.next()) != nullptr) {
for (auto j : jobs()) {
if (j->pgid == pid) {
return j->job_id;
}
@ -34,8 +31,7 @@ static job_id_t get_job_id_from_pid(pid_t pid) {
}
static bool all_jobs_finished() {
job_iterator_t jobs;
while (job_t *j = jobs.next()) {
for (auto j : jobs()) {
// If any job is not completed, return false.
// If there are stopped jobs, they are ignored.
if (j->is_constructed() && !j->is_completed() && !j->is_stopped()) {
@ -46,14 +42,13 @@ static bool all_jobs_finished() {
}
static bool any_jobs_finished(size_t jobs_len) {
job_iterator_t jobs;
bool no_jobs_running = true;
// If any job is removed from list, return true.
if (jobs_len != jobs.count()) {
if (jobs_len != jobs().size()) {
return true;
}
while (job_t *j = jobs.next()) {
for (auto j : jobs()) {
// If any job is completed, return true.
if (j->is_constructed() && (j->is_completed() || j->is_stopped())) {
return true;
@ -70,8 +65,7 @@ static bool any_jobs_finished(size_t jobs_len) {
}
static int wait_for_backgrounds(bool any_flag) {
job_iterator_t jobs;
size_t jobs_len = jobs.count();
size_t jobs_len = jobs().size();
while ((!any_flag && !all_jobs_finished()) || (any_flag && !any_jobs_finished(jobs_len))) {
if (reader_test_interrupted()) {
@ -143,10 +137,9 @@ static bool match_pid(const wcstring &cmd, const wchar_t *proc) {
/// It should search the job list for something matching the given proc.
static bool find_job_by_name(const wchar_t *proc, std::vector<job_id_t> &ids) {
job_iterator_t jobs;
bool found = false;
while (const job_t *j = jobs.next()) {
for (const auto j : jobs()) {
if (j->command_is_empty()) continue;
if (match_pid(j->command(), proc)) {
@ -179,7 +172,6 @@ static bool find_job_by_name(const wchar_t *proc, std::vector<job_id_t> &ids) {
int builtin_wait(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
ASSERT_IS_MAIN_THREAD();
int retval = STATUS_CMD_OK;
job_iterator_t jobs;
const wchar_t *cmd = argv[0];
int argc = builtin_count_args(argv);
bool any_flag = false; // flag for -n option

16
src/parse_execution.cpp
View file

@ -784,10 +784,8 @@ parse_execution_result_t parse_execution_context_t::populate_plain_process(
// Protect against exec with background processes running
static uint32_t last_exec_run_counter = -1;
if (process_type == process_type_t::exec && shell_is_interactive()) {
job_iterator_t jobs;
bool have_bg = false;
const job_t *bg = nullptr;
while ((bg = jobs.next())) {
for (const auto bg : jobs()) {
// The assumption here is that if it is a foreground job,
// it's related to us.
// This stops us from asking if we're doing `exec` inside a function.
@ -1130,6 +1128,16 @@ parse_execution_result_t parse_execution_context_t::populate_job_from_job_node(
return result;
}
static bool remove_job(job_t *job) {
for (auto j = jobs().begin(); j != jobs().end(); ++j) {
if (j->get() == job) {
jobs().erase(j);
return true;
}
}
return false;
}
parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_node,
const block_t *associated_block) {
if (should_cancel_execution(associated_block)) {
@ -1253,7 +1261,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
// Actually execute the job.
if (!exec_job(*this->parser, job)) {
parser->job_remove(job.get());
remove_job(job.get());
}
// Only external commands require a new fishd barrier.

20
src/parser.cpp
View file

@ -570,19 +570,6 @@ void parser_t::job_add(shared_ptr<job_t> job) {
this->my_job_list.push_front(std::move(job));
}
bool parser_t::job_remove(job_t *job) {
for (auto iter = my_job_list.begin(); iter != my_job_list.end(); ++iter) {
if (iter->get() == job) {
my_job_list.erase(iter);
return true;
}
}
debug(1, _(L"Job inconsistency"));
sanity_lose();
return false;
}
void parser_t::job_promote(job_t *job) {
job_list_t::iterator loc;
for (loc = my_job_list.begin(); loc != my_job_list.end(); ++loc) {
@ -604,20 +591,17 @@ job_t *parser_t::job_get(job_id_t id) {
}
job_t *parser_t::job_get_from_pid(pid_t pid) const {
job_iterator_t jobs;
job_t *job;
pid_t pgid = getpgid(pid);
if (pgid == -1) {
return 0;
}
while ((job = jobs.next())) {
for (auto job : jobs()) {
if (job->pgid == pgid) {
for (const process_ptr_t &p : job->processes) {
if (p->pid == pid) {
return job;
return job.get();
}
}
}

3
src/parser.h
View file

@ -288,9 +288,6 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
/// Return the function name for the specified stack frame. Default is one (current frame).
const wchar_t *get_function_name(int level = 1);
/// Removes a job.
bool job_remove(job_t *job);
/// Promotes a job to the front of the list.
void job_promote(job_t *job);

59
src/proc.cpp
View file

@ -59,20 +59,11 @@ bool job_list_is_empty() {
return parser_t::principal_parser().job_list().empty();
}
void job_iterator_t::reset() {
this->current = job_list->begin();
this->end = job_list->end();
}
job_iterator_t::job_iterator_t(job_list_t &jobs) : job_list(&jobs) { this->reset(); }
job_iterator_t::job_iterator_t() : job_list(&parser_t::principal_parser().job_list()) {
job_list_t& jobs() {
ASSERT_IS_MAIN_THREAD();
this->reset();
return parser_t::principal_parser().job_list();
}
size_t job_iterator_t::count() const { return this->job_list->size(); }
bool is_interactive_session = false;
bool is_subshell = false;
bool is_block = false;
@ -110,23 +101,15 @@ static std::vector<int> interactive_stack;
void proc_init() { proc_push_interactive(0); }
/// Remove job from list of jobs.
static int job_remove(job_t *j) {
ASSERT_IS_MAIN_THREAD();
return parser_t::principal_parser().job_remove(j);
}
void job_t::promote() {
ASSERT_IS_MAIN_THREAD();
parser_t::principal_parser().job_promote(this);
}
void proc_destroy() {
job_list_t &jobs = parser_t::principal_parser().job_list();
while (!jobs.empty()) {
job_t *job = jobs.front().get();
debug(2, L"freeing leaked job %ls", job->command_wcstr());
job_remove(job);
for (auto job = jobs().begin(); job != jobs().end(); ++job) {
debug(2, L"freeing leaked job %ls", (*job)->command_wcstr());
job = jobs().erase(job);
}
}
@ -364,8 +347,7 @@ static void process_mark_finished_children(bool block_ok) {
topic_set_t reaptopics{};
generation_list_t gens{};
gens.fill(invalid_generation);
job_iterator_t jobs;
while (auto *j = jobs.next()) {
for (const auto j: jobs()) {
for (const auto &proc : j->processes) {
if (auto mtopic = j->reap_topic_for_process(proc.get())) {
topic_t topic = *mtopic;
@ -390,8 +372,7 @@ static void process_mark_finished_children(bool block_ok) {
// We got some changes. Since we last checked we received SIGCHLD, and or HUP/INT.
// Update the hup/int generations and reap any reapable processes.
jobs.reset();
while (auto *j = jobs.next()) {
for (const auto j : jobs()) {
for (const auto &proc : j->processes) {
if (auto mtopic = j->reap_topic_for_process(proc.get())) {
// Update the signal hup/int gen.
@ -484,7 +465,7 @@ static bool process_clean_after_marking(bool allow_interactive) {
// avoid infinite recursion).
static bool locked = false;
if (locked) {
return 0;
return false;
}
locked = true;
@ -492,9 +473,10 @@ static bool process_clean_after_marking(bool allow_interactive) {
// don't try to print in that case (#3222)
const bool interactive = allow_interactive && cur_term != NULL;
job_iterator_t jobs;
const bool only_one_job = jobs.count() == 1;
while (job_t *const j = jobs.next()) {
bool erased = false;
const bool only_one_job = jobs().size() == 1;
for (auto itr = jobs().begin(); itr != jobs().end(); itr = (erased ? itr : (std::advance(itr, 1), itr)), erased = false) {
job_t *j = itr->get();
// If we are reaping only jobs who do not need status messages sent to the console, do not
// consider reaping jobs that need status messages.
if ((!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) && (!interactive) &&
@ -587,7 +569,8 @@ static bool process_clean_after_marking(bool allow_interactive) {
}
proc_fire_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id, 0);
job_remove(j);
itr = jobs().erase(itr);
erased = true;
} else if (j->is_stopped() && !j->get_flag(job_flag_t::NOTIFIED)) {
// Notify the user about newly stopped jobs.
if (!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) {
@ -663,10 +646,7 @@ unsigned long proc_get_jiffies(process_t *p) {
/// Update the CPU time for all jobs.
void proc_update_jiffies() {
job_t *job;
job_iterator_t j;
for (job = j.next(); job; job = j.next()) {
for (auto job : jobs()) {
for (process_ptr_t &p : job->processes) {
gettimeofday(&p->last_time, 0);
p->last_jiffies = proc_get_jiffies(p.get());
@ -901,8 +881,7 @@ void job_t::continue_job(bool send_sigcont) {
void proc_sanity_check() {
const job_t *fg_job = NULL;
job_iterator_t jobs;
while (const job_t *j = jobs.next()) {
for (const auto j : jobs()) {
if (!j->is_constructed()) continue;
// More than one foreground job?
@ -912,7 +891,7 @@ void proc_sanity_check() {
fg_job->command_wcstr(), j->command_wcstr());
sanity_lose();
}
fg_job = j;
fg_job = j.get();
}
for (const process_ptr_t &p : j->processes) {
@ -959,9 +938,7 @@ void proc_wait_any() {
}
void hup_background_jobs() {
job_iterator_t jobs;
while (job_t *j = jobs.next()) {
for (auto j : jobs()) {
// Make sure we don't try to SIGHUP the calling builtin
if (j->pgid == INVALID_PID || !j->get_flag(job_flag_t::JOB_CONTROL)) {
continue;

23
src/proc.h
View file

@ -456,27 +456,8 @@ typedef std::list<shared_ptr<job_t>> job_list_t;
bool job_list_is_empty(void);
/// A class to aid iteration over jobs list
class job_iterator_t {
job_list_t *const job_list;
job_list_t::iterator current, end;
public:
void reset(void);
job_t *next() {
job_t *job = NULL;
if (current != end) {
job = current->get();
++current;
}
return job;
}
explicit job_iterator_t(job_list_t &jobs);
job_iterator_t();
size_t count() const;
};
/// A helper function to more easily access the job list
job_list_t &jobs();
/// Whether a universal variable barrier roundtrip has already been made for the currently executing
/// command. Such a roundtrip only needs to be done once on a given command, unless a universal

6
src/reader.cpp
View file

@ -2229,8 +2229,7 @@ void reader_bg_job_warning() {
std::fputws(_(L"There are still jobs active:\n"), stdout);
std::fputws(_(L"\n PID Command\n"), stdout);
job_iterator_t jobs;
while (job_t *j = jobs.next()) {
for (auto j : jobs()) {
if (!j->is_completed()) {
std::fwprintf(stdout, L"%6d %ls\n", j->processes[0]->pid, j->command_wcstr());
}
@ -2254,8 +2253,7 @@ static void handle_end_loop() {
}
bool bg_jobs = false;
job_iterator_t jobs;
while (const job_t *j = jobs.next()) {
for (const auto j : jobs()) {
if (!j->is_completed()) {
bg_jobs = true;
break;