diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 31e32217b..4d7a65966 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -28,6 +28,7 @@ Scripting improvements - fish gained a ``--no-config`` option to disable reading the configuration. This applies both to the user's and the admin's config.fish (typically in /etc/fish/config.fish) and snippets and also sets $fish_function_path to just the functions shipped with fish and disables universal variables and history. (:issue:`7921`, :issue:`1256`). - When universal variables are unavailable for some reason, setting a universal variable now sets a global variable instead (:issue:`7921`). - ``process-exit`` event handlers now receive the same value as ``$status`` in all cases, instead of receiving -1 when the exit was due to a signal. +- ``process-exit`` event handlers for pid 0 also received ``JOB_EXIT`` events; this has been fixed. Interactive improvements ------------------------- diff --git a/src/builtin_function.cpp b/src/builtin_function.cpp index a24b0399e..d72d77818 100644 --- a/src/builtin_function.cpp +++ b/src/builtin_function.cpp @@ -113,7 +113,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig e.type = event_type_t::caller_exit; e.param1.caller_id = caller_id; } else if ((opt == 'p') && (wcscasecmp(w.woptarg, L"%self") == 0)) { - e.type = event_type_t::exit; + e.type = event_type_t::process_exit; e.param1.pid = getpid(); } else { pid_t pid = fish_wcstoi(w.woptarg); @@ -122,9 +122,13 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind, //!OCLINT(hig w.woptarg); return STATUS_INVALID_ARGS; } - - e.type = event_type_t::exit; - e.param1.pid = (opt == 'j' ? -1 : 1) * abs(pid); + if (opt == 'p') { + e.type = event_type_t::process_exit; + e.param1.pid = pid; + } else { + e.type = event_type_t::job_exit; + e.param1.pgid = pid; + } } opts.events.push_back(e); break; @@ -277,13 +281,14 @@ maybe_t builtin_function(parser_t &parser, io_streams_t &streams, // If there is an --on-process-exit or --on-job-exit event handler for some pid, and that // process has already exited, run it immediately (#7210). for (const event_description_t &ed : opts.events) { - if (ed.type == event_type_t::exit && ed.param1.pid != EVENT_ANY_PID) { - wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(abs(ed.param1.pid)); + if ((ed.type == event_type_t::process_exit || ed.type == event_type_t::job_exit) && + ed.param1.pid != EVENT_ANY_PID) { + wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(ed.param1.pid); if (wh && wh->completed) { - if (ed.param1.pid > 0) { + if (ed.type == event_type_t::process_exit) { event_fire(parser, event_t::process_exit(ed.param1.pid, wh->status)); } else { - event_fire(parser, event_t::job_exit(ed.param1.pid)); + event_fire(parser, event_t::job_exit(ed.param1.pgid)); } } } diff --git a/src/builtin_functions.cpp b/src/builtin_functions.cpp index c6663424d..ef553d2dc 100644 --- a/src/builtin_functions.cpp +++ b/src/builtin_functions.cpp @@ -189,6 +189,15 @@ static int report_function_metadata(const wchar_t *funcname, bool verbose, io_st return STATUS_CMD_OK; } +/// \return whether a type filter is valid. +static bool type_filter_valid(const wcstring &filter) { + if (filter.empty()) return true; + for (size_t i = 0; event_filter_names[i]; i++) { + if (filter == event_filter_names[i]) return true; + } + return false; +} + /// The functions builtin, used for listing and erasing functions. maybe_t builtin_functions(parser_t &parser, io_streams_t &streams, const wchar_t **argv) { const wchar_t *cmd = argv[0]; @@ -253,15 +262,12 @@ maybe_t builtin_functions(parser_t &parser, io_streams_t &streams, const wc } if (opts.handlers) { - maybe_t type_filter{}; - if (opts.handlers_type) { - type_filter = event_type_for_name(opts.handlers_type); - if (!type_filter) { - streams.err.append_format(_(L"%ls: Expected generic | variable | signal | exit | " - L"job-id for --handlers-type\n"), - cmd); - return STATUS_INVALID_ARGS; - } + wcstring type_filter = opts.handlers_type ? opts.handlers_type : L""; + if (!type_filter_valid(type_filter)) { + streams.err.append_format(_(L"%ls: Expected generic | variable | signal | exit | " + L"job-id for --handlers-type\n"), + cmd); + return STATUS_INVALID_ARGS; } event_print(streams, type_filter); return STATUS_CMD_OK; diff --git a/src/event.cpp b/src/event.cpp index eed1cac33..87003c3e2 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -114,11 +114,16 @@ static bool handler_matches(const event_handler_t &classv, const event_t &instan case event_type_t::variable: { return instance.desc.str_param1 == classv.desc.str_param1; } - case event_type_t::exit: { + case event_type_t::process_exit: { if (classv.desc.param1.pid == EVENT_ANY_PID) return true; only_once = true; return classv.desc.param1.pid == instance.desc.param1.pid; } + case event_type_t::job_exit: { + if (classv.desc.param1.pgid == EVENT_ANY_PID) return true; + only_once = true; + return classv.desc.param1.pgid == instance.desc.param1.pgid; + } case event_type_t::caller_exit: { only_once = true; return classv.desc.param1.caller_id == instance.desc.param1.caller_id; @@ -157,21 +162,18 @@ wcstring event_get_desc(const parser_t &parser, const event_t &evt) { return format_string(_(L"handler for variable '%ls'"), ed.str_param1.c_str()); } - case event_type_t::exit: { - if (ed.param1.pid > 0) { - return format_string(_(L"exit handler for process %d"), ed.param1.pid); + case event_type_t::process_exit: { + return format_string(_(L"exit handler for process %d"), ed.param1.pid); + } + + case event_type_t::job_exit: { + if (job_t *j = parser.job_get_from_pid(-ed.param1.pid)) { + return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(), + j->command_wcstr()); } else { - // In events, PGIDs are stored as negative PIDs - job_t *j = parser.job_get_from_pid(-ed.param1.pid); - if (j) { - return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(), - j->command_wcstr()); - } else { - return format_string(_(L"exit handler for job with process group %d"), - -ed.param1.pid); - } + return format_string(_(L"exit handler for job with process group %d"), + -ed.param1.pgid); } - DIE("Unreachable"); } case event_type_t::caller_exit: { @@ -369,38 +371,52 @@ void event_fire(parser_t &parser, const event_t &event) { } } -/// Mapping between event type to name. -/// Note we don't bother to sort this. -struct event_type_name_t { - event_type_t type; - const wchar_t *name; -}; - -static const event_type_name_t events_mapping[] = {{event_type_t::signal, L"signal"}, - {event_type_t::variable, L"variable"}, - {event_type_t::exit, L"exit"}, - {event_type_t::caller_exit, L"caller-exit"}, - {event_type_t::generic, L"generic"}}; - -maybe_t event_type_for_name(const wcstring &name) { - for (const auto &em : events_mapping) { - if (name == em.name) { - return em.type; - } - } - return none(); -} - static const wchar_t *event_name_for_type(event_type_t type) { - for (const auto &em : events_mapping) { - if (type == em.type) { - return em.name; - } + switch (type) { + case event_type_t::any: + return L"any"; + case event_type_t::signal: + return L"signal"; + case event_type_t::variable: + return L"variable"; + case event_type_t::process_exit: + return L"process-exit"; + case event_type_t::job_exit: + return L"job-exit"; + case event_type_t::caller_exit: + return L"caller-exit"; + case event_type_t::generic: + return L"generic"; } return L""; } -void event_print(io_streams_t &streams, maybe_t type_filter) { +const wchar_t *const event_filter_names[] = {L"signal", L"variable", L"exit", + L"process-exit", L"job-exit", L"caller-exit", + L"generic", nullptr}; + +static bool filter_matches_event(const wcstring &filter, event_type_t type) { + if (filter.empty()) return true; + switch (type) { + case event_type_t::any: + return false; + case event_type_t::signal: + return filter == L"signal"; + case event_type_t::variable: + return filter == L"variable"; + case event_type_t::process_exit: + return filter == L"process-exit" || filter == L"exit"; + case event_type_t::job_exit: + return filter == L"job-exit" || filter == L"exit"; + case event_type_t::caller_exit: + return filter == L"process-exit" || filter == L"exit"; + case event_type_t::generic: + return filter == L"generic"; + } + DIE("Unreachable"); +} + +void event_print(io_streams_t &streams, const wcstring &type_filter) { event_handler_list_t tmp = *s_event_handlers.acquire(); std::sort(tmp.begin(), tmp.end(), [](const shared_ptr &e1, const shared_ptr &e2) { @@ -412,8 +428,10 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { switch (d1.type) { case event_type_t::signal: return d1.signal < d2.signal; - case event_type_t::exit: + case event_type_t::process_exit: return d1.param1.pid < d2.param1.pid; + case event_type_t::job_exit: + return d1.param1.pgid < d2.param1.pgid; case event_type_t::caller_exit: return d1.param1.caller_id < d2.param1.caller_id; case event_type_t::variable: @@ -427,7 +445,7 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { maybe_t last_type{}; for (const shared_ptr &evt : tmp) { // If we have a filter, skip events that don't match. - if (type_filter && *type_filter != evt->desc.type) { + if (!filter_matches_event(type_filter, evt->desc.type)) { continue; } @@ -441,7 +459,8 @@ void event_print(io_streams_t &streams, maybe_t type_filter) { streams.out.append_format(L"%ls %ls\n", sig2wcs(evt->desc.param1.signal), evt->function_name.c_str()); break; - case event_type_t::exit: + case event_type_t::process_exit: + case event_type_t::job_exit: break; case event_type_t::caller_exit: streams.out.append_format(L"caller-exit %ls\n", evt->function_name.c_str()); @@ -497,7 +516,7 @@ event_t event_t::variable(wcstring name, wcstring_list_t args) { // static event_t event_t::process_exit(pid_t pid, int status) { - event_t evt{event_type_t::exit}; + event_t evt{event_type_t::process_exit}; evt.desc.param1.pid = pid; evt.arguments.reserve(3); evt.arguments.push_back(L"PROCESS_EXIT"); @@ -508,8 +527,8 @@ event_t event_t::process_exit(pid_t pid, int status) { // static event_t event_t::job_exit(pid_t pgid) { - event_t evt{event_type_t::exit}; - evt.desc.param1.pid = pgid; + event_t evt{event_type_t::job_exit}; + evt.desc.param1.pgid = pgid; evt.arguments.reserve(3); evt.arguments.push_back(L"JOB_EXIT"); evt.arguments.push_back(to_string(pgid)); diff --git a/src/event.h b/src/event.h index b9533bba7..2e2319fd4 100644 --- a/src/event.h +++ b/src/event.h @@ -27,14 +27,20 @@ enum class event_type_t { signal, /// An event triggered by a variable update. variable, - /// An event triggered by a job or process exit. - exit, + /// An event triggered by a process exit. + process_exit, + /// An event triggered by a job exit. + job_exit, /// An event triggered by a job exit, triggering the 'caller'-style events only. caller_exit, /// A generic event. generic, }; +/// Null-terminated list of valid event filter names. +/// These are what are valid to pass to 'functions --handlers-type' +extern const wchar_t *const event_filter_names[]; + /// Properties of an event. struct event_description_t { /// The event type. @@ -43,13 +49,14 @@ struct event_description_t { /// The type-specific parameter. The int types are one of the following: /// /// signal: Signal number for signal-type events.Use EVENT_ANY_SIGNAL to match any signal - /// pid: Process id for process-type events. Use EVENT_ANY_PID to match any pid. (Negative - /// values are used for PGIDs). + /// pid: Process id for process-type events. Use EVENT_ANY_PID to match any pid. + /// pgid: Process id for job-type events. This value is positive (or EVENT_ANY_PID). /// caller_id: Internal job id for caller_exit type events union { int signal; - uint64_t caller_id; pid_t pid; + pid_t pgid; + uint64_t caller_id; } param1{}; /// The string types are one of the following: @@ -94,7 +101,7 @@ struct event_t { /// Create a PROCESS_EXIT event. static event_t process_exit(pid_t pid, int status); - /// Create a JOB_EXIT event. The pgid should be negative. + /// Create a JOB_EXIT event. The pgid should be positive. /// The reported status is always 0 for historical reasons. static event_t job_exit(pid_t pgid); @@ -126,8 +133,8 @@ void event_fire_delayed(parser_t &parser); /// Enqueue a signal event. Invoked from a signal handler. void event_enqueue_signal(int signal); -/// Print all events. If type_filter is not none(), only output events with that type. -void event_print(io_streams_t &streams, maybe_t type_filter); +/// Print all events. If type_filter is not empty, only output events with that type. +void event_print(io_streams_t &streams, const wcstring &type_filter); /// Returns a string describing the specified event. wcstring event_get_desc(const parser_t &parser, const event_t &e); @@ -136,7 +143,4 @@ wcstring event_get_desc(const parser_t &parser, const event_t &e); void event_fire_generic(parser_t &parser, const wchar_t *name, const wcstring_list_t *args = nullptr); -/// Return the event type for a given name, or none. -maybe_t event_type_for_name(const wcstring &name); - #endif diff --git a/src/function.cpp b/src/function.cpp index 424f8f386..10db56869 100644 --- a/src/function.cpp +++ b/src/function.cpp @@ -383,11 +383,12 @@ wcstring functions_def(const wcstring &name) { append_format(out, L" --on-variable %ls", d.str_param1.c_str()); break; } - case event_type_t::exit: { - if (d.param1.pid > 0) - append_format(out, L" --on-process-exit %d", d.param1.pid); - else - append_format(out, L" --on-job-exit %d", -d.param1.pid); + case event_type_t::process_exit: { + append_format(out, L" --on-process-exit %d", d.param1.pid); + break; + } + case event_type_t::job_exit: { + append_format(out, L" --on-job-exit %d", d.param1.pgid); break; } case event_type_t::caller_exit: { diff --git a/src/proc.cpp b/src/proc.cpp index 7f06e1233..3dc511a81 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -674,7 +674,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive // don't create an event or it's easy to get an infinite loop. if (!j->from_event_handler() && j->should_report_process_exits()) { pid_t pgid = *j->get_pgid(); - exit_events.push_back(event_t::job_exit(-pgid)); + exit_events.push_back(event_t::job_exit(pgid)); } // Caller exit events we still create, which anecdotally fixes `source (thing | psub)` // inside event handlers. This seems benign since this event is barely used (basically diff --git a/tests/checks/signal.fish b/tests/checks/signal.fish index d332f690a..beeecba97 100644 --- a/tests/checks/signal.fish +++ b/tests/checks/signal.fish @@ -28,6 +28,11 @@ function anychild --on-process-exit 0 echo $argv[1] $argv[3] end +function anyjob --on-job-exit 0 + # Type and exit status + echo $argv[1] $argv[3] +end + echo "command false:" command false # CHECK: command false: