From aeabc76b2e8a40473f4db19ef835e619ee1de8ff Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Thu, 13 May 2021 12:11:00 -0700 Subject: [PATCH] Use internal job ids in builtin_wait This avoids any potential issues due to recycled job IDs. No user visible change. --- src/builtin_jobs.cpp | 6 +++--- src/builtin_wait.cpp | 33 ++++++++++++++++++--------------- src/parser.cpp | 6 +++--- src/parser.h | 6 ++++-- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/src/builtin_jobs.cpp b/src/builtin_jobs.cpp index 6f9f86d56..556597e6b 100644 --- a/src/builtin_jobs.cpp +++ b/src/builtin_jobs.cpp @@ -196,12 +196,12 @@ maybe_t builtin_jobs(parser_t &parser, io_streams_t &streams, const wchar_t if (argv[i][0] == L'%') { int job_id = fish_wcstoi(argv[i] + 1); - if (errno || job_id < -1) { - streams.err.append_format(_(L"%ls: '%ls' is not a valid job id"), cmd, + if (errno || job_id < 0) { + streams.err.append_format(_(L"%ls: '%ls' is not a valid job id\n"), cmd, argv[i]); return STATUS_INVALID_ARGS; } - j = parser.job_get(job_id); + j = parser.job_with_id(job_id); } else { int pid = fish_wcstoi(argv[i]); if (errno || pid < 0) { diff --git a/src/builtin_wait.cpp b/src/builtin_wait.cpp index e8a8a68fe..ee827f8ca 100644 --- a/src/builtin_wait.cpp +++ b/src/builtin_wait.cpp @@ -17,15 +17,15 @@ /// Return the job id to which the process with pid belongs. /// 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, const parser_t &parser) { +static internal_job_id_t get_job_id_from_pid(pid_t pid, const parser_t &parser) { for (const auto &j : parser.jobs()) { if (j->get_pgid() == maybe_t{pid}) { - return j->job_id(); + return j->internal_job_id; } // Check if the specified pid is a child process of the job. for (const process_ptr_t &p : j->processes) { if (p->pid == pid) { - return j->job_id(); + return j->internal_job_id; } } } @@ -76,9 +76,10 @@ static int wait_for_backgrounds(parser_t &parser, bool any_flag) { return 0; } -static bool all_specified_jobs_finished(const parser_t &parser, const std::vector &ids) { +static bool all_specified_jobs_finished(const parser_t &parser, + const std::vector &ids) { for (auto id : ids) { - if (const job_t *j = parser.job_get(id)) { + if (const job_t *j = parser.job_with_internal_id(id)) { // If any specified job is not completed, return false. // If there are stopped jobs, they are ignored. if (j->is_constructed() && !j->is_completed() && !j->is_stopped()) { @@ -89,9 +90,10 @@ static bool all_specified_jobs_finished(const parser_t &parser, const std::vecto return true; } -static bool any_specified_jobs_finished(const parser_t &parser, const std::vector &ids) { +static bool any_specified_jobs_finished(const parser_t &parser, + const std::vector &ids) { for (auto id : ids) { - if (const job_t *j = parser.job_get(id)) { + if (const job_t *j = parser.job_with_internal_id(id)) { // If any specified job is completed, return true. if (j->is_constructed() && (j->is_completed() || j->is_stopped())) { return true; @@ -104,7 +106,8 @@ static bool any_specified_jobs_finished(const parser_t &parser, const std::vecto return false; } -static int wait_for_backgrounds_specified(parser_t &parser, const std::vector &ids, +static int wait_for_backgrounds_specified(parser_t &parser, + const std::vector &ids, bool any_flag) { sigchecker_t sigint(topic_t::sighupint); while ((!any_flag && !all_specified_jobs_finished(parser, ids)) || @@ -138,7 +141,7 @@ 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 &ids, +static bool find_job_by_name(const wchar_t *proc, std::vector &ids, const parser_t &parser) { bool found = false; @@ -146,9 +149,9 @@ static bool find_job_by_name(const wchar_t *proc, std::vector &ids, if (j->command().empty()) continue; if (match_pid(j->command(), proc)) { - if (!contains(ids, j->job_id())) { + if (!contains(ids, j->internal_job_id)) { // If pids doesn't already have the pgid, add it. - ids.push_back(j->job_id()); + ids.push_back(j->internal_job_id); } found = true; } @@ -158,9 +161,9 @@ static bool find_job_by_name(const wchar_t *proc, std::vector &ids, if (p->actual_cmd.empty()) continue; if (match_pid(p->actual_cmd, proc)) { - if (!contains(ids, j->job_id())) { + if (!contains(ids, j->internal_job_id)) { // If pids doesn't already have the pgid, add it. - ids.push_back(j->job_id()); + ids.push_back(j->internal_job_id); } found = true; } @@ -216,7 +219,7 @@ maybe_t builtin_wait(parser_t &parser, io_streams_t &streams, const wchar_t retval = wait_for_backgrounds(parser, any_flag); } else { // jobs specified - std::vector waited_job_ids; + std::vector waited_job_ids; for (int i = w.woptind; i < argc; i++) { if (iswnumeric(argv[i])) { @@ -227,7 +230,7 @@ maybe_t builtin_wait(parser_t &parser, io_streams_t &streams, const wchar_t argv[i]); continue; } - if (job_id_t id = get_job_id_from_pid(pid, parser)) { + if (internal_job_id_t id = get_job_id_from_pid(pid, parser)) { waited_job_ids.push_back(id); } else { streams.err.append_format( diff --git a/src/parser.cpp b/src/parser.cpp index 1bc2271b2..a4ee68509 100644 --- a/src/parser.cpp +++ b/src/parser.cpp @@ -566,16 +566,16 @@ void parser_t::job_promote(job_t *job) { std::rotate(job_list.begin(), loc, std::next(loc)); } -job_t *parser_t::job_get(job_id_t id) { +const job_t *parser_t::job_with_id(job_id_t id) const { for (const auto &job : job_list) { if (id <= 0 || job->job_id() == id) return job.get(); } return nullptr; } -const job_t *parser_t::job_get(job_id_t id) const { +const job_t *parser_t::job_with_internal_id(internal_job_id_t id) const { for (const auto &job : job_list) { - if (id <= 0 || job->job_id() == id) return job.get(); + if (job->internal_job_id == id) return job.get(); } return nullptr; } diff --git a/src/parser.h b/src/parser.h index c93aa5ccd..9bc8748bf 100644 --- a/src/parser.h +++ b/src/parser.h @@ -389,8 +389,10 @@ class parser_t : public std::enable_shared_from_this { void job_promote(job_t *job); /// Return the job with the specified job id. If id is 0 or less, return the last job used. - job_t *job_get(job_id_t job_id); - const job_t *job_get(job_id_t job_id) const; + const job_t *job_with_id(job_id_t job_id) const; + + /// Return the job with the specified internal job id. + const job_t *job_with_internal_id(internal_job_id_t job_id) const; /// Returns the job with the given pid. job_t *job_get_from_pid(pid_t pid) const;