Skip to content
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

wait: Support ptrace events for Linux #438

Merged
merged 3 commits into from
Feb 14, 2017
Merged

Conversation

chaosagent
Copy link
Contributor

Adds new WaitStatus value PtraceEvent. Implementation of #273 that only affects Linux/Android.

@chaosagent
Copy link
Contributor Author

This works on newer versions of Rust; not really sure how to cleanly do a cfg on a block in previous versions.

Copy link
Member

@kamalmarhubi kamalmarhubi left a comment

Choose a reason for hiding this comment

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

Thanks for this! I have an idea on how to get this working on non-nightly. I hope it works. :-)

@@ -184,6 +191,12 @@ fn decode(pid : pid_t, status: i32) -> WaitStatus {
} else if status::signaled(status) {
WaitStatus::Signaled(pid, status::term_signal(status), status::dumped_core(status))
} else if status::stopped(status) {
#[cfg(any(target_os = "linux", target_os = "android"))] {
Copy link
Member

@kamalmarhubi kamalmarhubi Nov 4, 2016

Choose a reason for hiding this comment

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

I didn't know cfg on blocks was possible, even if nightly only.

To do this on stable, I think your best approach is going to be adding an inner function like this:

// ...
} else if status::stopped(status) {
cfg_if! {
    if #[cfg(any(target_os = "linux", target_os = "android"))] {
        fn decode_stopped(status) -> WaitStatus {
            // Linux code
        }
    } else {
        fn decode_stopped(status) -> WaitStatus {
            // Other platform code
        }
    }
}
decode_stopped(status)
} else {
    // ...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, should have pointed out where that comes from. We already have it in nix, so it's not a risk to use it.

@kamalmarhubi
Copy link
Member

@luser @abbradar could you take a look at this? It looks reasonable to me, but I haven't ptraced things much.

@kamalmarhubi
Copy link
Member

Thanks @chaosagent! I'm going to try and get someone who's used these APIs to see if they make sense. Should be able to merge soon though. :-)

@chaosagent
Copy link
Contributor Author

chaosagent commented Nov 6, 2016

I've found this implementation of this, which works pretty much the same, if that's of any help.

Edit: screwed up markdown

@abbradar
Copy link
Contributor

abbradar commented Nov 6, 2016

On a quick glance this seems OK to me. I can't remember if it's possible that non-signal event still has ptrace information. Anyway, that's better than my old solution!

@kamalmarhubi
Copy link
Member

@chaosagent I'm really sorry for the delay on this PR. I'm going to assume that nothing has changed since November in terms of correctness.

@homu r+

@homu
Copy link
Contributor

homu commented Feb 14, 2017

📌 Commit e6bfbbb has been approved by kamalmarhubi

homu added a commit that referenced this pull request Feb 14, 2017
wait: Support ptrace events for Linux

Adds new WaitStatus value `PtraceEvent`. Implementation of #273 that only affects Linux/Android.
@homu
Copy link
Contributor

homu commented Feb 14, 2017

⌛ Testing commit e6bfbbb with merge 7a91a81...

@homu
Copy link
Contributor

homu commented Feb 14, 2017

☀️ Test successful - status

@homu homu merged commit e6bfbbb into nix-rust:master Feb 14, 2017
kamalmarhubi added a commit to kamalmarhubi/nix-rust that referenced this pull request Feb 14, 2017
kamalmarhubi added a commit to kamalmarhubi/nix-rust that referenced this pull request Feb 14, 2017
homu added a commit that referenced this pull request Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants