Use signal map from x/sys/unix #2257
Conversation
Since [1], x/sys/unix has a function to convert signal name to a number. Let's use it and drop home-grown signal map. While at it, add a test case. [1] golang/sys@d455e41 Signed-off-by: Kir Kolyshkin <[email protected]>
|
LGTM |
| sig = "SIG" + sig | ||
| } | ||
| signal := unix.SignalNum(sig) | ||
| if signal == 0 { |
There was a problem hiding this comment.
sig 0 is valid to check if a process is alive
There was a problem hiding this comment.
Dropping the knowledge bombs 😄
Looks like an oversight in the golang.org/x/sys/unix library's SignalNum library: (https://godoc.org/golang.org/x/sys/unix#SignalNum)
SignalNum returns the syscall.Signal for signal named s, or 0 if a signal with such name is not found. The signal name should start with "SIG".
Given that there's not a name for "SIG zero" and that we catch the explicit "numeric input" case further up, this should be reasonable, right?
There was a problem hiding this comment.
reasonable I think until someone adds the equivalent of a new SIGNONE or SIGPING text name option to the list of kill signals and someone adds it to golang unix.SignalNum() :-0
There was a problem hiding this comment.
maybe a comment line or two explaining why the name lookup should fail for zero for now even though the raw 0 option should not fail..
There was a problem hiding this comment.
reasonable I think until someone adds the equivalent of a new SIGNONE or SIGPING text name option to the list of kill signals and someone adds it to golang unix.SignalNum()
@tklauser fyi (discussion above)
There was a problem hiding this comment.
sig 0 is valid to check if a process is alive
but it does not have a name, so...
@mikebrow I'm not sure if adding any comments to the code will improve it. It's pretty clear anyway -- we treat it as a number first, and pass on any valid number to Syscall.Signal(), letting it return an error if the number is wrong.
If it's not a number, we treat it as a name and use unix.SignalNum to resolve. The doc for the function clearly states that it returns 0 if a signal with such name is not found, so we check for 0.
All that is very clear from the code.
There was a problem hiding this comment.
reasonable I think until someone adds the equivalent of a new SIGNONE or SIGPING text name option to the list of kill signals and someone adds it to golang unix.SignalNum()
@tklauser fyi (discussion above)
That's indeed an oversight in x/sys/unix. The better option would have been to return -1 in case no signal with the given name is found. But I think, we probably cannot change that now as we would break existing users :(
There was a problem hiding this comment.
ok, i see. this logic should be fine since it's a name lookup
|
LGTM |
8 participants
Remove signalmap, use unix.SignalNum
Since golang/sys@d455e41, x/sys/unix has a function to convert signal name
to a number. Let's use it and drop home-grown signal map.
While at it, add a test case.
### runc kill: check numeric signals for sanityIf a signal is specified as a number, perform an additionalcheck that the signal is known, otherwise return an error.Add a test case for it.