-
Notifications
You must be signed in to change notification settings - Fork 314
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
Replace launcher's Process struct's unsafe implementations with rust stdlib #5733
Conversation
…stdlib Now that try and try_wait exist, Process can be a thin wrapper around std::process::Child on unix. Unfortunately, the same isn't currently feasible on Windows, since the process spawning depends on additional features not available in the rust stdlib currently. Also, fix some typos. Resolves #5361 Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
pub use sys::service::*; | ||
|
||
pub struct Service { | ||
args: protocol::Spawn, | ||
process: Process, | ||
status: Option<ExitStatus>, |
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.
Note this field was never mutated, so removing it should be safe
@@ -25,7 +24,7 @@ impl Handler for RestartHandler { | |||
type Reply = protocol::SpawnOk; | |||
|
|||
fn handle(msg: Self::Message, services: &mut ServiceTable) -> HandleResult<Self::Reply> { | |||
let mut service = match services.remove(msg.get_pid() as Pid) { | |||
let mut service = match services.remove(msg.get_pid() as u32) { |
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 like descriptive types as much as the next person, but std::process::Child::id()
returns a u32
, so I think it's preferable to align with the stdlib. This ripples out in a number of places, but also requires some casts since certain libc
calls take i32
.
} | ||
} | ||
pub fn try_wait(&mut self) -> io::Result<Option<ExitStatus>> { | ||
self.0.try_wait() |
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.
As you can see the stdlib implementation is nearly identical.
} | ||
} | ||
pub fn wait(&mut self) -> io::Result<ExitStatus> { | ||
self.0.wait() |
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.
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.
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
And only include use libc for unix to avoid warning Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Signed-off-by: Jon Bauman <5906042+baumanj@users.noreply.github.com>
Obvious fix; these changes are the result of automation not creative thinking.
Now that try and try_wait exist, Process can be a thin wrapper around
std::process::Child on unix. Unfortunately, the same isn't currently feasible
on Windows, since the process spawning depends on additional features not
available in the rust stdlib currently.
Also, fix some typos.
Resolves #5361