Skip to content

Add suport for /proc/[pid]/task/[tid]/children #180

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

Merged
merged 6 commits into from
Jul 23, 2022

Conversation

zmjackson
Copy link
Contributor

While using this library I came across the need to find the children of a process. I saw that it was not supported, but it seemed fairly easy to implement so I took a shot at it.

I'm new to Rust so I welcome any suggestions.

let (mut child1, mut child2) = (command.spawn().unwrap(), command.spawn().unwrap());

// Cargo tests run on multiple threads under the same process by default
// There doesn't seem to be an easy way to get the current thread ID
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Didn't realize rustix has that utility. I'll change it.

@eminence
Copy link
Owner

You can ignore the CI failure (or you can rebase to get a fix for that)

Comment on lines 80 to 87
/// Thread children from `/proc/<pid>/task/<tid>/children`
///
/// This data will be unique per task.
Copy link
Owner

Choose a reason for hiding this comment

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

I'd love to see these docs expanded a bit. In particular, the procfs(5) man page lists some caveats that look important and I'd like to see them replicated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking at the man page, it seems the inclusion of this interface might be more harmful than helpful. Were you intentionally excluding it?

Copy link
Owner

Choose a reason for hiding this comment

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

Nope, I wasn't intentionally excluding it. It didn't look very useful, so I didn't rush to add it myself, but the procfs crate aims to support as much as possible, so I'd like to include this functionality.

@eminence
Copy link
Owner

While using this library I came across the need to find the children of a process. I saw that it was not supported, but it seemed fairly easy to implement so I took a shot at it.

I should also note that using /proc/[pid]/task/[tid]/children is not really the recommended way to get the children of a process. Generally you have to look at each processes parent process and build up a list of children processes from that. See also this example

@eminence
Copy link
Owner

Could you please rebase on top of the latest master branch (instead of merging it into your feature branch)? This will result is a much cleaner history. If you're not sure how to do this, I'd be happy to help explain the steps.

@zmjackson
Copy link
Contributor Author

Sorry, I don't have a lot of experience with forked projects. I tried to rebase but clearly it didn't work. What steps should I take to get it in the right state?

@eminence
Copy link
Owner

Oh, I think I see what might have happened. Correct me if I'm wrong, but I think your successfully rebased, but when you went to push your branch, git told you that it wasn't a fast-forward, and it recommended that you run git pull. Unfortunately, running git pull after a rebase is almost always the wrong thing to do. (Not really your fault, I've been annoyed at git for years that it continues to make this recommendation)

I hope you don't mind, but I've fixed this for you by force-pushing to your task-children branch. Be sure to fetch and git reset your local branch before re-using it.

Since I did a force-push, can you quickly skim the commits in this PR to make sure they look OK to you?

@zmjackson
Copy link
Contributor Author

zmjackson commented Jul 22, 2022

That is exactly what happened. Thanks so much for the help! Sorry for the delay, everything looks good.

@eminence eminence merged commit 18e3300 into eminence:master Jul 23, 2022
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.

2 participants