Add the ability to parse signals based on a platform string#4793
Add the ability to parse signals based on a platform string#4793katiewasnothere wants to merge 1 commit intocontainerd:mainfrom
Conversation
|
Hi @katiewasnothere. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Build succeeded.
|
katiewasnothere
force-pushed
the
lcow_signal_parse
branch
from
e762ff8 to
cf76555
Compare
|
Build succeeded.
|
katiewasnothere
force-pushed
the
lcow_signal_parse
branch
from
cf76555 to
c96567f
Compare
|
Build succeeded.
|
katiewasnothere
force-pushed
the
lcow_signal_parse
branch
from
c96567f to
21c41fd
Compare
|
Build succeeded.
|
katiewasnothere
force-pushed
the
lcow_signal_parse
branch
from
21c41fd to
c526664
Compare
|
Build succeeded.
|
|
@crosbymichael or @cpuguy83 PTAL when you can. The test failure doesn't look related afaict, but is persistent across runs. Thoughts? |
Signed-off-by: Kathryn Baldauf <[email protected]>
katiewasnothere
force-pushed
the
lcow_signal_parse
branch
from
c526664 to
1159ade
Compare
|
Build succeeded.
|
| func ParsePlatformSignal(rawSignal, platform string) (syscall.Signal, error) { | ||
| return parseSignalGeneric(rawSignal, platform) | ||
| } | ||
|
|
||
| func parseSignalGeneric(rawSignal, platform string) (syscall.Signal, error) { |
There was a problem hiding this comment.
What is the difference between ParsePlatformSignal and parseSignalGeneric?
There was a problem hiding this comment.
parseSignalGeneric is just a helper function. I structured it this way since the unix version of parsing platforms also uses a helper function. It's not really needed, was included just for ease of reading really.
| linuxSigrtmax = 64 | ||
| ) | ||
|
|
||
| var signalMapWindows = map[string]syscall.Signal{ |
There was a problem hiding this comment.
I'm not sure if it's possible to move this part to golang.org/x/sys/windows. I added unix.SignalNum last year. It's better to have same function for windows.
There was a problem hiding this comment.
I'm not sure it would make much sense to do since windows doesn't really have "signals"
|
|
||
| // manually define signals for linux since we may be running an LCOW container, but | ||
| // the unix syscalls do not get built when running on windows | ||
| var signalMapLinux = map[string]syscall.Signal{ |
There was a problem hiding this comment.
Is this copied from somewhere? If so, we should provide a link as a reference.
| } | ||
|
|
||
| func getSignalMapForPlatform(platform string) map[string]syscall.Signal { | ||
| if platform != "windows" { |
There was a problem hiding this comment.
I think it would be better to return an error if we are passed an unrecognized platform. Otherwise we could end up with a bug in the future where the linux signal map is used unintentionally.
| } | ||
| } | ||
| return -1, fmt.Errorf("unknown signal %q", rawSignal) | ||
| return sig, fmt.Errorf("unknown signal %q", rawSignal) |
There was a problem hiding this comment.
Why do we return a valid value along with an error?
There was a problem hiding this comment.
I guess this this would happen if golang successfully looked up the signal number in its private signals table, but it wasn't in our platform-specific map here. (windows table in https://golang.org/src/syscall/types_windows.go)
But yea, returning sig won't help because the caller will see the error is non nil, please return -1 unless there is something else going on?
| if platform != "windows" { | ||
| return sig, nil | ||
| } | ||
| // on windows, make sure we support this signal |
There was a problem hiding this comment.
Is there some specific reason we need to validate this only for Windows?
| } | ||
| signal, ok := signalMap[strings.TrimPrefix(strings.ToUpper(rawSignal), "SIG")] | ||
| if !ok { | ||
| return -1, fmt.Errorf("unknown signal %q", rawSignal) |
There was a problem hiding this comment.
Can you add an extra hint for us by adding is it supported on $platform in the error message here?
|
|
||
| // manually define signals for linux since we may be running an LCOW container, but | ||
| // the unix syscalls do not get built when running on windows | ||
| var signalMapLinux = map[string]syscall.Signal{ |
There was a problem hiding this comment.
I'm kinda sad that this enhanced signal lookup feature won't work on linux!
(I would have to run a linux container on windows to get RTMIN+3 to work)
Can you put this in a generic location (signals.go), should be safe because it has linux in the name already, so it could be used by both signals_windows and signals_linux ? Bonus for making signals_linux actually use it.
7 participants
This PR adds a new call for parsing signals
ParsePlatformSignalthat allows for callers to specify what platform they want the signal to be parsed for. This fixes an issue with LCOW where signals for stopping a linux container are parsed for a windows platform.Signed-off-by: Kathryn Baldauf [email protected]