kill: fix the fail to use only least significant bits to identify signal with -l by Felle33 · Pull Request #7225 · uutils/coreutils

Felle33 · 2025-01-26T23:01:32Z

This pull request solves the issue number #7218.
Fixes #7218

Felle33 · 2025-01-26T23:03:30Z

With this pull request the tests/misc/kill.sh passes. Should I modify some file or is it just fine as it is?

github-actions · 2025-01-26T23:26:49Z

GNU testsuite comparison:

Congrats! The gnu test tests/misc/kill is no longer failing!

sylvestre · 2025-01-27T06:53:49Z

src/uu/kill/src/kill.rs

 }

 fn print_signal(signal_name_or_value: &str) -> UResult<()> {
+    let lower_5_bits = |x: usize| x & 0b11111;


Please document this magic number :)

I think it is not entirely correct, because it recognizes, for example, 111 (= b1101111) as a TERM signal whereas GNU kill doesn't recognize it.

Yes you are right. I have done some tests and the range of number accepted goes from 0 to 64 and from 128 to 192 (both endings included). It is like they are managing the same signals but mapped 2 times.
I noticed also that we don't manage the name of the signals from 32 to 64 (and therefore also from 160 to 192).

I could add this feature in this pull request or should I open a new one?

I would create a new PR. There is also a ticket for it: #6218

sylvestre · 2025-01-27T06:54:45Z

With this pull request the tests/misc/kill.sh passes. Should I modify some file or is it just fine as it is?

You can update utils/why-error.md but no worries if you don't

cakebaker · 2025-01-27T07:56:07Z

tests/by-util/test_kill.rs

+        .arg("-l")
+        .arg("143")
+        .succeeds()
+        .stdout_matches(&Regex::new("TERM").unwrap());


While this works, I think using a regex is a bit overkill and you can simplify it by using either stdout_is or stdout_contains.

Thank you, I have just seen the function stdout_only, what do you think?

Yes, that's also fine.

Felle33 · 2025-01-27T13:17:38Z

I tried to come up with a solution, take for example the signal EXIT (0):
with some tests, I noticed that it appears when the argument is 0 128 256 512 768 1024 and so on; it is repeated with a mod 128.
For this reason, we should only check only the low 7 bits

cakebaker · 2025-01-27T14:31:53Z

I think you are close :) It looks like it's mod 256 for numbers >= 256 and not mod 128:

$ ../gnu/src/kill -l $(seq 0 128 2560)
EXIT
EXIT
EXIT
kill: ‘384’: invalid signal
EXIT
kill: ‘640’: invalid signal
EXIT
kill: ‘896’: invalid signal
EXIT
kill: ‘1152’: invalid signal
EXIT
kill: ‘1408’: invalid signal
EXIT
kill: ‘1664’: invalid signal
EXIT
kill: ‘1920’: invalid signal
EXIT
kill: ‘2176’: invalid signal
EXIT
kill: ‘2432’: invalid signal
EXIT

sylvestre · 2025-01-27T14:33:28Z

is it a bug or a feature ?

cakebaker · 2025-01-27T14:41:02Z

@sylvestre I think it's a feature. This is the relevant part of the corresponding GNU kill test:

# Verify we only consider the lower "signal" bits,
# to support ksh which just adds 256 to the signal value
STD_TERM_STATUS=$(expr "$SIGTERM" + 128)
KSH_TERM_STATUS=$(expr "$SIGTERM" + 256)
test $(env kill -l $STD_TERM_STATUS $KSH_TERM_STATUS | uniq) = TERM || fail=1

github-actions · 2025-01-27T15:05:08Z

GNU testsuite comparison:

Congrats! The gnu test tests/misc/kill is no longer failing!

…ssue

github-actions · 2025-01-27T17:52:33Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/kill is no longer failing!
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

cakebaker · 2025-01-28T09:22:15Z

Congrats! The gnu test tests/misc/kill is no longer failing!

Well done and thanks :)

Felle33 · 2025-01-28T11:23:58Z

Thanks for your patience...it was a journey :)

sylvestre · 2025-01-28T11:27:25Z

only two days, kudos :)

Felle33 added 2 commits January 26, 2025 23:53

kill: check the lower 5 bits when the input is a number

4189f20

test/kill: added testcase

2089a9c

sylvestre reviewed Jan 27, 2025

View reviewed changes

cakebaker reviewed Jan 27, 2025

View reviewed changes

kill: check the last 7 bits

269684d

Merge branch 'main' into kill_issue

cffe7c8

Felle33 added 2 commits January 27, 2025 18:24

kill: check only the last 8 bits and the signals in the range [128, 159]

13b0276

Merge branch 'kill_issue' of github.com:Felle33/coreutils into kill_i…

da74927

…ssue

cakebaker merged commit 1595b6a into uutils:main Jan 28, 2025
65 checks passed

Felle33 deleted the kill_issue branch January 28, 2025 11:22

moonfruit mentioned this pull request Mar 10, 2025

uutils-selected 0.0.30 moonfruit/homebrew-tap#226

Closed

Uh oh!

Conversation

Felle33 commented Jan 26, 2025

Uh oh!

Felle33 commented Jan 26, 2025

Uh oh!

github-actions bot commented Jan 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Jan 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Felle33 commented Jan 27, 2025

Uh oh!

cakebaker commented Jan 27, 2025

Uh oh!

sylvestre commented Jan 27, 2025

Uh oh!

cakebaker commented Jan 27, 2025

Uh oh!

github-actions bot commented Jan 27, 2025

Uh oh!

github-actions bot commented Jan 27, 2025

Uh oh!

Uh oh!

cakebaker commented Jan 28, 2025

Uh oh!

Felle33 commented Jan 28, 2025

Uh oh!

sylvestre commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants