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

Filedescriptor crate to support dup2 #786

Closed
robinbernon opened this issue May 12, 2021 · 6 comments
Closed

Filedescriptor crate to support dup2 #786

robinbernon opened this issue May 12, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@robinbernon
Copy link
Contributor

Hi Wez. Am currently looking into tweaking the gag crate so that it works cross platform and wanted to use your filedescriptor crate to do it. Was wandering whether your crate supports dup2? Need this feature in order to redirect stdout/stderr into file. Happy to submit a PR for this, although I don't do much low level stuff so might need a couple pointers here..

@robinbernon robinbernon added the enhancement New feature or request label May 12, 2021
@wez
Copy link
Owner

wez commented May 12, 2021

There's no direct support for dup2, but it wouldn't be difficult to add; I'd be happy to accept a PR for that.

This is the code used for dup:
https://github.com/wez/wezterm/blob/main/filedescriptor/src/unix.rs#L107-L147

There are two flavors: one that atomically dup's with CLOEXEC set, and one that non-atomically does the same.
To add dup2 you'll repeat that pattern but using dup3 (pass O_CLOEXEC as the third parameter) for the atomic case, and dup2 for the non-atomic case.

I'd suggest making the method have an unsafe signature to represent that there is potential danger in re-assigning the fd:

#[cfg(unix)]
unsafe pub fn dup2<F: AsRawFileDescriptor>(f: &F, dest_fd: RawFd) -> Result<Self>;

If you wanted to portably redirect stdio streams on windows you can't literally use dup2 there, but instead would need to look at using SetStdHandle and passing the result of calling https://docs.rs/filedescriptor/0.7.3/i686-pc-windows-msvc/filedescriptor/trait.IntoRawFileDescriptor.html to that API.

If you also wanted to add a portable API for that to filedescriptor I'm open to it; I think you'd need to add an enum representing stdin, stdout and stderr and then have it call the appropriate dup2 or SetStdHandle call depending on the OS.

@robinbernon
Copy link
Contributor Author

I've opened up a premature PR with only windows directs supported for now: #788
Are these sorts of changes what you had in mind? (Gonna now work on the unix equivalent methods)

@wez
Copy link
Owner

wez commented May 12, 2021

I'd suggest something like:

#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub enum StdioDescriptor {
   Stdin,
   Stdout,
   Stderr,
}

impl FileDescriptor {
   /// Duplicates the descriptor representing `stdio`, then re-assigns that descriptor
   /// by duplicating `f`.
   /// Returns the original decriptor.
   pub fn redirect_stdio<F: AsRawFileDescriptor>(f: &F, stdio: StdioDescriptor) -> Result<Self>;
}

Under the covers that would use FileDescriptor::dup to get the original stream, the use dup2 on unix, or SetStdHandle on windows to assign it.

I'd prefer not to include the restore-on-drop functionality in this crate; gag could cleanly and portably implement that functionality on top of this interface:

struct TemporaryRedirection {
   stdio: StdioDescriptor,
   fd: FileDescriptor
}

impl TemporaryRedirection {
  pub fn new<F: AsRawFileDescriptor>(f: &F, stdio: StdioDescriptor) -> Result<Self> {
    let fd = FileDescriptor::redirect_stdio(f, stdio)?;
    Ok(Self { fd, stdio }}
  }
}

impl Drop for TemporaryRedirection {
   fn drop(&mut self) {
      let _ = FileDescriptor::redirect_stdio(self.fd, self.stdio);
   }
}

@robinbernon
Copy link
Contributor Author

Sounds good - will just return the original file descriptor in the redirect_stdio method like you mentioned above then 👍
(will aim to submit a finalised PR by the end of tomorrow then)

@robinbernon
Copy link
Contributor Author

@wez have also made a PR for the gag crate now: Stebalien/gag-rs#7

@wez wez closed this as completed in ed33154 May 23, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2023

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants