Skip to content

Add SIGRTMIN..SIGRTMAX signals.#1705

Closed
unshare wants to merge 1 commit intoopencontainers:masterfrom
unshare:pr-1
Closed

Add SIGRTMIN..SIGRTMAX signals.#1705
unshare wants to merge 1 commit intoopencontainers:masterfrom
unshare:pr-1

Conversation

@unshare
Copy link
Copy Markdown
Contributor

@unshare unshare commented Jan 25, 2018

Actually, it's ported Moby's signal map. If not added this way, parseSignal returns "unknown signal %q".

Actually, it's ported Moby's signal map. If not added this way, `parseSignal` returns "unknown signal %q".

Signed-off-by: Valentin Kulesh <valentin.kulesh@virtuozzo.com>
@unshare
Copy link
Copy Markdown
Contributor Author

unshare commented Jan 25, 2018

502 (bad gateway) from go.googlesource.com is definitely unrelated CI failure

@crosbymichael
Copy link
Copy Markdown
Member

@unshare restarted the CI for ya

Comment thread signalmap.go

const (
sigrtmin = 34
sigrtmax = 64
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.

It's not a good idea to hardcode these, for two reasons:

  1. glibc uses the top two (or three in some cases) SIGRT signals for NPTL(7), which could cause confusion for users (especially since this means that SIGRTMAX would be different for runc than it is for kill or regular processes).

  2. It's possible that in the future the kernel will support more realtime signals. While this is unlikely, hardcoding this is less optimal than using the SIGRTMIN and SIGRTMAX macros IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Go SDK doesn't provide SIGRTMIN and SIGRTMAX. It could be a good idea to go there first and return here afterwards, but I don't think I have great chances. Docker/Moby didn't do it.

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 could access them through CGo (though it would be a bit ugly).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know. It's how it's done in Go SDK for some OSes (but not for Linux -- for Linux it's hard-coded). It's hard-coded in Python, too. And in Moby/Docker. Different people have different opinions.
But signal map completeness is not the issue. See #1706

Comment thread signalmap.go
"RTMAX-3": sigrtmax - 3,
"RTMAX-2": sigrtmax - 2,
"RTMAX-1": sigrtmax - 1,
"RTMAX": sigrtmax,
Copy link
Copy Markdown
Member

@cyphar cyphar Jan 26, 2018

Choose a reason for hiding this comment

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

I'm not sure I understand the justification of having SIGRTMAX-n signal "names". Almost all usages I've seen of SIGRT have always been either SIGRTMIN+n or SIGRTnn. This matches the man page's recommentation:

programs should never refer to real-time signals using hard-coded numbers, but instead should always refer to real-time signals using the notation SIGRTMIN+n, and include suitable (run-time) checks that SIGRTMIN+n does not exceed SIGRTMAX.

Copy link
Copy Markdown
Contributor Author

@unshare unshare Jan 26, 2018

Choose a reason for hiding this comment

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

These names are the ones used by bash. So it's something users could be familiar with.
Actually, I need these signals under ANY name due to checks I propose to remove in #1706.

I guess, it'd be better to go ahead with #1706 instead, but that's a much more drastic measure.

@unshare
Copy link
Copy Markdown
Contributor Author

unshare commented Jan 26, 2018

Closing in favor of #1706

@unshare unshare closed this Jan 26, 2018
@unshare unshare deleted the pr-1 branch January 26, 2018 07:28
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