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.

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

Expose decode on WaitStatus and make it return a Result #741

Merged
merged 1 commit into from
Dec 9, 2017
Merged

Expose decode on WaitStatus and make it return a Result #741

merged 1 commit into from
Dec 9, 2017

Conversation

rocallahan
Copy link
Contributor

@rocallahan rocallahan commented Aug 21, 2017 •
edited by Susurrus
Loading

Closes #740.

@rocallahan rocallahan mentioned this pull request Aug 21, 2017
Closed
Susurrus
Susurrus suggested changes Aug 21, 2017
View reviewed changes
Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

This is seeming pretty reasonable to add to nix, so let's go ahead and move forward with refining it a bit. It'll need a CHANGELOG entry for the addition of from_raw and also one for the change in return type of status::* if those are exposed in the public API of nix.

src/sys/wait.rs Outdated
} else if status::signaled(status) {
WaitStatus::Signaled(pid, try!(status::term_signal(status)), status::dumped_core(status))
} else if status::stopped(status) {
cfg_if! {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would read better if we can get rid of this helper function. If not, let's move this helper function out of this function and instead into the module (and add good docs for it even though it's private).

src/sys/wait.rs Outdated
} else {
WaitStatus::PtraceEvent(pid, status::stop_signal(status), status::stop_additional(status))
impl WaitStatus {
pub fn from_raw(pid : Pid, status: i32) -> Result<WaitStatus> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a doccomment for consumers of this and also this could really use some comments internally to describe what's going on. I'm unfamiliar with this API and so it'd help me out greatly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, does it make sense to have the pid argument be Pid? Your examples all involve two from_raw calls because you have to convert the pid and the (pid, status) pair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to me that we should use Pid in public APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could really use some comments internally to describe what's going on.

I'll just reference the waitpid man pages... We're just following the logic there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it would be clearer if the functions in status actually used libc::WIFEXITED etc. I don't know why the code doesn't already do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And actually if you do that then this whole config-dependent status module can go away.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #732. Maybe this work should wait on me merging that. If you'd help me review that we could get that merged pretty quick I think.

test/sys/test_wait.rs Outdated
@@ -38,6 +39,14 @@ fn test_wait_exit() {
}

#[test]
fn test_wait_raw() {
Copy link
Contributor

Choose a reason for hiding this comment

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

test_waitstatus_from_raw

@rocallahan
Copy link
Contributor Author

I'll drop the second commit now that I see you've already created a PR for that, and your patch is better than mine.

@Susurrus Susurrus mentioned this pull request Aug 21, 2017
Merged
@Susurrus
Copy link
Contributor

This needs a bit of a rebase and clean up if it's going to get merged. @rocallahan You still interested in getting this merged?

@rocallahan
Copy link
Contributor Author

Yeah

@rocallahan
Copy link
Contributor Author

I think this addresses everything if the tests pass.

@Susurrus
Copy link
Contributor

Susurrus commented Dec 4, 2017

@rocallahan Sorry, this must have slipped through the radar, sorry about that! I think a simple rebase and this thing is good to merge. @asomers Did you want to have a look-see here?

@rocallahan
Copy link
Contributor Author

nix master doesn't build for me.

error[E0425]: cannot find value `MFD_CLOEXEC` in module `libc`
 --> src/sys/memfd.rs:8:9
  |
8 |         MFD_CLOEXEC;
  |         ^^^^^^^^^^^ did you mean `EFD_CLOEXEC`?

error[E0425]: cannot find value `MFD_ALLOW_SEALING` in module `libc`
 --> src/sys/memfd.rs:9:9
  |
9 |         MFD_ALLOW_SEALING;
  |         ^^^^^^^^^^^^^^^^^ not found in `libc`

@rocallahan
Copy link
Contributor Author

Never mind, fixed it.

@rocallahan
Copy link
Contributor Author

Alright, branch rebased.

Susurrus
Susurrus suggested changes Dec 4, 2017
View reviewed changes
Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

In addition to the small doccomment fix, I'd love an example for this method. Is there a simple usecase you could whip up as an example? It's be great if it could be run as a test, but even just a compile-test would be a nice improvement.

src/sys/wait.rs Outdated
WaitStatus::PtraceEvent(pid, stop_signal(status), stop_additional(status))
impl WaitStatus {
/// Convert a raw `wstatus` as returned by `waitpid`/`wait` into a `WaitStatus`.
/// Returns an `Error` corresponding to `EINVAL` for invalid status values.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a blank line between these 2 comments. That makes the first line the short summary line in the generated docs. Additionally since the second talks about errors, there's a standard way to display this into under a # Errors header. The rust doccomment documentation talks about those sections.

@rocallahan
Copy link
Contributor Author

OK.

@Susurrus
Copy link
Contributor

Susurrus commented Dec 5, 2017

I canceled those two Mac jobs, because Travis has a huge backlog and I trust this passes, so I'm ready to try to merge which will re-run tests anyways. I'll merge this tonight if #696 merges before I go to bed otherwise I'll wait until the Mac backlog dies down again tomorrow night.

@Susurrus
Copy link
Contributor

Susurrus commented Dec 5, 2017

bors r+

bors bot added a commit that referenced this pull request Dec 5, 2017
@bors
Merge #741
bd5c173 
741: Expose `decode` on `WaitStatus` and make it return a `Result` r=Susurrus a=rocallahan

Closes #740.
@bors
Copy link
Contributor

bors bot commented Dec 5, 2017

Timed out

@Susurrus
Copy link
Contributor

Susurrus commented Dec 9, 2017

bors r+

bors bot added a commit that referenced this pull request Dec 9, 2017
@bors
Merge #741
e1ce361 
741: Expose `decode` on `WaitStatus` and make it return a `Result` r=Susurrus a=rocallahan

Closes #740.
@bors
Copy link
Contributor

bors bot commented Dec 9, 2017

Build succeeded

@bors bors bot merged commit 996bc6b into nix-rust:master Dec 9, 2017
@Susurrus Susurrus mentioned this pull request Jan 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Susurrus Susurrus Susurrus approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@rocallahan @Susurrus