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

libct: Don't use stale mount fds #3518

Closed
wants to merge 2 commits into from

Conversation

rata
Copy link
Member

@rata rata commented Jun 20, 2022

This is an alternative fix for the same bug that PR #3510 is tackling. Both PRs should have the same effect (the .fd field is nil when it should).

I'm just opening this PR so people can chose the PR they prefer. Both are correct and easy to backport.

What I like about this is that similar bugs should not happen, as the mountConfig is cleared on every iteration, and should be very simple to backport too.

Please keep me in the loop in the future too, so I can help to fix the bugs that come around :)

The mountConfig fd field can be set by a previous iteration and we are
not clearing it.

Let's just move the mountConfig inside the loop, so we avoid all this
kinds of bugs in the future too. Note that none of the fields set on
mountConfig have secondary effects (they are just reading fields from
config/iConfig), so this is safe to do.

Signed-off-by: Rodrigo Campos <[email protected]>
@rata
Copy link
Member Author

rata commented Jun 20, 2022

cc @kolyshkin

@rata rata force-pushed the rata/stale-mount-fds branch from ba63cae to 9bba111 Compare June 20, 2022 14:58
@rata
Copy link
Member Author

rata commented Jun 20, 2022

I've also verified that the test passes with the fix and fails without (see the revert commit that I removed now)

This was created by kolyshkin in:
	opencontainers#3510

Signed-off-by: Rodrigo Campos <[email protected]>
@kolyshkin
Copy link
Contributor

I think that #3512 is a better way long term

@rata rata closed this Jun 23, 2022
@rata rata deleted the rata/stale-mount-fds branch June 23, 2022 09:39
@rata
Copy link
Member Author

rata commented Jun 23, 2022

Sure, closing then!

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.

2 participants