code review 4532075: syscall: Controlling TTY support in StartProcess

syscall: Controlling TTY support in StartProcess

These changes add a Ctty int field to the Unix syscall.ProcAttr which,
if set >= 0 in conjuction with Setsid=true, will be used by
forkAndExecInChild as the file descriptor for the new child's
controlling terminal.

Necessary changes have been made to mkerrors.sh to generate defs for
TIOC*, though changes to its output files are not included here.
The changes made should support Linux, FreeBSD and Darwin, at least.

[krockot, 2011-05-21 02:14:01]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[iant, 2011-05-21 05:54:08]

http://codereview.appspot.com/4532075/diff/4001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4532075/diff/4001/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:147: if attr.Ctty >= 0 {
In general we want the 0 value to indicate that nothing special should happen. 
We should either just not permit setting the controlling terminal to descriptor
0, or do something odd like set the value to descriptor plus 1.

[krockot, 2011-05-21 06:09:18]

> In general we want the 0 value to indicate that nothing special should happen.

> We should either just not permit setting the controlling terminal to
descriptor
> 0, or do something odd like set the value to descriptor plus 1.

Yeah, I had considered this too.  I think it might be reasonable to not support
fd 0 at all here.  Using (Ctty - 1) feels awkward, and it should be uncommon for
a parent to hand over fd 0.  In the case where someone wanted to, they could
just Dup it first.

[krockot, 2011-05-21 07:38:52]

Hello golang-dev@googlegroups.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-05-21 14:27:30]

Pointer to int like optional fields in protobuf?

(phone), brad
On May 20, 2011 10:54 PM, <iant@golang.org> wrote:
>
>
http://codereview.appspot.com/4532075/diff/4001/src/pkg/syscall/exec_unix.go
> File src/pkg/syscall/exec_unix.go (right):
>
>
http://codereview.appspot.com/4532075/diff/4001/src/pkg/syscall/exec_unix.go#...
> src/pkg/syscall/exec_unix.go:147: if attr.Ctty >= 0 {
> In general we want the 0 value to indicate that nothing special should
> happen. We should either just not permit setting the controlling
> terminal to descriptor 0, or do something odd like set the value to
> descriptor plus 1.
>
> http://codereview.appspot.com/4532075/

[krockot, 2011-05-22 01:40:24]

Hello golang-dev@googlegroups.com, iant@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-05-22 01:44:45]

http://codereview.appspot.com/4532075/diff/10001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4532075/diff/10001/src/pkg/syscall/exec_unix.go...
src/pkg/syscall/exec_unix.go:277: Ctty       *int        // Controlling TTY fd
if Setsid.
why is this a pointer?

[r2, 2011-05-22 02:37:29]

On 22/05/2011, at 11:51 AM, Ken Rockot wrote:

> fd 0 is a valid value to want to use as the controlly tty.  original code had
an int and used -1 to mean "no tty," but then zero would have meaning.  the next
alternative was to simply treat 0 as no-tty and consequently disallow fd 0 as an
option.  brad suggested using an *int, which both allows fd 0 to be used and
prevents Ctty=0 from being meaningul.

This seems really hacky. There's a bool and an integer pointer, which is not a
great interface.

I don't see what's wrong with the original design.  I would have used -1 for "no
tty".  Fd 0 is a fine default for the right tty to use.

-rob

[bradfitzgoog, 2011-05-22 02:50:33]

Ian wanted a no-op zero value. I threw out the pointer thing as a possible
solution that's hacky (as Ian requested) but at least consistent with
protos. *shrug*

(phone), brad
On May 21, 2011 7:37 PM, "Rob &apos;Commander&apos; Pike" <r@google.com>
wrote:
>
> On 22/05/2011, at 11:51 AM, Ken Rockot wrote:
>
>> fd 0 is a valid value to want to use as the controlly tty. original code
had an int and used -1 to mean "no tty," but then zero would have meaning.
the next alternative was to simply treat 0 as no-tty and consequently
disallow fd 0 as an option. brad suggested using an *int, which both allows
fd 0 to be used and prevents Ctty=0 from being meaningul.
>
> This seems really hacky. There's a bool and an integer pointer, which is
not a great interface.
>
> I don't see what's wrong with the original design. I would have used -1
for "no tty". Fd 0 is a fine default for the right tty to use.
>
> -rob
>

[r2, 2011-05-22 03:54:32]

On 22/05/2011, at 12:50 PM, Brad Fitzpatrick wrote:

> Ian wanted a no-op zero value. I threw out the pointer thing as a possible
solution that's hacky (as Ian requested) but at least consistent with protos.
*shrug*
> 
Yeah, but protos require a mechanism and the one I picked is... hacky.  It's not
clear a mechanism is needed here, but if Ian insists on the zero  value being a
no op (a reasonable demand, but then if setsid is false (its zero value) none of
this matters) let's make it clean and alloc-free.

Setsid bool  (should be SetSid but anyway)
SetCtty bool
Ctty int

With tagged composite literals the extra bulk only costs if you use it.

Or you could do a typed integer field:

type sidOp int
const (
	SidNop sidOp = iota
	SidCreate
	SidCreateWithCtty
)

then

SidOp sidOp
Ctty int // only used for SidOp = SidCreateWithTTY

You could even go as far as to make the Sid stuff a substructure, if you want.  
The op-driven approach also have the merit that its provides a growth path,
which is clearly needed: Linux is sure to add more crap.

-rob

[r2, 2011-05-23 13:17:58]

On 23/05/2011, at 11:20 AM, Ken Rockot wrote:

> How about this:
> 
> type Session struct {
>     Setctty bool
>     Ctty int
> }
> 
> type ProcAttr struct {
>     Session *Session
>     //...
> }

ok, but call it a SessionAttr and the field should be SetCtty.

Yeah, controlling tty. When did you last see a teletype? (Rhetorical question.)

-rob

[krockot, 2011-05-23 21:33:24]

Hello golang-dev@googlegroups.com, iant@golang.org, bradfitz@golang.org,
r@golang.org, r@google.com, bradfitz@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[krockot, 2011-05-23 21:33:38]

Hello golang-dev@googlegroups.com, iant@golang.org, bradfitz@golang.org,
r@golang.org, r@google.com, bradfitz@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2011-05-26 03:48:47]

i think this is a good design.

http://codereview.appspot.com/4532075/diff/15001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4532075/diff/15001/src/pkg/syscall/exec_unix.go...
src/pkg/syscall/exec_unix.go:270: type Credential struct {
these types all need doc comments.

[krockot, 2011-05-27 23:52:58]

Hello golang-dev@googlegroups.com, iant@golang.org, bradfitz@golang.org,
r@golang.org, r@google.com, bradfitz@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-14 15:42:18]

+borman for his opinion

Sorry this fell by the wayside.  I just submitted
some changes to syscall that shuffle this around a bit
but your changes are still just as relelvant.

If you hg sync, update the code, and hg mail 
I'd be happy to push this forward.

Thanks.

[borman, 2011-06-14 19:16:55]

TIOCSCTTY taking an argument is OS specific (it is used in Linux to allow
"stealing" the controlling terminal but is not available in darwin (and I
don't believe *BSD)).  Both TIOCSCTTY and TIOCNOTTY should be made
available.  While in almost all probability calling TIOCSCTTY without first
calling setsid() will fail in this usage, the code should do what is
requested, even if we know it will generate an error (this comes from using
the presence  of the Session structure to determine if setsid should be
called or not, I prefer the explicit bool Setsid as in the most recent
code).

If an argument is to be provided for TIOCSCTTY then it should fail at
compile time for any system that does not support it.  The name Ctty for the
argument is misleading.  I first thought it was the fd you wanted to call
TIOCSCTTY on, now I see it is in fact the linux specific argument.

    -Paul

On Tue, Jun 14, 2011 at 8:42 AM, <rsc@golang.org> wrote:

> +borman for his opinion
>
> Sorry this fell by the wayside.  I just submitted
> some changes to syscall that shuffle this around a bit
> but your changes are still just as relelvant.
>
> If you hg sync, update the code, and hg mail
> I'd be happy to push this forward.
>
> Thanks.
>
>
http://codereview.appspot.com/**4532075/<http://codereview.appspot.com/4532075/>
>

[borman, 2011-06-15 00:14:31]

I am not convinced that this should be a complete new structure.  I see four
different things:


   1. Indicator we should call setsid()
   2. Indicator we should call setpgid(0, 0) (same as System V's setpgrp())
   3. Indicator we should call ioctl(fd, TIOCSCTTY)
   4. Indicator we should call ioctl(fd, TIOCNOCTTY)

While there is a relationship between a controlling TTY and a session ID and
a process group ID I don't think they need to be clumped together.

An argument can be made against 2) above because in Posix you can call
setpgid on another process.  There is a race condition there, however.
 Also, in all but System V you can specify the process group to become part
of.  There can be multiple process groups in a single session.

For the latter two, the ioctls, while it might not make sense to call both,
it is probably cleaner to allow calling both.  This probably could each be
represented by a reference to an integer:

    ScttyFD *int
    NocttyFD *int

The presence of the reference both indicates we should make the call and
provides us the value to the call.

I will say I am torn between flexibility and the fact that nearly every call
to TIOC{S,NO}CTTY is on filedes 0...

Is there a valid use case wear fd is not 0?

    -Paul

On Tue, Jun 14, 2011 at 3:44 PM, Ken Rockot <ken.rockot@gmail.com> wrote:

> I was thinking: how do you guys feel about moving Setsid into the
> SessionAttr struct?  I was thinking of defining it like:
>
> type SessionAttr struct {
>     Notty bool
>     Setsid bool
>     Setctty bool
>     Ctty int
> }
>
> type SysProcAttr struct {
>     ...
>     Session *Session
> }
>
> It doesn't feel like there's any way to combine the above options into a
> more general flag without losing some potential functionality (regardless of
> its usefulness or lack thereof.)  It also doesn't feel right to have a
> separate Session field dealing with all things session-related, but to have
> Setsid sit beside it in SysProcAttr.
>
> If Session is nil, nothing happens.  Otherwise, Notty=true will call
> TIOCNOTTY and the remaining flags will elicit the same behavior they do now,
> in that order.
>

[krockot, 2011-06-20 01:08:31]

Hello iant@golang.org, bradfitz@golang.org, r@golang.org, rsc@golang.org,
borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[borman, 2011-06-20 01:21:53]

Other than perhaps changing Sctty to Setctty, this looks reasonable to me.

http://codereview.appspot.com/4532075/diff/22001/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4532075/diff/22001/src/pkg/syscall/exec_unix.go...
src/pkg/syscall/exec_unix.go:310: Sctty      bool        // Set controlling
terminal to fd 0
Setctty probably is better than Sctty.

[krockot, 2011-06-20 01:54:44]

Hello iant@golang.org, bradfitz@golang.org, r@golang.org, rsc@golang.org,
borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[borman, 2011-06-20 16:36:29]

LGTM

[rsc, 2011-06-20 19:34:35]

It looks like you need to add some files to this change list?
mkerrors.sh changed but none of the files it generates are updated.

[rsc, 2011-06-20 22:30:01]

> I could make the relevant (minimal) changes by hand, but files will still be
> out of sync with the full output of mkerrors.sh.  Is that a problem so long
> as syscall still builds?

Please remove exec_unix.go from this change list
(hg file -d 4532075 path:src/syscall/exec_unix.go)
and then remail it (hg mail 4532075).  We can apply
that and then gather the necessary outputs.

Then run hg change to create a new change list with
exec_unix.go, which can be submitted once all the
necessary constants have been added to the syscall
package.

Thanks.
Russ

[krockot, 2011-06-20 22:34:51]

Hello iant@golang.org, bradfitz@golang.org, r@golang.org, rsc@golang.org,
borman@google.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-21 15:59:13]

LGTM

[rsc, 2011-06-21 16:00:55]

It doesn't look like you've completed a CLA.
Please complete a CLA as described at
http://golang.org/doc/contribute.html#copyright

Thanks.
Russ

[rsc, 2011-06-22 22:07:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=d988ff98cb14 ***

syscall: add tty support to StartProcess

These changes add a Ctty int field to the Unix syscall.ProcAttr which,
if set >= 0 in conjuction with Setsid=true, will be used by
forkAndExecInChild as the file descriptor for the new child's
controlling terminal.

Necessary changes have been made to mkerrors.sh to generate defs for
TIOC*, though changes to its output files are not included here.
The changes made should support Linux, FreeBSD and Darwin, at least.

R=iant, bradfitz, r, rsc, borman
CC=golang-dev
http://codereview.appspot.com/4532075

Committer: Russ Cox <rsc@golang.org>