-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try to delete exec fifo file when failure in creation #4319
Try to delete exec fifo file when failure in creation #4319
Conversation
455ce26
to
f7aa220
Compare
Signed-off-by: lifubang <lifubang@acmcoder.com>
f7aa220
to
e729452
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
btw, have you checked all users of createExecFifo() that none is deleting it if it fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
defer func() { | ||
if retErr != nil { | ||
os.Remove(fifoName) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I don't think we need a test case for this as the code is rather obvious
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.