-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: Make Client::from_env
safe to call for any number of times
#57
Conversation
095629e
to
1a67970
Compare
1a67970
to
633fb6d
Compare
So that it can be used in different crates without causing UB. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
633fb6d
to
108d8a4
Compare
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
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.
Looks good to me. Thanks!
@@ -444,6 +445,14 @@ unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner> | |||
} | |||
} | |||
|
|||
fn clone_fd_and_set_cloexec(fd: c_int) -> Result<File, FromEnvErrorInner> { | |||
// Safety: fd is a valid fd dand it remains open until returns |
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.
// Safety: fd is a valid fd dand it remains open until returns | |
// Safety: fd is a valid fd and it remains open until returns |
/// # Safety | ||
/// | ||
/// This function is `unsafe` to call on Unix specifically as it | ||
/// transitively requires usage of the `from_raw_fd` function, which is | ||
/// itself unsafe in some circumstances. | ||
/// | ||
/// It's recommended to call this function very early in the lifetime of a | ||
/// program before any other file descriptors are opened. That way you can | ||
/// make sure to take ownership properly of the file descriptors passed | ||
/// down, if any. |
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.
nit: How about this?
/// # Safety | |
/// | |
/// This function is `unsafe` to call on Unix specifically as it | |
/// transitively requires usage of the `from_raw_fd` function, which is | |
/// itself unsafe in some circumstances. | |
/// | |
/// It's recommended to call this function very early in the lifetime of a | |
/// program before any other file descriptors are opened. That way you can | |
/// make sure to take ownership properly of the file descriptors passed | |
/// down, if any. | |
/// # Safety | |
/// | |
/// See the "Safety" section in [`Client::from_env_ext`]. |
I don't think it worth more back-and-forth. Let's merge this and improve comments later! |
So that it can be used in different crates without causing UB.
Motivation
opencv-rust
usesjobserver::Client::from_env
in itsbuild.rs
to parallelize generation of bindings, then callcc::Build::compile
with featurescc/parallel
enabled it will also calljobserver::Client::from_env
and creates a UB rust-lang/cc-rs#844 (comment)Workaround for this will require either wrapping
jobserver::Client
withstd::mem::ManuallyDrop
, or switching to my forkjobslot
which supports callingClient::from_env
unlimited number of times.This PR thus port the change to upstream so that any crate using
jobserver
can benefit from this and can stop worrying about UB caused by callingClieng::from_env
multiple times in different crates.