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

Implement dup and close for stdin/stdout/stderr #1511

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

samrat
Copy link
Contributor

@samrat samrat commented Aug 15, 2020

Implements some of the operations for stdin/out/err, towards #1499

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2020

@samrat what is the status of this? Trying to make sure it does not get lost. :)

@samrat
Copy link
Contributor Author

samrat commented Sep 2, 2020

@RalfJung I'll try to address the comments on the PR sometime this weekend. Sorry for having been a bit laggy with follow-ups on this issue(and the last :/ )

@RalfJung
Copy link
Member

RalfJung commented Sep 3, 2020

It's okay, delays happen. :) I just want to make sure this doesn't get lost.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

This is great, thanks a lot for working through all my comments. :)
I have just one last minor nit. Then, could you please squash these commits together? And then we can land this. :D

Ok(0)
}
let result = file_descriptor.close(this.machine.communicate)?;

Copy link
Member

Choose a reason for hiding this comment

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

Spurious empty line, I think?

@samrat
Copy link
Contributor Author

samrat commented Sep 9, 2020

Removed the newline and squashed all commits into one.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2020

📌 Commit d4538af has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Sep 9, 2020

⌛ Testing commit d4538af with merge 004ce76...

bors added a commit that referenced this pull request Sep 9, 2020
Implement dup and close for stdin/stdout/stderr

Implements some of the operations for stdin/out/err, towards #1499
@bors
Copy link
Contributor

bors commented Sep 9, 2020

💔 Test failed - checks-travis


fn main() {
unsafe {
libc::close(1); //~ ERROR stdout cannot be closed(not supported by Rust)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, the error message here is still wrong.
(Please squash that fix.)

Support F_DUPFD on stdin/stdout/stderr

Enable `close`-ing stdin/stdout/stderr

For `dup`, check if FD is `File` first

If not, clone the appropriate standard IO stream

Merge POSIX `close` and `dup` tests into same module

Also, add assertion that `write` on a closed FD returns an error.

Add `dup` as FileDescriptor trait fn

Also:
- Fix `close` so it drops `self` instead of reference to it
- Remove FD clamping in insert_fd_with_min_fd, since FDs 0-2 can be
closed

Fix fs_libc tests

Make error message when closing stdin/out/err more specific

Return io::Result from `FileDescriptor::dup`

Change error message when closing stdin/out/err

Refactor `FileDescriptor::dup` impl for `FileHandle`

Remove empty line
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2020

📌 Commit 563fb8e has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Sep 9, 2020

⌛ Testing commit 563fb8e with merge dc0a54c...

@bors
Copy link
Contributor

bors commented Sep 9, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing dc0a54c to master...

@bors bors merged commit dc0a54c into rust-lang:master Sep 9, 2020
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.

None yet

3 participants