Skip to content

libstd fuchsia fixes #64023

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 4 commits into from
Sep 7, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
15 changes: 10 additions & 5 deletions src/libstd/sys/unix/process/process_fuchsia.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,18 @@ impl Command {
None => ptr::null(),
};

let transfer_or_clone = |opt_fd, target_fd| if let Some(local_fd) = opt_fd {
let make_action = |local_io: &ChildStdio, target_fd| if let Some(local_fd) = local_io.fd() {
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 Default::default();
}
fdio_spawn_action_t {
action: FDIO_SPAWN_ACTION_CLONE_FD,
local_fd: target_fd,
Expand All @@ -69,9 +73,9 @@ impl Command {
};

// 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 @@ -86,7 +90,8 @@ impl Command {
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(),
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
8 changes: 4 additions & 4 deletions src/libstd/sys/unix/process/zircon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::convert::TryInto;
use crate::io;
use crate::os::raw::c_char;
use crate::u64;
use crate::i64;

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

Expand All @@ -14,8 +14,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,7 +120,7 @@ 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;
}

Expand Down