Nothing Special   »   [go: up one dir, main page]

Skip to content
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

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

lifubang
Copy link
Member
@lifubang lifubang commented Jun 10, 2024

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.

@lifubang lifubang force-pushed the fix-execfifo-delete-error branch from 455ce26 to f7aa220 Compare June 10, 2024 11:46
Signed-off-by: lifubang <lifubang@acmcoder.com>
@lifubang lifubang force-pushed the fix-execfifo-delete-error branch from f7aa220 to e729452 Compare June 10, 2024 12:05
@lifubang lifubang added easy-to-review backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 labels Jun 10, 2024
Copy link
Member
@rata rata left a 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?

Copy link
Member
@rata rata left a 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?

@lifubang
Copy link
Member Author

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)

Copy link
Contributor
@kolyshkin kolyshkin left a 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

@lifubang
Copy link
Member Author

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 lifubang removed the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Jun 11, 2024
Comment on lines +426 to +430
defer func() {
if retErr != nil {
os.Remove(fifoName)
}
}()
Copy link
Member

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

Copy link
Contributor
@kolyshkin kolyshkin left a 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

@kolyshkin kolyshkin merged commit ab010ae into opencontainers:main Jun 26, 2024
39 checks passed
@lifubang lifubang mentioned this pull request Aug 31, 2024
@lifubang lifubang deleted the fix-execfifo-delete-error branch October 15, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants