Try to delete exec fifo file when failure in creation#4319
Try to delete exec fifo file when failure in creation#4319kolyshkin merged 1 commit intoopencontainers:mainfrom
Conversation
lifubang
force-pushed
the
fix-execfifo-delete-error
branch
from
455ce26 to
f7aa220
Compare
Signed-off-by: lifubang <lifubang@acmcoder.com>
lifubang
force-pushed
the
fix-execfifo-delete-error
branch
from
f7aa220 to
e729452
Compare
lifubang
added
easy-to-review
backport/1.1-todo
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
Wait, don't we need to delete it in the caller too, if createExecFifo() succeeds and some other thing in the callers fails and we abort it all?
Has already deleted it if other logic in the caller fails.(Indeed only one caller at this time) |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
lgtm
Does it affect someone practically? If yes, maybe it makes sense to backport to 1.1
No, I have not seen any submit issues about this. So remove the backpaort label. |
lifubang
removed
the
backport/1.1-todo
| defer func() { | ||
| if retErr != nil { | ||
| os.Remove(fifoName) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Do you want to add a unit test for this? The value is questionable, though. Maybe on refactors it will help.
I think making mkfifo fail will just be a matter of removing the permissions on the statedir
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
lgtm. I don't think we need a test case for this as the code is rather obvious
3 participants
There is a small possibility error after the exec fifo file has created, so we should try to delete it if we get an error from
createExecFifo.This is a possible bug discovered when doing code review, I think we have not meet it in daily usage.