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

oci/caps: refactor, remove unused code, and improved error messages #41459

Merged
merged 7 commits into from Aug 9, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Sep 16, 2020

  • oci/caps: rename some vars that conflicted with imports / built-ins
  • oci/caps: minor optimization in init
  • oci/caps: generate list of all capabilities on "init"
  • oci/caps: use map for capabilities to simplify lookup
  • oci/caps: improve error message for unsupported capabilities
    A capability can either be invalid, or not supported by the kernel on which we're running. This patch changes the error message produced to reflect if the capability is invalid/unknown, or a known capability, but not supported by the kernel version.
  • oci/caps: remove unused GetCapability() and ValidateCapabilities()
  • oci/caps: simplify, and remove types that were not needed
    The CapabilityMapping and Capabilities types appeared to be only used locally, and added unneeded complexity. This patch removes those types, and simplifies the logic to use a map that maps names to capability.Caps
  • oci/caps: remove hack for RHEL6 kernels
    We no longer support these kernels, so we can remove the workaround

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Sep 16, 2020
@thaJeztah thaJeztah marked this pull request as draft September 17, 2020 18:44
@thaJeztah thaJeztah force-pushed the caps_refactor branch 3 times, most recently from 8606000 to d099749 Compare September 19, 2020 12:55
@thaJeztah thaJeztah changed the title oci/caps: small optimizations, and improved error messages oci/caps: refactor, remove unused code, and improved error messages Sep 19, 2020
@thaJeztah thaJeztah marked this pull request as ready for review September 19, 2020 15:08
@thaJeztah
Copy link
Member Author

thaJeztah commented Sep 19, 2020

@kolyshkin @AkihiroSuda ptal

@thaJeztah
Copy link
Member Author

@cpuguy83 @tonistiigi ptal 🤗

Comment on lines 24 to 33
allCaps = make([]string, capability.CAP_LAST_CAP+1)
capabilityList = make(map[string]*capability.Cap, len(capability.List()))
for i, c := range capability.List() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of capability.List() appears to generate/return a new slice of the full list, so it seems a tiny bit wasteful to call twice -- should probably go into a variable for len() vs for?

Suggested change
allCaps = make([]string, capability.CAP_LAST_CAP+1)
capabilityList = make(map[string]*capability.Cap, len(capability.List()))
for i, c := range capability.List() {
allCaps = make([]string, capability.CAP_LAST_CAP+1)
rawCaps := capability.List()
capabilityList = make(map[string]*capability.Cap, len(rawCaps))
for i, c := range rawCaps {

Does it also make sense to use len(rawCaps) for the size of allCaps, given that we do allCaps[i] = ... below, which will be based on the number of capabilities total, not on the maximum value? (or is that an implementation bug that should've been allCaps[c] instead? Looking at how it gets used/returned, it doesn't seem like a bug so it looks like we're allocating more space than we actually need here. 😅)

Suggested change
allCaps = make([]string, capability.CAP_LAST_CAP+1)
capabilityList = make(map[string]*capability.Cap, len(capability.List()))
for i, c := range capability.List() {
rawCaps := capability.List()
allCaps = make([]string, len(rawCaps))
capabilityList = make(map[string]*capability.Cap, len(rawCaps))
for i, c := range rawCaps {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at how it gets used/returned, it doesn't seem like a bug so it looks like we're allocating more space than we actually need here

Hmmm good point, so I don't think it's a bug (but I do take advantage of the fact that all the caps are in sequential order as proxy for "length") as CAP_LAST_CAP should be <= len(capability.List();

f, err := os.Open("/proc/sys/kernel/cap_last_cap")

Hmmm... although

  • it's initially set to Cap(63), and will be if the code above check fails
  • newer kernels may support more capabilities than what the library supports, so we should check something like math.Min(len(rawCaps), CAP_LAST_CAP+1)

capName := "CAP_" + strings.ToUpper(c.String())
if c > capability.CAP_LAST_CAP {
capabilityList[capName] = nil
continue
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we reach CAP_LAST_CAP we continue here, so allCaps will no longer get more capabilities after that

@thaJeztah
Copy link
Member Author

@tianon updated; let me know if this is what you meant/pointed out

oci/caps/utils.go Outdated Show resolved Hide resolved
We no longer support these kernels, so we can remove the workaround

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
A capability can either be invalid, or not supported by the kernel
on which we're running. This patch changes the error message produced
to reflect if the capability is invalid/unknown, or a known capability,
but not supported by the kernel version.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `CapabilityMapping` and `Capabilities` types appeared to be only
used locally, and added unneeded complexity.

This patch removes those types, and simplifies the logic to use a
map that maps names to `capability.Cap`s

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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

@thaJeztah thaJeztah added this to the 21.xx milestone Aug 6, 2021
@thaJeztah thaJeztah merged commit f91b0d3 into moby:master Aug 9, 2021
@thaJeztah thaJeztah deleted the caps_refactor branch August 9, 2021 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants