-
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
libct/userns: assorted (godoc) improvements #4330
Conversation
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.
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]>
//go:build linux && gofuzz | ||
// +build linux,gofuzz |
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.
Needs a rebase as #4329 is now merged
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.
Thanks! Just did; also fixed the commit message 👍 🫶
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 aside from a couple of nits
I think CentOS 7 may be really gone now?
|
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;
RunningInUserNS
being 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.
RunningInUserNS
being 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.