code review 3749041: os/signal: selective signal handling

os/signal: selective signal handling

Restore package os/signal, with new API:
Notify replaces Incoming, allowing clients
to ask for certain signals only.  Also, signals
go to everyone who asks, not just one client.

This could plausibly move into package os now
that there are no magic side effects as a result
of the import.

Update runtime for new API: move common Unix
signal handling code into signal_unix.c.
(It's so easy to do this now that we don't have
to edit Makefiles!)

Tested on darwin,linux 386,amd64.

Fixes issue 1266.

[rsc, 2012-02-08 23:13:41]

Hello golang-dev@googlegroups.com,

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

[rsc, 2012-02-08 23:14:41]

This touches a lot of files, but almost all trivially.

Don't worry about the large diffs in a few of the z files.
I will regenerate them on the correct machines.

Russ

[r2, 2012-02-09 00:07:16]

doc/go1 updates too please

[r, 2012-02-09 00:11:56]

i'm happy with it staying a separate package but could be talked into merging it
with os

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode52
src/pkg/os/exec.go:52: type Signal interface {
this is OK, but i wonder if we want to add a unique method so this is
discriminated from other Stringers, for safety

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:8: // BUG(rsc): This package is not implemented on
Plan 9 and Windows.
s/not/not yet/

[dsymonds, 2012-02-09 00:12:46]

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:17: l []handler
I assume "l" means "list", but it's not obvious to me, and the single "l" is too
close to "1" in many fonts. Can you call this "handlers" or "hs" or something?

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:34: func Notify(c chan<- os.Signal, sig os.Signal) {
This could even be variadic, which would avoid the whole "sig == nil" vs. "sig
!= nil" business. Plus, if you are selective, it's reasonably common to want
more than one on the same channel anyway, so it would be a neat shortcut.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sig.go
File src/pkg/runtime/sig.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sig.go#newcode7
src/pkg/runtime/sig.go:7: // Sigrecv returns a bitmask of signals that have
arrived since the last call to Sigrecv.
fix doc names too

[bradfitz, 2012-02-09 00:23:35]

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode52
src/pkg/os/exec.go:52: type Signal interface {
On 2012/02/09 00:11:57, r wrote:
> this is OK, but i wonder if we want to add a unique method so this is
> discriminated from other Stringers, for safety

Or just change this from String() to Signal(), like we did for the error
interface and Error().

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode59
src/pkg/os/exec.go:59: SIGINT  Signal = syscall.SIGINT
is this all caps thing just legacy?  These are starting to look odd.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:60: if e := syscall.Kill(p.Pid, sig.(syscall.Signal)); e
!= nil {
can we comma-ok assert the type and return an error rather than exploding here?

[iant, 2012-02-09 01:20:58]

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode59
src/pkg/os/exec.go:59: SIGINT  Signal = syscall.SIGINT
On 2012/02/09 00:23:35, bradfitz wrote:
> is this all caps thing just legacy?  These are starting to look odd.

I think if we keep the name SIGINT it should be all-caps.  For this purpose I
could imagine os.Interrupt and os.Kill, except that those sound like function
names, not constants.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal_unix.go
File src/pkg/os/signal/signal_unix.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal_unix...
src/pkg/os/signal/signal_unix.go:22: for sig := uint(0); sig < 32; sig++ {
The real time signals have values > 32.

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

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:25: runtime·sigignore(int32, Siginfo*, void*, G*)
What is the advantage of using this function rather than SIG_IGN?

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:41: restart = (t->flags & SigRestart) != 0;
Why handle SigRestart separately?  In the current code, SigRestart and SigQueue
always appear together.  In Go I can not imagine a case when we would want
SigQueue without SigRestart.  And SigRestart without SigQueue doesn't make much
sense either, since it would mean that the signal was dropped.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:60: t->flags |= SigEnabled;
Could use a comment here explaining why this data structure is locked.

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

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:88: if(!sig.inuse) {
Here I think you are assuming that the first call is at initialization time with
m == 0.  Worth at least a comment.

[rsc, 2012-02-09 03:08:50]

On Wed, Feb 8, 2012 at 20:20,  <iant@golang.org> wrote:
> runtime·sigignore(int32, Siginfo*, void*, G*)
> What is the advantage of using this function rather than SIG_IGN?

SIG_IGN is inherited by child processes, while if I use my own
ignoring handler the child process will get SIG_DFL instead.
This is problematic if you, say, ignore SIGPIPE and then run a
program that expects the default SIGPIPE behavior because it
isn't doing anything special.  The same is true but worse for SIGCHLD.
If you use your own ignoring handler, what happens is what you expect:
you ignore SIGCHLD signals.  If you set SIGCHLD's handler to SIG_IGN,
then wait(2) stops working: all child processes that exit are immediately
reclaimed instead of turning into zombies that can be waited for.

That's just a long way of saying that every time I've used SIG_IGN
it has turned out to hurt, so I instinctively shy away from it now.
I decided it was probably okay for SIGPROF.

Russ

[borman, 2012-02-09 16:43:27]

This solution does not address the issue of the static runtime·sigtab table and
its use.  The application has no way to catch many of the signals, including
SIGQUIT.  An application should be able to catch any signal the OS will deliver
to the running program.

[rsc, 2012-02-09 18:10:17]

On Thu, Feb 9, 2012 at 11:43,  <borman@google.com> wrote:
> This solution does not address the issue of the static runtime·sigtab
> table and its use.  The application has no way to catch many of the
> signals, including SIGQUIT.  An application should be able to catch any
> signal the OS will deliver to the running program.

Thanks for the feedback.

If there are specific signals that an application should be able to handle,
please point them out and I will adjust the table.  I will fix SIGQUIT.
It is, however, not true that an application should be able to catch
any signal that could be delivered.  Some signals are a direct result
of execution behaviors and cannot be handled asynchronously.
These signals are translated into panics in Go when appropriate
and can be handled that way.  I am thinking of things like SIGTRAP,
SIGFPE, SIGSEGV.

Russ

[borman, 2012-02-09 20:20:02]

This brings up an issue with Unix signals.  All signals can be interrupts
(excepting SIGKILL) while some signals can also be faults.  The potential
faults (in the linux table) are:

ILL, TRAP, BUS, FPE, SEGV, STKFLT, SYS (sort of)

It is possible for an application to handle all of these.  Using panic
means you must have the recover in the exact same go routine the fault is
triggered.  For an application to try and be robust it can handle all of
these.  That handler reverts the signal to default (to prevent recursion)
and then attempts cleanup.  If what are saying is that if any of these
faults happen it is always impossible for the go runtime to continue then
there is no reason to provide handlers.

But as a middle ground, I can accept that these are the only signals
(+KILL) that are not handleable by the application (FPE is probably the
most likely to be handleable, but recover is probably sufficient in that
case.)

On Thu, Feb 9, 2012 at 10:10 AM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Feb 9, 2012 at 11:43,  <borman@google.com> wrote:
> > This solution does not address the issue of the static runtime·sigtab
> > table and its use.  The application has no way to catch many of the
> > signals, including SIGQUIT.  An application should be able to catch any
> > signal the OS will deliver to the running program.
>
> Thanks for the feedback.
>
> If there are specific signals that an application should be able to handle,
> please point them out and I will adjust the table.  I will fix SIGQUIT.
> It is, however, not true that an application should be able to catch
> any signal that could be delivered.  Some signals are a direct result
> of execution behaviors and cannot be handled asynchronously.
> These signals are translated into panics in Go when appropriate
> and can be handled that way.  I am thinking of things like SIGTRAP,
> SIGFPE, SIGSEGV.
>
> Russ
>

[rsc, 2012-02-09 20:49:53]

On Thu, Feb 9, 2012 at 15:20, Paul Borman <borman@google.com> wrote:
> This brings up an issue with Unix signals.  All signals can be interrupts
> (excepting SIGKILL) while some signals can also be faults.  The potential
> faults (in the linux table) are:
>
> ILL, TRAP, BUS, FPE, SEGV, STKFLT, SYS (sort of)
>
> It is possible for an application to handle all of these.

Not with this API.  We are not going to make signal handlers
trigger extra code in whatever goroutine happens to be running.
The point of the channel is to turn what amounts to an external
attempt at communication into an actual communication.
But that means that signals that must be handled synchronously
(or else they'll just keep coming in), like ILL or TRAP or BUS
or FPE cannot be handled this way.  They must be panics.

Do you happen to know how to distinguish
signals generated by the kernel due to program execution from
signals of the same kind generated by kill(2)?  I don't mind
handing the latter off to handlers.

Russ

[iant2, 2012-02-09 21:41:57]

Russ Cox <rsc@golang.org> writes:

> Do you happen to know how to distinguish
> signals generated by the kernel due to program execution from
> signals of the same kind generated by kill(2)?  I don't mind
> handing the latter off to handlers.

A signal sent by kill(2) will set the si_code field of the siginfo_t
structure to SI_USER.

Ian

[borman, 2012-02-09 21:49:13]

In linux you can use the si_code in the siginfo_t structure passed to your
handler if you register for SA_SIGINFO.

#include <signal.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

void h(int s, siginfo_t *info, void *v) {
        printf("h1: %d %d\n", s, info->si_code);
        exit(0);
}

main(int ac, char **av) {
        struct sigaction act;

        memset(&act, 0, sizeof(act));
        act.sa_sigaction = h;
        act.sa_flags = SA_SIGINFO;
        sigaction(SIGSEGV, &act, NULL);
        if (ac > 1) {
                kill(0, SIGSEGV);
        } else {
                printf("%s\n", (char *)0);
        }
}

si_code == 0 on a kill and != 0 on a fault.

I tried to test it with FPE but divide by zero doesn't fault.
 max_float*max_float doesn't fault...

On Thu, Feb 9, 2012 at 12:49 PM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Feb 9, 2012 at 15:20, Paul Borman <borman@google.com> wrote:
> > This brings up an issue with Unix signals.  All signals can be interrupts
> > (excepting SIGKILL) while some signals can also be faults.  The potential
> > faults (in the linux table) are:
> >
> > ILL, TRAP, BUS, FPE, SEGV, STKFLT, SYS (sort of)
> >
> > It is possible for an application to handle all of these.
>
> Not with this API.  We are not going to make signal handlers
> trigger extra code in whatever goroutine happens to be running.
> The point of the channel is to turn what amounts to an external
> attempt at communication into an actual communication.
> But that means that signals that must be handled synchronously
> (or else they'll just keep coming in), like ILL or TRAP or BUS
> or FPE cannot be handled this way.  They must be panics.
>
> Do you happen to know how to distinguish
> signals generated by the kernel due to program execution from
> signals of the same kind generated by kill(2)?  I don't mind
> handing the latter off to handlers.
>
> Russ
>

[rsc1, 2012-02-10 23:08:44]

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go
File src/pkg/os/exec.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode52
src/pkg/os/exec.go:52: type Signal interface {
On 2012/02/09 00:23:35, bradfitz wrote:
> On 2012/02/09 00:11:57, r wrote:
> > this is OK, but i wonder if we want to add a unique method so this is
> > discriminated from other Stringers, for safety
> 
> Or just change this from String() to Signal(), like we did for the error
> interface and Error().

The difference is that after we did that, we taught fmt about the error
interface.
I don't want to teach fmt about Signal, but I want Signals to be printable.
I added a dummy Signal() method.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec.go#newcode59
src/pkg/os/exec.go:59: SIGINT  Signal = syscall.SIGINT
I renamed these to Interrupt and Kill.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec_unix.go
File src/pkg/os/exec_unix.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/exec_unix.go#newco...
src/pkg/os/exec_unix.go:60: if e := syscall.Kill(p.Pid, sig.(syscall.Signal)); e
!= nil {
On 2012/02/09 00:23:35, bradfitz wrote:
> can we comma-ok assert the type and return an error rather than exploding
here?

Done.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:8: // BUG(rsc): This package is not implemented on
Plan 9 and Windows.
On 2012/02/09 00:11:57, r wrote:
> s/not/not yet/

Done.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:17: l []handler
On 2012/02/09 00:12:46, dsymonds wrote:
> I assume "l" means "list", but it's not obvious to me, and the single "l" is
too
> close to "1" in many fonts. Can you call this "handlers" or "hs" or something?

Done.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:34: func Notify(c chan<- os.Signal, sig os.Signal) {
On 2012/02/09 00:12:46, dsymonds wrote:
> This could even be variadic, which would avoid the whole "sig == nil" vs. "sig
> != nil" business. Plus, if you are selective, it's reasonably common to want
> more than one on the same channel anyway, so it would be a neat shortcut.

Done.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal_unix.go
File src/pkg/os/signal/signal_unix.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/os/signal/signal_unix...
src/pkg/os/signal/signal_unix.go:22: for sig := uint(0); sig < 32; sig++ {
On 2012/02/09 01:20:58, iant wrote:
> The real time signals have values > 32.

Done.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sig.go
File src/pkg/runtime/sig.go (right):

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sig.go#newcode7
src/pkg/runtime/sig.go:7: // Sigrecv returns a bitmask of signals that have
arrived since the last call to Sigrecv.
On 2012/02/09 00:12:46, dsymonds wrote:
> fix doc names too

File is deleted.

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

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:25: runtime·sigignore(int32, Siginfo*, void*, G*)
On 2012/02/09 01:20:58, iant wrote:
> What is the advantage of using this function rather than SIG_IGN?

Gone (in favor of the general handler).

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:41: restart = (t->flags & SigRestart) != 0;
On 2012/02/09 01:20:58, iant wrote:
> Why handle SigRestart separately?  In the current code, SigRestart and
SigQueue
> always appear together.  In Go I can not imagine a case when we would want
> SigQueue without SigRestart.  And SigRestart without SigQueue doesn't make
much
> sense either, since it would mean that the signal was dropped.

Agreed.  I made everything restart.  I can't think of any cases where
we want to interrupt the system call, actually.

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:60: t->flags |= SigEnabled;
On 2012/02/09 01:20:58, iant wrote:
> Could use a comment here explaining why this data structure is locked.

Done.

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

http://codereview.appspot.com/3749041/diff/4007/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:88: if(!sig.inuse) {
On 2012/02/09 01:20:58, iant wrote:
> Here I think you are assuming that the first call is at initialization time
with
> m == 0.  Worth at least a comment.

Done.

[rsc, 2012-02-10 23:14:07]

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

Please take another look.

[rsc, 2012-02-10 23:18:03]

PTAL

I changed the code to handle an arbitrary number of signals
and to distinguish signals sent by the kernel from signals
sent using the kill(2) system call.  The Go runtime only tries
to handle the signals it cares about when they have been
sent by the kernel.

Russ

[iant, 2012-02-11 01:47:29]

LGTM

Looks you can get rid of the restart parameter of runtime·setsig.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_posix.go
File src/pkg/os/exec_posix.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:42: return p.Signal(syscall.SIGKILL)
s/syscall.SIGKILL/Kill/ ?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:40: if sig == syscall.SIGKILL {
s/syscall.SIGKILL/Kill/ ?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:47: if s != nil {
Why are you checking for nil here?  Should that be documented?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_darwin.h
File src/pkg/runtime/os_darwin.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_darwin.h#n...
src/pkg/runtime/os_darwin.h:26: void runtime·sighandler(int32 sig, Siginfo
*info, void *context, G *gp);
Indentation of function names looks wrong.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_linux.h
File src/pkg/runtime/os_linux.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_linux.h#ne...
src/pkg/runtime/os_linux.h:15: void runtime·sighandler(int32 sig, Siginfo *info,
void *context, G *gp);
Indentation of function name looks wrong.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_netbsd.h
File src/pkg/runtime/os_netbsd.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_netbsd.h#n...
src/pkg/runtime/os_netbsd.h:14: void runtime·sighandler(int32 sig, Siginfo
*info, void *context, G *gp);
Indentation of function name looks wrong.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_openbsd.h
File src/pkg/runtime/os_openbsd.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_openbsd.h#...
src/pkg/runtime/os_openbsd.h:14: void runtime·sighandler(int32 sig, Siginfo
*info, void *context, G *gp);
Indentation of function name looks wrong.

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

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:27: if(mask == 0) {
Do you still need this test?  Do you need the mask parameter at all?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:35: return;
This return statement is kind of useless.

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

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:58: if(!sig.inuse || s < 0 || s >=
32*nelem(sig.wanted) || !(sig.wanted[s/32]&(1<<(s&31))))
s/1<</1U<</
(signed 1 << 31 is undefined in ISO C if int is 32 bits)

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:60: bit = 1 << (s&31);
s/1 <</1U <</

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:85: if(recv[i/32]&(1<<(i&31))) {
s/1<</1U<</

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:86: recv[i/32] ^= 1<<(i&31);
s/1<</1U<</

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:105: recv[i] = m;
I think this code would be clearer if you did only a break; if the runtime·cas
returns true.  Then the other lines would move after the for(;;) loop.  That
would make it clear that the for(;;) is just a cas loop.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:149: sig.wanted[s/32] |= 1<<(s&31);
s/1<</1U<</

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh
File src/pkg/syscall/mkerrors.sh (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:223: egrep -v '(SIGSTKSIZE|SIGRT)' |
You don't like SIGSTKSIZE?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:231: echo '#include <sys/signal.h>' | $GCC -x c - -E
-dM $ccflags |
s|<sys/signal.h>|<signal.h>| ?

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:260: #include <sys/signal.h>
Seems like should be <signal.h>, not <sys/signal.h>.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
File src/pkg/syscall/zerrors_darwin_386.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
src/pkg/syscall/zerrors_darwin_386.go:1324: 131072: "unknown signal",
How did this 131072 entry get in there?  We definitely don't want that.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
File src/pkg/syscall/zerrors_darwin_amd64.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
src/pkg/syscall/zerrors_darwin_amd64.go:1324: 131072: "unknown signal",
Another 131072.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
File src/pkg/syscall/zerrors_linux_386.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
src/pkg/syscall/zerrors_linux_386.go:1514: 8192: "unknown signal 8192",
This one has an 8192.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
File src/pkg/syscall/zerrors_linux_amd64.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
src/pkg/syscall/zerrors_linux_amd64.go:1515: 8192: "unknown signal 8192",
Another undesired 8192.

[dsymonds, 2012-02-11 02:10:28]

LGTM

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:42: if len(sig) == 0 {
I was imagining something slightly different: using a nil os.Signal to mean all
signals. The code ends up a bit more subtle, but shorter. I'm not fussed either
way, but thought I'd sketch out my original idea.

if len(sig) == 0 {
  sig = []os.Signal{nil}
}
for _, s := range sig {
  enableSignal(s)
  handlers.list = append(handlers.list, handler{c: c, sig: s})
}

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:60: if h.all || h.sig == sig {
... and then this is just
  if h.sig == nil || h.sig == sig {

[rsc, 2012-02-13 17:03:26]

(Not uploaded yet.)

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_posix.go
File src/pkg/os/exec_posix.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_posix.go#newc...
src/pkg/os/exec_posix.go:42: return p.Signal(syscall.SIGKILL)
On 2012/02/11 01:47:29, iant wrote:
> s/syscall.SIGKILL/Kill/ ?

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_windows.go
File src/pkg/os/exec_windows.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/exec_windows.go#ne...
src/pkg/os/exec_windows.go:40: if sig == syscall.SIGKILL {
On 2012/02/11 01:47:29, iant wrote:
> s/syscall.SIGKILL/Kill/ ?

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:42: if len(sig) == 0 {
On 2012/02/11 02:10:29, dsymonds wrote:
> I was imagining something slightly different: using a nil os.Signal to mean
all
> signals. The code ends up a bit more subtle, but shorter. I'm not fussed
either
> way, but thought I'd sketch out my original idea.
> 
> if len(sig) == 0 {
>   sig = []os.Signal{nil}
> }
> for _, s := range sig {
>   enableSignal(s)
>   handlers.list = append(handlers.list, handler{c: c, sig: s})
> }

If you allow the "Notify(c) means everything", then
I don't think you should also make Notify(c, nil) mean
the same thing.  There should be only one way to do it.
That's why this code is explicitly ignoring the nil
(you'll never get that signal).

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:47: if s != nil {
On 2012/02/11 01:47:29, iant wrote:
> Why are you checking for nil here?  Should that be documented?

Commented.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:60: if h.all || h.sig == sig {
On 2012/02/11 02:10:29, dsymonds wrote:
> ... and then this is just
>   if h.sig == nil || h.sig == sig {

See above.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_darwin.h
File src/pkg/runtime/os_darwin.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_darwin.h#n...
src/pkg/runtime/os_darwin.h:26: void runtime·sighandler(int32 sig, Siginfo
*info, void *context, G *gp);
On 2012/02/11 01:47:29, iant wrote:
> Indentation of function names looks wrong.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_linux.h
File src/pkg/runtime/os_linux.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_linux.h#ne...
src/pkg/runtime/os_linux.h:15: void runtime·sighandler(int32 sig, Siginfo *info,
void *context, G *gp);
On 2012/02/11 01:47:29, iant wrote:
> Indentation of function name looks wrong.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_openbsd.h
File src/pkg/runtime/os_openbsd.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/os_openbsd.h#...
src/pkg/runtime/os_openbsd.h:14: void runtime·sighandler(int32 sig, Siginfo
*info, void *context, G *gp);
On 2012/02/11 01:47:29, iant wrote:
> Indentation of function name looks wrong.

Done.

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

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:27: if(mask == 0) {
On 2012/02/11 01:47:29, iant wrote:
> Do you still need this test?  Do you need the mask parameter at all?

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:27: if(mask == 0) {
On 2012/02/11 01:47:29, iant wrote:
> Do you still need this test?  Do you need the mask parameter at all?

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signal_unix.c...
src/pkg/runtime/signal_unix.c:35: return;
On 2012/02/11 01:47:29, iant wrote:
> This return statement is kind of useless.

Done.

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

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:58: if(!sig.inuse || s < 0 || s >=
32*nelem(sig.wanted) || !(sig.wanted[s/32]&(1<<(s&31))))
On 2012/02/11 01:47:29, iant wrote:
> s/1<</1U<</
> (signed 1 << 31 is undefined in ISO C if int is 32 bits)

!!!!!!!!!!!!!!!!!!!?! WTF.  Wow.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:60: bit = 1 << (s&31);
On 2012/02/11 01:47:29, iant wrote:
> s/1 <</1U <</

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:85: if(recv[i/32]&(1<<(i&31))) {
On 2012/02/11 01:47:29, iant wrote:
> s/1<</1U<</

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:86: recv[i/32] ^= 1<<(i&31);
On 2012/02/11 01:47:29, iant wrote:
> s/1<</1U<</

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:105: recv[i] = m;
On 2012/02/11 01:47:29, iant wrote:
> I think this code would be clearer if you did only a break; if the runtime·cas
> returns true.  Then the other lines would move after the for(;;) loop.  That
> would make it clear that the for(;;) is just a cas loop.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/sigqueue.goc#...
src/pkg/runtime/sigqueue.goc:149: sig.wanted[s/32] |= 1<<(s&31);
On 2012/02/11 01:47:29, iant wrote:
> s/1<</1U<</

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh
File src/pkg/syscall/mkerrors.sh (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:223: egrep -v '(SIGSTKSIZE|SIGRT)' |
On 2012/02/11 01:47:29, iant wrote:
> You don't like SIGSTKSIZE?

It's kind of big.  :-)

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:231: echo '#include <sys/signal.h>' | $GCC -x c - -E
-dM $ccflags |
On 2012/02/11 01:47:29, iant wrote:
> s|<sys/signal.h>|<signal.h>| ?

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/mkerrors.sh#n...
src/pkg/syscall/mkerrors.sh:260: #include <sys/signal.h>
On 2012/02/11 01:47:29, iant wrote:
> Seems like should be <signal.h>, not <sys/signal.h>.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
File src/pkg/syscall/zerrors_darwin_386.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
src/pkg/syscall/zerrors_darwin_386.go:1324: 131072: "unknown signal",
On 2012/02/11 01:47:29, iant wrote:
> How did this 131072 entry get in there?  We definitely don't want that.

That's SIGSTKSZ, not to be confused with Linux's SIGSTKSIZE.
Fixed.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
File src/pkg/syscall/zerrors_darwin_amd64.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_darwi...
src/pkg/syscall/zerrors_darwin_amd64.go:1324: 131072: "unknown signal",
On 2012/02/11 01:47:29, iant wrote:
> Another 131072.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
File src/pkg/syscall/zerrors_linux_386.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
src/pkg/syscall/zerrors_linux_386.go:1514: 8192: "unknown signal 8192",
On 2012/02/11 01:47:29, iant wrote:
> This one has an 8192.

Done.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
File src/pkg/syscall/zerrors_linux_amd64.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/syscall/zerrors_linux...
src/pkg/syscall/zerrors_linux_amd64.go:1515: 8192: "unknown signal 8192",
On 2012/02/11 01:47:29, iant wrote:
> Another undesired 8192.

Done.

[rsc, 2012-02-13 17:09:57]

On Fri, Feb 10, 2012 at 20:47,  <iant@golang.org> wrote:
> Looks you can get rid of the restart parameter of runtime·setsig.

That is probably true, but those are in the OS-specific API,
and just in case I need to have a non-restarting handler
at some point, I'd rather leave it in so I don't have to go
back and edit all those copies again later.

Russ

[borman, 2012-02-13 17:24:14]

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:64: default:
I almost feel like there should be a bit mask of dropped signals that can be
returned. 

case h.c <- sig: dropped &= ^(1<<sig)
default: dropped |= (1<<sig)

and a routine to return them.  I just feel a little uncomfortable with having a
solution that can totally miss a signal with no way to know it happened. 
Perhaps this is my OS vending background peeking through.

I guess the other argument is if you application is taking that many signals
then you really should redesign your application rather than make the
infrastructure support it.

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signals_linux.h
File src/pkg/runtime/signals_linux.h (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/runtime/signals_linux...
src/pkg/runtime/signals_linux.h:42: /* 31 */	N, "SIGSYS: bad system call",
Shouldn't this be N+T?

[rsc, 2012-02-13 17:26:56]

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go
File src/pkg/os/signal/signal.go (right):

http://codereview.appspot.com/3749041/diff/2056/src/pkg/os/signal/signal.go#n...
src/pkg/os/signal/signal.go:64: default:
On 2012/02/13 17:24:14, borman wrote:
> I almost feel like there should be a bit mask of dropped signals that can be
> returned. 
> 
> case h.c <- sig: dropped &= ^(1<<sig)
> default: dropped |= (1<<sig)
> 
> and a routine to return them.  I just feel a little uncomfortable with having
a
> solution that can totally miss a signal with no way to know it happened. 
> Perhaps this is my OS vending background peeking through.

Use a separate channel for each signal and give it a buffer size >= 1.
Then you cannot miss any signals, although two SIGHUPs might get
coalesced to one.  The comment on Notify points this out.

> I guess the other argument is if you application is taking that many signals
> then you really should redesign your application rather than make the
> infrastructure support it.

Use a channel with a big enough buffer size.

[rsc, 2012-02-13 18:52:44]

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

os/signal: selective signal handling

Restore package os/signal, with new API:
Notify replaces Incoming, allowing clients
to ask for certain signals only.  Also, signals
go to everyone who asks, not just one client.

This could plausibly move into package os now
that there are no magic side effects as a result
of the import.

Update runtime for new API: move common Unix
signal handling code into signal_unix.c.
(It's so easy to do this now that we don't have
to edit Makefiles!)

Tested on darwin,linux 386,amd64.

Fixes issue 1266.

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