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

libstd fuchsia fixes #64023

Merged
merged 4 commits into from
Sep 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/libstd/sys/unix/process/process_common.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
use crate::os::unix::prelude::*;

use crate::ffi::{OsString, OsStr, CString, CStr};
use crate::ffi::{OsString, OsStr, CString};
use crate::fmt;
use crate::io;
use crate::ptr;
use crate::sys::fd::FileDesc;
use crate::sys::fs::{File, OpenOptions};
use crate::sys::fs::File;
use crate::sys::pipe::{self, AnonPipe};
use crate::sys_common::process::{CommandEnv, DefaultEnvKey};
use crate::collections::BTreeMap;

#[cfg(not(target_os = "fuchsia"))]
Copy link
Member

Choose a reason for hiding this comment

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

Since this file is shared amongst most platforms, would it be possible to avoid the fuchsia-specific cfgs? Could Null be used on all platforms and only at the very end it's lowered to /dev/null?

use {
crate::ffi::CStr,
crate::sys::fs::OpenOptions,
};

use libc::{c_int, gid_t, uid_t, c_char, EXIT_SUCCESS, EXIT_FAILURE};

cfg_if::cfg_if! {
if #[cfg(target_os = "redox")] {
if #[cfg(target_os = "fuchsia")] {
// fuchsia doesn't have /dev/null
} else if #[cfg(target_os = "redox")] {
const DEV_NULL: &'static str = "null:\0";
} else {
const DEV_NULL: &'static str = "/dev/null\0";
Expand Down Expand Up @@ -83,6 +91,11 @@ pub enum ChildStdio {
Inherit,
Explicit(c_int),
Owned(FileDesc),

// On Fuchsia, null stdio is the default, so we simply don't specify
// any actions at the time of spawning.
#[cfg(target_os = "fuchsia")]
Null,
}

pub enum Stdio {
Expand Down Expand Up @@ -301,6 +314,7 @@ impl Stdio {
Ok((ChildStdio::Owned(theirs.into_fd()), Some(ours)))
}

#[cfg(not(target_os = "fuchsia"))]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in love with these inline cfgs, but they seem to match the way redox was treated above, and I don't think this would be more cleanly expressed via a separate file. 🤷‍♀️

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

Stdio::Null => {
let mut opts = OpenOptions::new();
opts.read(readable);
Expand All @@ -311,6 +325,11 @@ impl Stdio {
let fd = File::open_c(&path, &opts)?;
Ok((ChildStdio::Owned(fd.into_fd()), None))
}

#[cfg(target_os = "fuchsia")]
Stdio::Null => {
Ok((ChildStdio::Null, None))
}
}
}
}
Expand All @@ -333,6 +352,9 @@ impl ChildStdio {
ChildStdio::Inherit => None,
ChildStdio::Explicit(fd) => Some(fd),
ChildStdio::Owned(ref fd) => Some(fd.raw()),

#[cfg(target_os = "fuchsia")]
ChildStdio::Null => None,
}
}
}
Expand Down
63 changes: 43 additions & 20 deletions src/libstd/sys/unix/process/process_fuchsia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,30 +48,51 @@ impl Command {
use crate::sys::process::zircon::*;

let envp = match maybe_envp {
Some(envp) => envp.as_ptr(),
// None means to clone the current environment, which is done in the
// flags below.
None => ptr::null(),
Some(envp) => envp.as_ptr(),
};

let transfer_or_clone = |opt_fd, target_fd| if let Some(local_fd) = opt_fd {
fdio_spawn_action_t {
action: FDIO_SPAWN_ACTION_TRANSFER_FD,
local_fd,
target_fd,
..Default::default()
}
} else {
fdio_spawn_action_t {
action: FDIO_SPAWN_ACTION_CLONE_FD,
local_fd: target_fd,
target_fd,
..Default::default()
let make_action = |local_io: &ChildStdio, target_fd| -> io::Result<fdio_spawn_action_t> {
if let Some(local_fd) = local_io.fd() {
Ok(fdio_spawn_action_t {
action: FDIO_SPAWN_ACTION_TRANSFER_FD,
local_fd,
target_fd,
..Default::default()
})
} else {
if let ChildStdio::Null = local_io {
// acts as no-op
return Ok(Default::default());
}

let mut handle = ZX_HANDLE_INVALID;
let status = fdio_fd_clone(target_fd, &mut handle);
if status == ERR_INVALID_ARGS || status == ERR_NOT_SUPPORTED {
// This descriptor is closed; skip it rather than generating an
// error.
return Ok(Default::default());
}
zx_cvt(status)?;

let mut cloned_fd = 0;
zx_cvt(fdio_fd_create(handle, &mut cloned_fd))?;

Ok(fdio_spawn_action_t {
action: FDIO_SPAWN_ACTION_TRANSFER_FD,
local_fd: cloned_fd as i32,
target_fd,
..Default::default()
})
}
};

// Clone stdin, stdout, and stderr
let action1 = transfer_or_clone(stdio.stdin.fd(), 0);
let action2 = transfer_or_clone(stdio.stdout.fd(), 1);
let action3 = transfer_or_clone(stdio.stderr.fd(), 2);
let action1 = make_action(&stdio.stdin, 0)?;
let action2 = make_action(&stdio.stdout, 1)?;
let action3 = make_action(&stdio.stderr, 2)?;
let actions = [action1, action2, action3];

// We don't want FileDesc::drop to be called on any stdio. fdio_spawn_etc
Expand All @@ -84,9 +105,11 @@ impl Command {

let mut process_handle: zx_handle_t = 0;
zx_cvt(fdio_spawn_etc(
0,
FDIO_SPAWN_CLONE_JOB | FDIO_SPAWN_CLONE_LDSVC | FDIO_SPAWN_CLONE_NAMESPACE,
self.get_argv()[0], self.get_argv().as_ptr(), envp, 3, actions.as_ptr(),
ZX_HANDLE_INVALID,
FDIO_SPAWN_CLONE_JOB | FDIO_SPAWN_CLONE_LDSVC | FDIO_SPAWN_CLONE_NAMESPACE
| FDIO_SPAWN_CLONE_ENVIRON, // this is ignored when envp is non-null
self.get_argv()[0], self.get_argv().as_ptr(), envp,
actions.len() as size_t, actions.as_ptr(),
&mut process_handle,
ptr::null_mut(),
))?;
Expand Down
12 changes: 8 additions & 4 deletions src/libstd/sys/unix/process/zircon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

use crate::convert::TryInto;
use crate::io;
use crate::i64;
use crate::mem::MaybeUninit;
use crate::os::raw::c_char;
use crate::u64;

use libc::{c_int, c_void, size_t};

Expand All @@ -14,8 +15,8 @@ pub type zx_status_t = i32;

pub const ZX_HANDLE_INVALID: zx_handle_t = 0;

pub type zx_time_t = u64;
pub const ZX_TIME_INFINITE : zx_time_t = u64::MAX;
pub type zx_time_t = i64;
pub const ZX_TIME_INFINITE : zx_time_t = i64::MAX;

pub type zx_signals_t = u32;

Expand Down Expand Up @@ -120,8 +121,11 @@ pub struct fdio_spawn_action_t {
extern {
pub fn fdio_spawn_etc(job: zx_handle_t, flags: u32, path: *const c_char,
argv: *const *const c_char, envp: *const *const c_char,
action_count: u64, actions: *const fdio_spawn_action_t,
action_count: size_t, actions: *const fdio_spawn_action_t,
process: *mut zx_handle_t, err_msg: *mut c_char) -> zx_status_t;

pub fn fdio_fd_clone(fd: c_int, out_handle: *mut zx_handle_t) -> zx_status_t;
pub fn fdio_fd_create(handle: zx_handle_t, fd: *mut c_int) -> zx_status_t;
}

// fdio_spawn_etc flags
Expand Down