8000 env: add program signal messages by ndd7xv · Pull Request #3290 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

env: add program signal messages #3290

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 4, 2022
Merged

env: add program signal messages #3290

merged 1 commit into from
Apr 4, 2022

Conversation

ndd7xv
Copy link
Contributor
@ndd7xv ndd7xv commented Mar 21, 2022

Addresses #3188.

  1. These changes don't print the error output as described in the issue, but they do provide similar information and replicate what I get when encountering a signal termination (x86_64 Fedora 35, GNU coreutils 8.32):
$ env ./a.out
Segmentation fault (core dumped)
$ ~/repo/coreutils/target/debug/env ./a.out 
Segmentation fault (core dumped)
  1. I'm not entirely sure how to test/get code coverage for this; ideas are welcome. For testing my changes, I essentially compared outputs of a c program that an exception, and changed the exception to check for any discrepancies:
#include <signal.h>

int main() {
        raise(SIGSEGV);
}
  1. I use unsafe code and add libc as a dependency in order to convert a signal number into a name. If there's reservations about this, I could instead have output like terminated by signal 11. I try to describe my logic for thinking this is safe in a comment.

@tertsdiepraam
Copy link
Member

Cool! For testing, is it possible to just check whether the output includes that error message?

jfinkels
jfinkels reviewed Mar 21, 2022
View reviewed changes
src/uu/env/src/env.rs Outdated
let signal_code = exit.signal().unwrap();
println!(
"{}{}",
unsafe {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

coreutils/src/uucore/src/lib/features/signals.rs

Line 98 in 5eeac58

pub fn signal_name_by_value(signal_value: usize) -> Option<&'static str> {
works here?

Copy link
Contributor Author

Choose a 8000 reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, thanks! When I was looking into how to test, I came across it and I'm using that now.

However, I'm still not sure how to test, considering what I did before was run a program that raised a signal - I don't know how to raise a signal straight from the command line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kill will send a signal to a process.

@ndd7xv ndd7xv force-pushed the env-signal-messages branch from 8eadf1f to 287cf82 Compare March 22, 2022 03:14
@ndd7xv
Copy link
Contributor Author
ndd7xv commented Mar 22, 2022 •
edited
Loading

Sorry to pester with messages and requests - the test case I've tried creating doesn't seem to be catching any output.

I've added a new commit (that I'll clean up more once it's working) that emulates what I know has worked typing into the terminal.

$ ~/repo/coreutils/target/debug/env sleep 100

<in another terminal>
$ ps -e | grep sleep
$ kill -s SEGV <pid>

<back to original terminal>
"env sleep" terminated by signal SIGSEGV (core dumped)
$ $?
139 <the error number specified in env.rs, 128 + 11>

"env sleep" terminated by signal SIGSEGV (core dumped) does not appear in the test case - only the termination signal is there, and it's not being obtained from my code changes (it's always 139 regardless of what I return as an error in the code). There's nothing on the kill command either. Is there anything glaringly obvious that would prevent output from being captured by the child process?

@sylvestre
Copy link
Contributor

I guess you saw the errors ?

error[E0432]: unresolved import `uucore::signals`
  --> src\uu\env\src/env.rs:31:13
   |
31 | use uucore::signals::signal_name_by_value;
   |             ^^^^^^^ could not find `signals` in `uucore`

    Checking uu_seq v0.0.12 (D:\a\coreutils\coreutils\src\uu\seq)

@ndd7xv ndd7xv force-pushed the env-signal-messages branch 3 times, most recently from 935518e to feab2aa Compare March 28, 2022 21:15
@ndd7xv
Copy link
Contributor Author
ndd7xv commented Mar 28, 2022

I think I've just fixed that error - there's a problem with the MinRustV. I use a function to determine if I should print core dumped, but I guess that can be removed if you think I should!

The problem with the test remains - it doesn't work, and I'm not sure how to fix up this test to actually test the signal messages, as even killing GNU's coreutil's env sleep 10 command doesn't capture the error message I get (which would be Segmentation fault (core dumped)). I've described what I did in a previous comment to test if my changes are working, but the results don't seem to carry over when moved into the test. I've also tried piping stderr and stdout but they don't contain the expected output.

I've separated the test from the env changes in different commits, so we can completely scrap or change the test.

@tertsdiepraam
Copy link
Member

Thanks! It looks like that function is only stabilized in 1.58, so I think it's best to avoid it for now if possible. I think it's fine to leave out the test for this if it's really that hard to test.

@ndd7xv ndd7xv force-pushed the env-signal-messages branch from feab2aa to c64a061 Compare April 1, 2022 00:28
@sylvestre sylvestre force-pushed the env-signal-messages branch from c64a061 to 298f73f Compare April 3, 2022 18:01
@sylvestre sylvestre requested a review from tertsdiepraam April 4, 2022 07:59
@tertsdiepraam tertsdiepraam merged commit 945c66f into uutils:main Apr 4, 2022
@tertsdiepraam
Copy link
Member

Looks good! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfinkels jfinkels jfinkels left review comments

@tertsdiepraam tertsdiepraam Awaiting requested review from tertsdiepraam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ndd7xv @tertsdiepraam @sylvestre @jfinkels
0