Add a method to set the controlling tty of child processes (unix).

[withoutboats]

On UNIX, processes may have a controlling terminal. Currently, there is no way to set the controlling terminal of a child process using the std::process::Command API. I am working on a library which abstracts over the tty subsystem; this method is needed to create a Rustic equivalent to fork_pty(3).

The proposed API works like this:

• Calling Command::tty() sets session_leader to true, regardless of whether or not session_leader() has been called. It prevents future calls to session_leader from having any impact.
• Command::tty() does not automatically cause the controlling terminal to be set as the stdio handles of the child process. To do that, one needs to call the appropriate methods with Stdio::from_raw_fd(fd) or the like. I made this decision to make the API more flexible (e.g. if you want to run a process that needs to have a controlling terminal set for some reason, but you don't want to communicate with it over that tty pair).
• During Command::spawn(), in the child process, after setting the process to be the leader of a new session, if an fd has been set to be the ctty for the child, this change is performed using the TIOCSCTTY ioctl.

I think this code is correct, but I feel a bit out of my depth with regard to a few things. In particular: is TIOCSCTTY supported, with the same ioctl number, on all relevant platforms? is closing the file descriptor in the child process the correct behavior in the context of Rust's RAII?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

I do not think calling Command::tty(c).session_leader(false) should pass through silently and should be reported when a finalizer method (spawn) is called.

[withoutboats]

@nagisa what would you propose should happen? I think there are four options:

• tty() doesn't set session_leader at all, and not calling session_leader() will result in whatever error the TIOCSCTTY ioctl returns when called on a process that is not the session lead.
• tty() doesn't set session_leader, and not calling session_leader() causes the TIOCSCTTY call to never occur (and therefore fail silently).
• tty() does set session_leader, but sessional_leader() is not blocked from mutating the tty? tty(fd).session_leader(false) would then be different from session_leader(false).tty(fd).
• The present behavior, tty() sets session_leader and calls to session_leader() are ignored.

Only the first and last seem reasonable to me; I chose the last because you will always want to set session_leader if you are setting the controlling tty. It is a choice between having to do session_leader(true).tty(fd) every time, and having .session_leader(false).tty(fd) not do what it says (because what it says cannot be done). I definitely see the argument for the latter as being more clear.

[nagisa]

tty sets session_leader to true and if it is unset by the time spawn is
called an error occurs.
On Oct 12, 2015 10:37 AM, "withoutboats" [email protected] wrote:

@nagisa https://github.com/nagisa what woul you propose should happen?
I think there are four options:

• tty() doesn't set session_leader at all, and not calling
  session_leader() will result in whatever error the TIOCSCTTY ioctl
  returns when called on a process that is not the session lead.
• tty() doesn't set session_leader, and not calling session_leader()
  causes the TIOCSCTTY call to never occur (and therefore fail silently).
• tty() does set session_leader, but sessional_leader() is not blocked
  from mutating the tty? tty(fd).session_leader(false) would then be
  different from session_leader(false).tty(fd).
• The present behavior, tty() sets session_leader and calls to
  session_leader() are ignored.

Only the first and last seem reasonable to me; I chose the last because
you will always want to set session_leader if you are setting the
controlling tty. It is a choice between having to do
session_leader(true).tty(fd) every time, and having
.session_leader(false).tty(fd) not do what it says (because what it says
cannot be done). I definitely see the argument for the latter as being more
clear.

—
Reply to this email directly or view it on GitHub
#28982 (comment).

[nagisa]

Expanding on my point above:

It is not really unrealistic for Command::tty(xxyy).session_leader(false).session_leader(true) to happen, because builder can be passed around and may easily accumulate some odd options which in the end are all valid. That being said, there should also be some way to unset tty in addition to setting it.

[alexcrichton]

Thanks for the PR @withoutboats! I think m feelings by this point are that there's enough various niche things you can do post-fork, pre-exec that there's no way we're going to bind all of them. I would prefer to have at this point a unix-specific "run this closure after the fork" function rather than continue to pile on various functions here and there.

There's already a bunch of related desires to have this sort of functionality, and it should make building this sort of function quite easy on crates.io as an extension trait!

[withoutboats]

@nagisa I understand wanting to be able to undo any setting on the Command, but that currently isn't possible for many other settings (e.g. args()), and only indirectly possible for most of the rest (e.g. to remove all set env vars, you'd have to call env_clear() and then rebuild setting the inherited env vars). session_leader() is the odd one out here in taking a boolean value, more on this below.

@alexcrichton Being able to run a closure after the fork is definitely the solution to everyone's problem. Literally none of the functionality in os::unix::CommandExt would need to be in std if that were available. Reading old issues, though, it seemed like that had a soundness issue I didn't fully understand that blocks it for now. If there's not a soundness issue, I'd be glad to implement that also.

I also don't think that setting controlling terminal is niche, though: its a fundamental part of job control just like creating a new session. Passing in termios flags and window size parameters (which fork_pty(3) accepts) is what I'd say is niche, but that's all possible to write in a third party library if std will let us set an fd as the controlling tty.

Merging these two conversation streams, I'd actually say that creating a new session and setting the ctty for that session are deeply coupled behaviors. Currently the API lets you create a new session, but only one without a controlling terminal. I think the actual best API here is for session_leader() to take an Option<&T> where T: AsRawFd, which will be made into the controlling terminal for the session of the child process. My branch has been updated to reflect the new proposed API. I think being able to unset the various configurations on the process builder is a separate conversation, though.

(I've incidentally removed a misleading comment from the docs, though it seems out of place that the API docs for session_leader gives instructions on how to create a daemon in the first place.)

[alexcrichton]

@withoutboats Ah yeah I agree that most of the extension trait on unix may not be necessary, but that doesn't mean we should remove all the methods! It's definitely nice for us to handle some of the most common use cases (like this if it turns out to be that way) in the standard library itself. The purpose of the escape hatch "run this closure" would just be to provide any ability to do this externally so this sort of functionality could grow outside first.

I also think old issues about safety and such don't necessarily matter so much, this may need to be unsafe but beyond that there shouldn't be anything fundamentally impossible about this I believe.

[withoutboats]

Oh no, I dind't mean to suggest that other methods be deprecated. Obviously having to pass a closure to the child to run after the fork is not very convenient and shouldn't be the only way to set up the child. I do think that it doesn't make a lot of sense to support session_leader() without supporting setting the controlling terminal of the new session.

I'll look into opening a separate PR with a run-after-fork closure. I think the closure would be an FnOnce() -> ()?

[alexcrichton]

That plus I believe it may want to be F: FnOnce() + Sync + Send + 'static to ensure that that send/sync-ness of Command stayed the same.

[alexcrichton]

cc rust-lang/rfcs#1359, an RFC I've now opened on adding the ability to run an after-fork closure

[l0kod]

@withoutboats you may be interested in tty-rs ;)

[alexcrichton]

I'm gonna close this due to inactivity in favor of rust-lang/rfcs#1359 for now, but we can always reopen/resubmit depending on the outcome over there!