runc
runc copied to clipboard
opencontainers
skip setup signal notifier for detached container
For detached container, we don't need to setup signal notifier, because there is no customer to consume the signals in forward().
But, if notifySocker is non-nil, we set up signal forwarding and when we do forward signals for some short time (until we exit).
I don’t think so, if notifySocket is non-nil, we also don’t consume any signals. There are two places checking detach in ‘forward()’:
- https://github.com/opencontainers/runc/blob/b2d5924e106c2f17c23d5e8211cefcb7085a4836/signals.go#L73-L75
- https://github.com/opencontainers/runc/blob/b2d5924e106c2f17c23d5e8211cefcb7085a4836/signals.go#L82-L86
The signal consumer is in: https://github.com/opencontainers/runc/blob/b2d5924e106c2f17c23d5e8211cefcb7085a4836/signals.go#L95
If I miss something, please tell me.
Ah, yes, you are right, if detach is true, it exits either way. But the code around notifySocket is spaghetti (in a bad way).
yet I wish someone can refactor the code dealing with notifySocket, it seems it is orthogonal to signals and yet it is mixed together
Refactored. I think we should setup notify socket first before we handler and forward signals for non detached containers, so I didn't split them to two functions, but refactor to keep one path for detached container. PTAL
However, it feels like several different things are changed here. If you can separate your patches as explained there, IMHO it will greatly simplify review.
Oh, yes, these codes were written during my business trip. I split it into two commits now, do you think it's more clear now?
to disambiguate the use or
err.So it seems that this is a bug?
Probably not, I just don't like using err in a defer -- we mean to check what the function will return, not what the value of some variable named err is. I mean, if someone will add a code like
return -1, errors.New("oops")
the check in defer won't work properly.
So, this code is error-prone as it is. It won't be if we name a return variable and check it.
Probably not
This is a bug for sometimes, for example: https://github.com/opencontainers/runc/blob/main/utils_linux.go#L264
Had another look, left this comment. The diff says it is outdated, that is why I'm highlighting it here, but it seems to be still relevant.
