Skip to content

Add a try_clone() function to OwnedFd. #88794

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Jan 25, 2022

Conversation

sunfishcode
Copy link
Member

As suggested in #88564. This adds a try_clone() to OwnedFd by
refactoring the code out of the existing File/Socket code.

r? @joshtriplett

As suggested in rust-lang#88564. This adds a `try_clone()` to `OwnedFd` by
refactoring the code out of the existing `File`/`Socket` code.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021
@rust-log-analyzer

This comment has been minimized.

@@ -266,22 +266,9 @@ impl FileDesc {
}
}

#[inline]
pub fn duplicate(&self) -> io::Result<FileDesc> {
Copy link
Member

Choose a reason for hiding this comment

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

Long-term, I wonder if we can eliminate this internal interface in favor of try_clone. But that's not an issue for this PR.

@joshtriplett
Copy link
Member

LGTM. r=me when CI passes.

@rust-log-analyzer

This comment has been minimized.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@sunfishcode
Copy link
Member Author

The CI now passes; @joshtriplett I think this just needs an r+.

@joshtriplett
Copy link
Member

joshtriplett commented Oct 4, 2021

LGTM

@bors r+

As a follow-up, I think we should investigate whether we can drop the special-case handling of systems without F_DUPFD_CLOEXEC. (In particular, I'm wondering if the platform could instead automatically handle F_DUPFD_CLOEXEC and translate it to F_DUPFD, or alternatively if we should export the flag from libc as an alias for compatibility.)

@bors
Copy link
Collaborator

bors commented Oct 4, 2021

📌 Commit 2d6a4c8 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 4, 2021
…r=joshtriplett

Add a `try_clone()` function to `OwnedFd`.

As suggested in rust-lang#88564. This adds a `try_clone()` to `OwnedFd` by
refactoring the code out of the existing `File`/`Socket` code.

r? `@joshtriplett`
@Manishearth
Copy link
Member

@bors r-

#89525 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2021
@sunfishcode
Copy link
Member Author

The compilation errors on WASI are now fixed.

@sunfishcode
Copy link
Member Author

@joshtriplett This previously had a compilation error on WASI, which is now fixed.

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 14, 2021
@sunfishcode
Copy link
Member Author

I've now fixed the error reported by bors.

r? @joshtriplett

@sunfishcode sunfishcode removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 15, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 11, 2021
…r=joshtriplett

Add a `try_clone()` function to `OwnedFd`.

As suggested in rust-lang#88564. This adds a `try_clone()` to `OwnedFd` by
refactoring the code out of the existing `File`/`Socket` code.

r? ``@joshtriplett``
@matthiaskrgr
Copy link
Member

@bors r-
failed in a rollup: #91790 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 11, 2021
`WSADuplicateSocketW` returns 0 on success, which differs from
handle-oriented functions which return 0 on error. Use `sys::net::cvt`
to handle its return value, which handles the socket convention of
returning 0 on success, rather than `sys::cvt`, which handles the
handle-oriented convention of returning 0 on failure.
@sunfishcode
Copy link
Member Author

The CI failure turned out to be that the code was calling cvt, and when it was moved to a different file, it silently started calling a different cvt. On Windows, one cvt treats 0 as indicating a failure, while the other cvt treats 0 as indicating success.

r? @joshtriplett

@sunfishcode sunfishcode added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2022
@joshtriplett
Copy link
Member

@sunfishcode Good catch. That seems like a source of potential future bugs. Would you consider a separate PR to rename the socket-like one to cvt_socket or similar, to make the behavior difference more obvious?

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 24, 2022

📌 Commit 83aebf8 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2022
…r=joshtriplett

Add a `try_clone()` function to `OwnedFd`.

As suggested in rust-lang#88564. This adds a `try_clone()` to `OwnedFd` by
refactoring the code out of the existing `File`/`Socket` code.

r? `@joshtriplett`
This was referenced Jan 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88794 (Add a `try_clone()` function to `OwnedFd`.)
 - rust-lang#93064 (Properly track `DepNode`s in trait evaluation provisional cache)
 - rust-lang#93118 (Move param count error emission to end of `check_argument_types`)
 - rust-lang#93144 (Work around missing code coverage data causing llvm-cov failures)
 - rust-lang#93169 (Fix inconsistency of local blanket impls)
 - rust-lang#93175 (Implement stable overlap check considering negative traits)
 - rust-lang#93251 (rustdoc settings: use radio buttons for theme)
 - rust-lang#93269 (Use error-on-mismatch policy for PAuth module flags.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 687bb58 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Feb 8, 2022
Port std's `try_clone`, added in rust-lang/rust#88794, to io-lifetimes.

This puts the actua clone logic under control of `feature = "close"`, as
it has a similar circular dependency issue with rustix.
sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Feb 8, 2022
This corresponds to the `try_clone` function in sunfishcode/io-lifetimes#16
and rust-lang/rust#88794.
sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Feb 8, 2022
This corresponds to the `try_clone` function in sunfishcode/io-lifetimes#16
and rust-lang/rust#88794.
sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Feb 8, 2022
This corresponds to the `try_clone` function in sunfishcode/io-lifetimes#16
and rust-lang/rust#88794.
sunfishcode added a commit to sunfishcode/io-lifetimes that referenced this pull request Feb 8, 2022
Port std's `try_clone`, added in rust-lang/rust#88794, to io-lifetimes.

This puts the actua clone logic under control of `feature = "close"`, as
it has a similar circular dependency issue with rustix.
sunfishcode added a commit to bytecodealliance/rustix that referenced this pull request Feb 10, 2022
This corresponds to the `try_clone` function in sunfishcode/io-lifetimes#16
and rust-lang/rust#88794.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.