libct/specconv: reduce init allocations, use global maps, refactor #3281
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,8 +7,8 @@ import ( | |
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
| systemdDbus "github.com/coreos/go-systemd/v22/dbus" | ||
|
|
@@ -24,26 +24,85 @@ import ( | |
| "golang.org/x/sys/unix" | ||
| ) | ||
|
|
||
| var ( | ||
| initMapsOnce sync.Once | ||
| namespaceMapping map[specs.LinuxNamespaceType]configs.NamespaceType | ||
| mountPropagationMapping map[string]int | ||
| mountFlags, extensionFlags map[string]struct { | ||
| clear bool | ||
| flag int | ||
| } | ||
| ) | ||
|
|
||
| func initMaps() { | ||
| initMapsOnce.Do(func() { | ||
| namespaceMapping = map[specs.LinuxNamespaceType]configs.NamespaceType{ | ||
| specs.PIDNamespace: configs.NEWPID, | ||
| specs.NetworkNamespace: configs.NEWNET, | ||
| specs.MountNamespace: configs.NEWNS, | ||
| specs.UserNamespace: configs.NEWUSER, | ||
| specs.IPCNamespace: configs.NEWIPC, | ||
| specs.UTSNamespace: configs.NEWUTS, | ||
| specs.CgroupNamespace: configs.NEWCGROUP, | ||
| } | ||
|
|
||
| mountPropagationMapping = map[string]int{ | ||
| "rprivate": unix.MS_PRIVATE | unix.MS_REC, | ||
| "private": unix.MS_PRIVATE, | ||
| "rslave": unix.MS_SLAVE | unix.MS_REC, | ||
| "slave": unix.MS_SLAVE, | ||
| "rshared": unix.MS_SHARED | unix.MS_REC, | ||
| "shared": unix.MS_SHARED, | ||
| "runbindable": unix.MS_UNBINDABLE | unix.MS_REC, | ||
| "unbindable": unix.MS_UNBINDABLE, | ||
| "": 0, | ||
| } | ||
|
|
||
| mountFlags = map[string]struct { | ||
| clear bool | ||
| flag int | ||
| }{ | ||
| "acl": {false, unix.MS_POSIXACL}, | ||
| "async": {true, unix.MS_SYNCHRONOUS}, | ||
| "atime": {true, unix.MS_NOATIME}, | ||
| "bind": {false, unix.MS_BIND}, | ||
| "defaults": {false, 0}, | ||
| "dev": {true, unix.MS_NODEV}, | ||
| "diratime": {true, unix.MS_NODIRATIME}, | ||
| "dirsync": {false, unix.MS_DIRSYNC}, | ||
| "exec": {true, unix.MS_NOEXEC}, | ||
| "iversion": {false, unix.MS_I_VERSION}, | ||
| "lazytime": {false, unix.MS_LAZYTIME}, | ||
| "loud": {true, unix.MS_SILENT}, | ||
| "mand": {false, unix.MS_MANDLOCK}, | ||
| "noacl": {true, unix.MS_POSIXACL}, | ||
| "noatime": {false, unix.MS_NOATIME}, | ||
| "nodev": {false, unix.MS_NODEV}, | ||
| "nodiratime": {false, unix.MS_NODIRATIME}, | ||
| "noexec": {false, unix.MS_NOEXEC}, | ||
| "noiversion": {true, unix.MS_I_VERSION}, | ||
| "nolazytime": {true, unix.MS_LAZYTIME}, | ||
| "nomand": {true, unix.MS_MANDLOCK}, | ||
| "norelatime": {true, unix.MS_RELATIME}, | ||
| "nostrictatime": {true, unix.MS_STRICTATIME}, | ||
| "nosuid": {false, unix.MS_NOSUID}, | ||
| "rbind": {false, unix.MS_BIND | unix.MS_REC}, | ||
| "relatime": {false, unix.MS_RELATIME}, | ||
| "remount": {false, unix.MS_REMOUNT}, | ||
| "ro": {false, unix.MS_RDONLY}, | ||
| "rw": {true, unix.MS_RDONLY}, | ||
| "silent": {false, unix.MS_SILENT}, | ||
| "strictatime": {false, unix.MS_STRICTATIME}, | ||
| "suid": {true, unix.MS_NOSUID}, | ||
| "sync": {false, unix.MS_SYNCHRONOUS}, | ||
| } | ||
| extensionFlags = map[string]struct { | ||
| clear bool | ||
| flag int | ||
| }{ | ||
| "tmpcopyup": {false, configs.EXT_COPYUP}, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // AllowedDevices is the set of devices which are automatically included for | ||
|
|
@@ -256,6 +315,8 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { | |
| config.Cgroups = c | ||
| // set linux-specific config | ||
| if spec.Linux != nil { | ||
| initMaps() | ||
|
|
||
| var exists bool | ||
| if config.RootPropagation, exists = mountPropagationMapping[spec.Linux.RootfsPropagation]; !exists { | ||
| return nil, fmt.Errorf("rootfsPropagation=%v is not supported", spec.Linux.RootfsPropagation) | ||
|
|
@@ -332,33 +393,38 @@ func createLibcontainerMount(cwd string, m specs.Mount) (*configs.Mount, error) | |
| // return nil, fmt.Errorf("mount destination %s is not absolute", m.Destination) | ||
| logrus.Warnf("mount destination %s is not absolute. Support for non-absolute mount destinations will be removed in a future release.", m.Destination) | ||
| } | ||
| mnt := parseMountOptions(m.Options) | ||
|
|
||
| mnt.Destination = m.Destination | ||
| mnt.Source = m.Source | ||
| mnt.Device = m.Type | ||
| if mnt.Flags&unix.MS_BIND != 0 { | ||
| // Any "type" the user specified is meaningless (and ignored) for | ||
| // bind-mounts -- so we set it to "bind" because rootfs_linux.go | ||
| // (incorrectly) relies on this for some checks. | ||
| mnt.Device = "bind" | ||
| if !filepath.IsAbs(mnt.Source) { | ||
| mnt.Source = filepath.Join(cwd, m.Source) | ||
| } | ||
| } | ||
| return mnt, nil | ||
| } | ||
|
|
||
| // isValidName checks if systemd property name is valid. It should consists | ||
| // of latin letters only, and have least 3 of them. | ||
| func isValidName(s string) bool { | ||
|
Member
There was a problem hiding this comment. Related to my other comment (no need to fix in this PR); perhaps would be nice to have this return an error with the reason for it being invalid (too short or invalid character) |
||
| if len(s) < 3 { | ||
| return false | ||
| } | ||
| // Check ASCII characters rather than Unicode runes. | ||
| for _, ch := range s { | ||
| if (ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') { | ||
| continue | ||
| } | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // Some systemd properties are documented as having "Sec" suffix | ||
| // (e.g. TimeoutStopSec) but are expected to have "USec" suffix | ||
|
|
@@ -406,11 +472,16 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) { | |
| if err != nil { | ||
| return nil, fmt.Errorf("Annotation %s=%s value parse error: %w", k, v, err) | ||
| } | ||
| // Check for Sec suffix. | ||
| if trimName := strings.TrimSuffix(name, "Sec"); len(trimName) < len(name) { | ||
| // Check for a lowercase ascii a-z just before Sec. | ||
| if ch := trimName[len(trimName)-1]; ch >= 'a' && ch <= 'z' { | ||
| // Convert from Sec to USec. | ||
| name = trimName + "USec" | ||
| value, err = convertSecToUSec(value) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Annotation %s=%s value parse error: %w", k, v, err) | ||
|
Member
There was a problem hiding this comment. Not something new, so probably better to fix separately, but this should've been lowercase
Contributor
Author
There was a problem hiding this comment. Are you aware of any linter for such things? Would be nice to have
Member
There was a problem hiding this comment. I expect the |
||
| } | ||
| } | ||
| } | ||
| sp = append(sp, systemdDbus.Property{Name: name, Value: value}) | ||
|
|
@@ -759,92 +830,38 @@ func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { | |
| return nil | ||
| } | ||
|
|
||
| // parseMountOptions parses options and returns a configs.Mount | ||
| // structure with fields that depends on options set accordingly. | ||
| func parseMountOptions(options []string) *configs.Mount { | ||
| var ( | ||
| data []string | ||
| m configs.Mount | ||
| ) | ||
| initMaps() | ||
| for _, o := range options { | ||
| // If the option does not exist in the mountFlags table, | ||
| // or the flag is not supported on the platform, | ||
| // then it is a data value for a specific fs type. | ||
| if f, exists := mountFlags[o]; exists && f.flag != 0 { | ||
| if f.clear { | ||
| m.Flags &= ^f.flag | ||
| } else { | ||
| m.Flags |= f.flag | ||
| } | ||
| } else if f, exists := mountPropagationMapping[o]; exists && f != 0 { | ||
| m.PropagationFlags = append(m.PropagationFlags, f) | ||
| } else if f, exists := extensionFlags[o]; exists && f.flag != 0 { | ||
| if f.clear { | ||
| m.Extensions &= ^f.flag | ||
| } else { | ||
| m.Extensions |= f.flag | ||
| } | ||
| } else { | ||
| data = append(data, o) | ||
| } | ||
| } | ||
| m.Data = strings.Join(data, ",") | ||
| return &m | ||
| } | ||
|
|
||
| func SetupSeccomp(config *specs.LinuxSeccomp) (*configs.Seccomp, error) { | ||
|
|
||
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
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.
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.
I like the improvement that's brought by this, but (as a follow-up) perhaps we should consider having utilities / accessor functions for these variables, so that the initialisation happens more 'organically' (reducing the cognitive overload of having to thing about first calling
initMap()whenever we need to use one of the variables.In this case it could be a function to check if
fooexists inmountPropagationMappingThere 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.
Do you mean something like
To me it looks like a bloat, since the
nsMap := getNamespaceMapping()is just a way to sugar-coatinitMaps().A runtime assertion would be nice. If only accessing a nil map would panic....
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.
I can't think of anything better than
but it's still inferior to
initMaps()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.
Some kind of a linter to which we can add a rule (in the source code) like "X should be called before using Y" would also be nice to have.
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.
Ideally, a lazy way to initialize variables. IOW same as this PR does, but automatically (perhaps via an annotation).
Uh oh!
There was an error while loading. Please reload this page.
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.
Someone worked on something like this -- alas it does not look good without generics. https://github.com/zetamatta/go-lazy
I guess we have to postpone it till Go 2.0 ))))