-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kill: fix the fail to use only least significant bits to identify signal with -l #7225
Conversation
With this pull request the |
GNU testsuite comparison:
|
src/uu/kill/src/kill.rs
Outdated
@@ -164,13 +164,19 @@ fn table() { | |||
} | |||
|
|||
fn print_signal(signal_name_or_value: &str) -> UResult<()> { | |||
let lower_5_bits = |x: usize| x & 0b11111; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this magic number :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add this feature in this pull request or should I open a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would create a new PR. There is also a ticket for it: #6218
You can update utils/why-error.md but no worries if you don't |
tests/by-util/test_kill.rs
Outdated
.arg("-l") | ||
.arg("143") | ||
.succeeds() | ||
.stdout_matches(&Regex::new("TERM").unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I have just seen the function stdout_only
, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's also fine.
I tried to come up with a solution, take for example the signal EXIT ( |
I think you are close :) It looks like it's
|
is it a bug or a feature ? |
@sylvestre I think it's a feature. This is the relevant part of the corresponding GNU # 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 |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
Well done and thanks :) |
Thanks for your patience...it was a journey :) |
only two days, kudos :) |
This pull request solves the issue number #7218.
Fixes #7218