code review 5674072: runtime: Permit default behaviour of SIGTSTP, SIGTTIN, ...

runtime: Permit default behaviour of SIGTSTP, SIGTTIN, SIGTTOU.

Fixes issue 3037.

[rsc, 2012-02-16 19:15:09]

LGTM

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/signal_unix.c
File src/pkg/runtime/signal_unix.c (right):

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:30: if((t->flags == 0) || (t->flags & SigDefault))
{
Drop new { }

[minux1, 2012-02-16 19:27:33]

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:144: if(runtime·sigtab[i].flags & SigDefault)
You have to declare runtime·sigtab[] in this file, or the code won't compile.

[dsymonds, 2012-02-16 21:11:57]

Hello rsc@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

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

[dsymonds, 2012-02-16 21:13:17]

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/signal_unix.c
File src/pkg/runtime/signal_unix.c (right):

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:30: if((t->flags == 0) || (t->flags & SigDefault))
{
On 2012/02/16 19:15:10, rsc wrote:
> Drop new { }

Oops, dreg.

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:144: if(runtime·sigtab[i].flags & SigDefault)
On 2012/02/16 19:27:33, minux wrote:
> You have to declare runtime·sigtab[] in this file, or the code won't compile.

I clearly don't, since this compiles just fine.  ;-)

[rsc, 2012-02-16 21:14:23]

> I clearly don't, since this compiles just fine.  ;-)

you should put

extern SigTab runtime.sigtab[]; 

at the top of the file.  The fact that C code compiles
is not a guarantee that it is correct.

[dsymonds, 2012-02-16 21:15:35]

On Fri, Feb 17, 2012 at 8:14 AM,  <rsc@golang.org> wrote:

>> I clearly don't, since this compiles just fine.  ;-)
>
>
> you should put
>
> extern SigTab runtime.sigtab[];
>
> at the top of the file.  The fact that C code compiles
> is not a guarantee that it is correct.

Done.

[rsc, 2012-02-16 21:15:47]

Actually, I have no idea why the C compiler would allow
this code.  I can't find a forward declaration of runtime.sigtab
anywhere, and it would need one to determine what .flags means.

[rsc, 2012-02-16 21:18:41]

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc
File src/pkg/runtime/sigqueue.goc (right):

http://codereview.appspot.com/5674072/diff/2001/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:144: if(runtime·sigtab[i].flags & SigDefault)
On 2012/02/16 21:13:17, dsymonds wrote:
> On 2012/02/16 19:27:33, minux wrote:
> > You have to declare runtime·sigtab[] in this file, or the code won't
compile.
> 
> I clearly don't, since this compiles just fine.  ;-)

Also, this is supposed to be portable, meaning not
Unix-specific.  Can you put this code in signal_unix.c,
which already has the right extern declaration?

[dsymonds, 2012-02-16 21:29:08]

On Fri, Feb 17, 2012 at 8:18 AM,  <rsc@golang.org> wrote:

> Also, this is supposed to be portable, meaning not
> Unix-specific.  Can you put this code in signal_unix.c,
> which already has the right extern declaration?

SigTab is in runtime.h, as are the flags, so it looks portable enough
to me. But I could put these bits into new functions in signal_unix.c
if you really want.

[rsc, 2012-02-16 21:36:57]

On Thu, Feb 16, 2012 at 16:29, David Symonds <dsymonds@golang.org> wrote:
> SigTab is in runtime.h, as are the flags, so it looks portable enough
> to me. But I could put these bits into new functions in signal_unix.c
> if you really want.

setsig is not in runtime.h, and it is not implemented on
the non-unix systems.  please put the code in signal_unix.c,
maybe runtime.sigenable(int32) with 0 meaning everything.
you'll need to add dummy functions to signal_windows.c
and signal_plan9.c too.

it is possible that if you run

GOOS=windows GOARCH=386 make.bash

it will do a windows build for you, to test that
it builds.

[dsymonds, 2012-02-16 22:11:09]

PTAL

[dsymonds, 2012-02-16 22:18:21]

wait, I'm getting a panic in the os/signal test.

[dsymonds, 2012-02-16 23:20:02]

Okay, ready for a final review. I need to use ~0 as the sentinel to
runtime.sigenable.

[dsymonds, 2012-02-16 23:22:40]

No, wait. It isn't. Darn.

[rsc, 2012-02-16 23:44:11]

> No, wait. It isn't. Darn.

Thanks for slogging through it.  Welcome to signals.

[dsymonds, 2012-02-17 01:55:34]

Okay, it's good to go now.

Turns out that calling runtime·setsig on a signal the second time
causes a mysterious panic. Rather than trying to unravel all that to
try to make it idempotent, I just changed runtime·sigenable(~0) to
only call runtime·setsig for SigDefault flags (i.e. the ones that
weren't enabled during runtime·initsig).


Dave.

[r, 2012-02-17 02:03:48]

Fixes issue 3037?

[dsymonds, 2012-02-17 02:05:13]

On Fri, Feb 17, 2012 at 1:03 PM,  <r@golang.org> wrote:

> Fixes issue 3037?

Yep.

[rsc1, 2012-02-17 02:47:16]

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c
File src/pkg/runtime/signal_unix.c (right):

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:46: if(t->flags & SigDefault)
You should probably clear SigDefault here so you don't setsig it twice if there
are two Notify(c) calls.

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:52: runtime·setsig(sig, runtime·sighandler, true);
Same here: check SigDefault and only enable in that case (and clear).

[dsymonds, 2012-02-17 03:23:56]

PTAL

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c
File src/pkg/runtime/signal_unix.c (right):

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:46: if(t->flags & SigDefault)
On 2012/02/17 02:47:16, rsc1 wrote:
> You should probably clear SigDefault here so you don't setsig it twice if
there
> are two Notify(c) calls.

Done.

http://codereview.appspot.com/5674072/diff/7024/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:52: runtime·setsig(sig, runtime·sighandler, true);
On 2012/02/17 02:47:16, rsc1 wrote:
> Same here: check SigDefault and only enable in that case (and clear).

This is now called from initsig above. I guess I need to detach it again.

I rewrote this function to cut down the duplication.

[rsc1, 2012-02-17 03:25:43]

LGTM

[dsymonds, 2012-02-17 03:36:46]

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

runtime: Permit default behaviour of SIGTSTP, SIGTTIN, SIGTTOU.

Fixes issue 3037.

R=rsc, minux.ma, r, rsc
CC=golang-dev
http://codereview.appspot.com/5674072