libct/userns: assorted (godoc) improvements#4330
Merged
kolyshkin merged 3 commits intoopencontainers:mainfrom Jul 1, 2024
Merged
libct/userns: assorted (godoc) improvements#4330kolyshkin merged 3 commits intoopencontainers:mainfrom
kolyshkin merged 3 commits intoopencontainers:mainfrom
Conversation
AkihiroSuda
approved these changes
Jun 30, 2024
kolyshkin
reviewed
Jul 1, 2024
Contributor
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Typo in commit message: s/ asd / as /.
This was a poor decision on my side; 4316df8 moved this utility to a separate package, and split the exported function from the implementation (and stubs). Out of convenience, I used an alias for the latter part, but there's two downsides to that; - `RunningInUserNS` being an exported var means that (technically) it can be replaced by other code; perhaps that's a "feature", but not one we intended it to be used for. - `RunningInUserNS` being implemented through a var / alias means it's also documented as such on [pkg.go.dev], which is confusing. This patch changes it to a regular function, acting as a wrapper for the underlying implementations. While at it, also slightly touching up the GoDoc to describe its functionality / behavior. [pkg.go.dev]: https://pkg.go.dev/github.com/opencontainers/[email protected]/libcontainer/userns#RunningInUserNS Signed-off-by: Sebastiaan van Stijn <[email protected]>
The fuzzer for this only runs on Linux; rename the file to be Linux-only so that we don't have to stub out the uidMapInUserNS function. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Now that we dropped support for go < 1.21, we can use this; moving the sync.once out of the runningInUserNS() implementation would also allow for it to be more easily tested if we'd decide to. Signed-off-by: Sebastiaan van Stijn <[email protected]>
kolyshkin
reviewed
Jul 1, 2024
Comment on lines
1
to
2
| //go:build linux && gofuzz | ||
| // +build linux,gofuzz |
Member
Author
There was a problem hiding this comment.
Thanks! Just did; also fixed the commit message 👍 🫶
kolyshkin
approved these changes
Jul 1, 2024
Contributor
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
lgtm aside from a couple of nits
Member
Author
|
I think CentOS 7 may be really gone now? |
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
libct/userns: change RunningInUserNS to a wrapper instead of an alias
This was a poor decision on my side; 4316df8 (#2850)
moved this utility to a separate package, and split the exported function
from the implementation (and stubs). Out of convenience, I used an alias
for the latter part, but there's two downsides to that;
RunningInUserNSbeing an exported var means that (technically) it canbe replaced by other code; perhaps that's a "feature", but not one we
intended it to be used for.
RunningInUserNSbeing implemented through a var / alias means it'salso documented as such on pkg.go.dev, which is confusing.
This patch changes it to a regular function, acting as a wrapper for
the underlying implementations. While at it, also slightly touching
up the GoDoc to describe its functionality / behavior.
libct/userns: make fuzzer Linux-only, and remove stub for uidMapInUserNS
The fuzzer for this only runs on Linux; rename the file to be Linux-only
so that we don't have to stub out the uidMapInUserNS function.
libct/userns: implement RunningInUserNS with sync.OnceValue
Now that we dropped support for go < 1.21, we can use this; moving
the sync.once out of the runningInUserNS() implementation would also
allow for it to be more easily tested if we'd decide to.