Skip to content

Use signal map from x/sys/unix #2257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
crosbymichael merged 1 commit into from
Mar 23, 2020
Merged

Use signal map from x/sys/unix #2257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions kill.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"syscall"

"github.com/urfave/cli"
"golang.org/x/sys/unix"
)

var killCommand = cli.Command{
Expand Down Expand Up @@ -60,8 +61,12 @@ func parseSignal(rawSignal string) (syscall.Signal, error) {
if err == nil {
return syscall.Signal(s), nil
}
signal, ok := signalMap[strings.TrimPrefix(strings.ToUpper(rawSignal), "SIG")]
if !ok {
sig := strings.ToUpper(rawSignal)
if !strings.HasPrefix(sig, "SIG") {
sig = "SIG" + sig
}
signal := unix.SignalNum(sig)
if signal == 0 {
Copy link
Member

@crosbymichael crosbymichael Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sig 0 is valid to check if a process is alive

Copy link
Member

@tianon tianon Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mikebrow mikebrow Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mikebrow mikebrow Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member

@thaJeztah thaJeztah Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@kolyshkin kolyshkin Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tklauser tklauser Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

Copy link
Member

@crosbymichael crosbymichael Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i see. this logic should be fine since it's a name lookup

return -1, fmt.Errorf("unknown signal %q", rawSignal)
}
return signal, nil
Expand Down
47 changes: 0 additions & 47 deletions signalmap.go
View file
Open in desktop

This file was deleted.

45 changes: 0 additions & 45 deletions signalmap_mipsx.go
View file
Open in desktop

This file was deleted.