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.
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
signal: Handle more signals to make ParseSignal containerd compatible #71
Conversation
signal/signal_windows.go
Outdated
"HUP": syscall.Signal(windows.SIGHUP), | ||
"INT": syscall.Signal(windows.SIGINT), | ||
"QUIT": syscall.Signal(windows.SIGQUIT), | ||
"SIGILL": syscall.Signal(windows.SIGILL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably need "ILL", not "SIGILL".
"SIGILL": syscall.Signal(windows.SIGILL), | |
"ILL": syscall.Signal(windows.SIGILL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Fixed in the last revision.
signal/signal_windows.go
Outdated
"HUP": syscall.Signal(windows.SIGHUP), | ||
"INT": syscall.Signal(windows.SIGINT), | ||
"QUIT": syscall.Signal(windows.SIGQUIT), | ||
"SIGILL": syscall.Signal(windows.SIGILL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is SIGSIGILL since 2017. Looks like a typo though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's likely a typo in the original.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially since CI is passing now 😆
We'd like to use moby/sys/signal in containerd, but containerd currently supports more signals. Adding them here closes the gap. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We'd like to use moby/sys/signal in containerd, but containerd currently
supports more signals. Adding them here closes the gap.
Signed-off-by: Kazuyoshi Kato katokazu@amazon.com