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

default FDs to CLOEXEC, expose enable_on_exec #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

krallin
Copy link

@krallin krallin commented Mar 10, 2023

Hey there, thanks for this crate.

Here are couple patches I've been using that I thought you might find useful to merge:

default FDs to CLOEXEC

Note: this is a behavior change.

Currently, this crate opens FDs without CLOEXEC, which means they
persist across exec. This is not the convention throughout the Rust
ecosystem, which is to do the opposite:

https://github.com/rust-lang/rust/issues/24237

This patch sets CLOEXEC :)

Note that doing that after-the-fact on the resulting FD isn't quite good
enough: if your process is forking a lot, that's racy.

expose enable-on-exec

This adds support for setting `enable-on-exec`

krallin added 2 commits March 10, 2023 05:51
This adds support for setting `enable-on-exec`
Note: this is a behavior change.

Currently, this crate opens FDs without CLOEXEC, which means they
persist across exec. This is not the convention throughout the Rust
ecosystem, which is to do the opposite:

rust-lang/rust#24237

This patch setting CLOEXEC :)

Note that doing that after-the-fact on the resulting FD isn't quite good
enough: if your process is forking a lot, that's racy.
facebook-github-bot pushed a commit to facebookexperimental/reverie that referenced this pull request Mar 15, 2023
Summary:
For jimblandy/perf-event#29 which sets CLOEXEC by
default.

Reviewed By: edward-shen

Differential Revision:
D43983384

Privacy Context Container: L1123788

fbshipit-source-id: 478c0e11bd815e916094b48ce7763b4dfc3e5f79
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Mar 15, 2023
Summary:
For jimblandy/perf-event#29 which sets CLOEXEC by
default.

Reviewed By: edward-shen

Differential Revision:
D43983384

Privacy Context Container: L1123788

fbshipit-source-id: 478c0e11bd815e916094b48ce7763b4dfc3e5f79
facebook-github-bot pushed a commit to facebook/fb303 that referenced this pull request Mar 15, 2023
Summary:
For jimblandy/perf-event#29 which sets CLOEXEC by
default.

Reviewed By: edward-shen

Differential Revision:
D43983384

Privacy Context Container: L1123788

fbshipit-source-id: 478c0e11bd815e916094b48ce7763b4dfc3e5f79
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Mar 15, 2023
Summary:
For jimblandy/perf-event#29 which sets CLOEXEC by
default.

Reviewed By: edward-shen

Differential Revision:
D43983384

Privacy Context Container: L1123788

fbshipit-source-id: 478c0e11bd815e916094b48ce7763b4dfc3e5f79
This adds support for excuding user code, thus measuring only Kernel
code.
Phantomical added a commit to Phantomical/perf-event that referenced this pull request Apr 14, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
Phantomical added a commit to Phantomical/perf-event that referenced this pull request Apr 20, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
Phantomical added a commit to Phantomical/perf-event that referenced this pull request Apr 20, 2023
This appears to be what the rust stdlib does (see [0]) so following
along with their conventions is probably best.

This commit was inspired by this pr to perf-event:
jimblandy/perf-event#29

[0]: rust-lang/rust#24237
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.

1 participant