From 1df4b23cff81a8818ba4dcea4b0f2629679de77e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20M=C3=BCller?= Date: Tue, 17 Dec 2024 11:40:18 +0100 Subject: [PATCH] Fix `Process::Status` for unknown signals (#15280) Using `Signal.from_value` limits the result to named members of the enum `Signal`. That means it only accepts signals that are explicitly defined in the Crystal bindings. While we generally strive for completeness, there's no guarantee that the operating system might indicate a signal that is not in the bindings. And in fact there are no mappings for real-time signals (`SIGRTMIN`..`SIGRTMAX`), for example (not sure if they're particularly relevant as exit signals, but it shows incompleteness). This mechanism should be fault tolerant and be able to represent _any_ signal value, regardless of whether it's mapped in the Crystal bindings or not. If it's missing we cannot associate a name, but we know it's a signal and can express that in the type `Signal` (enums can take any value, not just those of named members). --- spec/std/process/status_spec.cr | 8 ++++++++ src/enum.cr | 3 ++- src/process/status.cr | 12 +++++++++--- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/spec/std/process/status_spec.cr b/spec/std/process/status_spec.cr index 86529b2cefd4..63136a2ddac7 100644 --- a/spec/std/process/status_spec.cr +++ b/spec/std/process/status_spec.cr @@ -105,6 +105,9 @@ describe Process::Status do Process::Status.new(Signal::INT.value).exit_signal.should eq Signal::INT last_signal = Signal.values[-1] Process::Status.new(last_signal.value).exit_signal.should eq last_signal + + unknown_signal = Signal.new(126) + Process::Status.new(unknown_signal.value).exit_signal.should eq unknown_signal end it "#normal_exit? with signal code" do @@ -249,6 +252,8 @@ describe Process::Status do assert_prints Process::Status.new(Signal::HUP.value).to_s, "HUP" last_signal = Signal.values[-1] assert_prints Process::Status.new(last_signal.value).to_s, last_signal.to_s + + assert_prints Process::Status.new(Signal.new(126).value).to_s, "Signal[126]" end {% end %} end @@ -275,6 +280,9 @@ describe Process::Status do assert_prints Process::Status.new(Signal::HUP.value).inspect, "Process::Status[Signal::HUP]" last_signal = Signal.values[-1] assert_prints Process::Status.new(last_signal.value).inspect, "Process::Status[#{last_signal.inspect}]" + + unknown_signal = Signal.new(126) + assert_prints Process::Status.new(unknown_signal.value).inspect, "Process::Status[Signal[126]]" end {% end %} end diff --git a/src/enum.cr b/src/enum.cr index 0b2214ff34ad..058c17f6ee1c 100644 --- a/src/enum.cr +++ b/src/enum.cr @@ -225,7 +225,8 @@ abstract struct Enum end end - private def member_name + # :nodoc: + def member_name : String? # Can't use `case` here because case with duplicate values do # not compile, but enums can have duplicates (such as `enum Foo; FOO = 1; BAR = 1; end`). {% for member in @type.constants %} diff --git a/src/process/status.cr b/src/process/status.cr index 5fd70e5ad203..28e6049238dc 100644 --- a/src/process/status.cr +++ b/src/process/status.cr @@ -217,7 +217,7 @@ class Process::Status # which also works on Windows. def exit_signal : Signal {% if flag?(:unix) && !flag?(:wasm32) %} - Signal.from_value(signal_code) + Signal.new(signal_code) {% else %} raise NotImplementedError.new("Process::Status#exit_signal") {% end %} @@ -298,7 +298,12 @@ class Process::Status if normal_exit? io << exit_code else - io << exit_signal + signal = exit_signal + if name = signal.member_name + io << name + else + signal.inspect(io) + end end {% end %} end @@ -314,7 +319,8 @@ class Process::Status if normal_exit? exit_code.to_s else - exit_signal.to_s + signal = exit_signal + signal.member_name || signal.inspect end {% end %} end