Skip to content

signal: Handle more signals to make ParseSignal containerd compatible#71

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
kzys:windows-signals
Jul 23, 2021
Merged

signal: Handle more signals to make ParseSignal containerd compatible#71
cpuguy83 merged 1 commit intomoby:masterfrom
kzys:windows-signals

Conversation

@kzys
Copy link
Copy Markdown
Member

@kzys kzys commented Jul 23, 2021

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 [email protected]

Comment thread 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),
Copy link
Copy Markdown
Member

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

Suggested change
"SIGILL": syscall.Signal(windows.SIGILL),
"ILL": syscall.Signal(windows.SIGILL),

Copy link
Copy Markdown
Member Author

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.

Comment thread 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),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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 😆

@kzys kzys force-pushed the windows-signals branch from fcc61a7 to 8037ded Compare July 23, 2021 22:11
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 <[email protected]>
Copy link
Copy Markdown
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit 8a51b5c into moby:master Jul 23, 2021
@thaJeztah thaJeztah changed the title Handle more signals to make ParseSignal containerd compatible signal: Handle more signals to make ParseSignal containerd compatible Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants