From 7149781fd091e76e1d45043088f1295148a6e884 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 22:36:26 -0700 Subject: [PATCH 01/16] exec: use strings.Cut to parse --cgroup Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). This part of code is covered by tests in tests/integration/exec.bats. [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- exec.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/exec.go b/exec.go index 16bbeebfb..b896b4b56 100644 --- a/exec.go +++ b/exec.go @@ -124,20 +124,17 @@ func getSubCgroupPaths(args []string) (map[string]string, error) { paths := make(map[string]string, len(args)) for _, c := range args { // Split into controller:path. - cs := strings.SplitN(c, ":", 3) - if len(cs) > 2 { - return nil, fmt.Errorf("invalid --cgroup argument: %s", c) - } - if len(cs) == 1 { // no controller: prefix + if ctr, path, ok := strings.Cut(c, ":"); ok { + // There may be a few comma-separated controllers. + for _, ctrl := range strings.Split(ctr, ",") { + paths[ctrl] = path + } + } else { + // No controller: prefix. if len(args) != 1 { return nil, fmt.Errorf("invalid --cgroup argument: %s (missing : prefix)", c) } paths[""] = c - } else { - // There may be a few comma-separated controllers. - for _, ctrl := range strings.Split(cs[0], ",") { - paths[ctrl] = cs[1] - } } } return paths, nil From 871d9186ee65b86526c6046442b86fe6411cf3d2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 15:02:59 -0800 Subject: [PATCH 02/16] exec: improve getSubCgroupPaths 1. Document the function. 2. Add sanity checks for empty and repeated controllers. Reported-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- exec.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/exec.go b/exec.go index b896b4b56..e33b8d44a 100644 --- a/exec.go +++ b/exec.go @@ -117,6 +117,12 @@ following will output a list of processes running in the container: SkipArgReorder: true, } +// getSubCgroupPaths parses --cgroup arguments, which can either be +// - a single "path" argument (for cgroup v2); +// - one or more controller[,controller[,...]]:path arguments (for cgroup v1). +// +// Returns a controller to path map. For cgroup v2, it's a single entity map +// with empty controller value. func getSubCgroupPaths(args []string) (map[string]string, error) { if len(args) == 0 { return nil, nil @@ -127,10 +133,16 @@ func getSubCgroupPaths(args []string) (map[string]string, error) { if ctr, path, ok := strings.Cut(c, ":"); ok { // There may be a few comma-separated controllers. for _, ctrl := range strings.Split(ctr, ",") { + if ctrl == "" { + return nil, fmt.Errorf("invalid --cgroup argument: %s (empty prefix)", c) + } + if _, ok := paths[ctrl]; ok { + return nil, fmt.Errorf("invalid --cgroup argument(s): controller %s specified multiple times", ctrl) + } paths[ctrl] = path } } else { - // No controller: prefix. + // No "controller:" prefix (cgroup v2, a single path). if len(args) != 1 { return nil, fmt.Errorf("invalid --cgroup argument: %s (missing : prefix)", c) } From bfcd479c5d78412da7c816bba1ac20995c71ee05 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 22:38:00 -0700 Subject: [PATCH 03/16] libct/cg/fs: getPercpuUsage: rm TODO Nowadays strings.Fields are as fast as strings.SplitN so remove TODO. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/cpuacct.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libcontainer/cgroups/fs/cpuacct.go b/libcontainer/cgroups/fs/cpuacct.go index 88af8c568..01a5bc336 100644 --- a/libcontainer/cgroups/fs/cpuacct.go +++ b/libcontainer/cgroups/fs/cpuacct.go @@ -112,7 +112,6 @@ func getPercpuUsage(path string) ([]uint64, error) { if err != nil { return percpuUsage, err } - // TODO: use strings.SplitN instead. for _, value := range strings.Fields(data) { value, err := strconv.ParseUint(value, 10, 64) if err != nil { From 4271ecf73fc3a7777c4898e8609554670f3e43c2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 22:58:00 -0700 Subject: [PATCH 04/16] libct/cg/fs: refactor getCpusetStat Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). This also drops the check for extra dash (we're unlikely to get it from the kernel anyway). While at it, rename min/max -> from/to to avoid collision with Go min/max builtins. This code is tested by TestCPUSetStats* tests. [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/cpuset.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index 4cf1f30ff..b51413ba8 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -48,26 +48,23 @@ func getCpusetStat(path string, file string) ([]uint16, error) { } for _, s := range strings.Split(fileContent, ",") { - sp := strings.SplitN(s, "-", 3) - switch len(sp) { - case 3: - return extracted, &parseError{Path: path, File: file, Err: errors.New("extra dash")} - case 2: - min, err := strconv.ParseUint(sp[0], 10, 16) + fromStr, toStr, ok := strings.Cut(s, "-") + if ok { + from, err := strconv.ParseUint(fromStr, 10, 16) if err != nil { return extracted, &parseError{Path: path, File: file, Err: err} } - max, err := strconv.ParseUint(sp[1], 10, 16) + to, err := strconv.ParseUint(toStr, 10, 16) if err != nil { return extracted, &parseError{Path: path, File: file, Err: err} } - if min > max { - return extracted, &parseError{Path: path, File: file, Err: errors.New("invalid values, min > max")} + if from > to { + return extracted, &parseError{Path: path, File: file, Err: errors.New("invalid values, from > to")} } - for i := min; i <= max; i++ { + for i := from; i <= to; i++ { extracted = append(extracted, uint16(i)) } - case 1: + } else { value, err := strconv.ParseUint(s, 10, 16) if err != nil { return extracted, &parseError{Path: path, File: file, Err: err} From 075cea3a4563966e122c11d9111d2696f7f5d01a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:13:41 -0700 Subject: [PATCH 05/16] libcontainer/cgroups/fs: some refactoring Remove extra global constants that are only used in a single place and make it harder to read the code. Rename nanosecondsInSecond -> nsInSec. This code is tested by unit tests. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/cpuacct.go | 28 ++++++++++--------------- libcontainer/cgroups/fs/cpuacct_test.go | 8 +++---- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/libcontainer/cgroups/fs/cpuacct.go b/libcontainer/cgroups/fs/cpuacct.go index 01a5bc336..53ffbb8e6 100644 --- a/libcontainer/cgroups/fs/cpuacct.go +++ b/libcontainer/cgroups/fs/cpuacct.go @@ -11,14 +11,7 @@ import ( ) const ( - cgroupCpuacctStat = "cpuacct.stat" - cgroupCpuacctUsageAll = "cpuacct.usage_all" - - nanosecondsInSecond = 1000000000 - - userModeColumn = 1 - kernelModeColumn = 2 - cuacctUsageAllColumnsNumber = 3 + nsInSec = 1000000000 // The value comes from `C.sysconf(C._SC_CLK_TCK)`, and // on Linux it's a constant which is safe to be hard coded, @@ -80,7 +73,7 @@ func getCpuUsageBreakdown(path string) (uint64, uint64, error) { const ( userField = "user" systemField = "system" - file = cgroupCpuacctStat + file = "cpuacct.stat" ) // Expected format: @@ -102,7 +95,7 @@ func getCpuUsageBreakdown(path string) (uint64, uint64, error) { return 0, 0, &parseError{Path: path, File: file, Err: err} } - return (userModeUsage * nanosecondsInSecond) / clockTicks, (kernelModeUsage * nanosecondsInSecond) / clockTicks, nil + return (userModeUsage * nsInSec) / clockTicks, (kernelModeUsage * nsInSec) / clockTicks, nil } func getPercpuUsage(path string) ([]uint64, error) { @@ -125,7 +118,7 @@ func getPercpuUsage(path string) ([]uint64, error) { func getPercpuUsageInModes(path string) ([]uint64, []uint64, error) { usageKernelMode := []uint64{} usageUserMode := []uint64{} - const file = cgroupCpuacctUsageAll + const file = "cpuacct.usage_all" fd, err := cgroups.OpenFile(path, file, os.O_RDONLY) if os.IsNotExist(err) { @@ -139,22 +132,23 @@ func getPercpuUsageInModes(path string) ([]uint64, []uint64, error) { scanner.Scan() // skipping header line for scanner.Scan() { - lineFields := strings.SplitN(scanner.Text(), " ", cuacctUsageAllColumnsNumber+1) - if len(lineFields) != cuacctUsageAllColumnsNumber { + // Each line is: cpu user system + fields := strings.SplitN(scanner.Text(), " ", 3) + if len(fields) != 3 { continue } - usageInKernelMode, err := strconv.ParseUint(lineFields[kernelModeColumn], 10, 64) + user, err := strconv.ParseUint(fields[1], 10, 64) if err != nil { return nil, nil, &parseError{Path: path, File: file, Err: err} } - usageKernelMode = append(usageKernelMode, usageInKernelMode) + usageUserMode = append(usageUserMode, user) - usageInUserMode, err := strconv.ParseUint(lineFields[userModeColumn], 10, 64) + kernel, err := strconv.ParseUint(fields[2], 10, 64) if err != nil { return nil, nil, &parseError{Path: path, File: file, Err: err} } - usageUserMode = append(usageUserMode, usageInUserMode) + usageKernelMode = append(usageKernelMode, kernel) } if err := scanner.Err(); err != nil { return nil, nil, &parseError{Path: path, File: file, Err: err} diff --git a/libcontainer/cgroups/fs/cpuacct_test.go b/libcontainer/cgroups/fs/cpuacct_test.go index d7d926d50..116454e48 100644 --- a/libcontainer/cgroups/fs/cpuacct_test.go +++ b/libcontainer/cgroups/fs/cpuacct_test.go @@ -53,8 +53,8 @@ func TestCpuacctStats(t *testing.T) { 962250696038415, 981956408513304, 1002658817529022, 994937703492523, 874843781648690, 872544369885276, 870104915696359, 870202363887496, }, - UsageInKernelmode: (uint64(291429664) * nanosecondsInSecond) / clockTicks, - UsageInUsermode: (uint64(452278264) * nanosecondsInSecond) / clockTicks, + UsageInKernelmode: (uint64(291429664) * nsInSec) / clockTicks, + UsageInUsermode: (uint64(452278264) * nsInSec) / clockTicks, } if !reflect.DeepEqual(expectedStats, actualStats.CpuStats.CpuUsage) { @@ -86,8 +86,8 @@ func TestCpuacctStatsWithoutUsageAll(t *testing.T) { }, PercpuUsageInKernelmode: []uint64{}, PercpuUsageInUsermode: []uint64{}, - UsageInKernelmode: (uint64(291429664) * nanosecondsInSecond) / clockTicks, - UsageInUsermode: (uint64(452278264) * nanosecondsInSecond) / clockTicks, + UsageInKernelmode: (uint64(291429664) * nsInSec) / clockTicks, + UsageInUsermode: (uint64(452278264) * nsInSec) / clockTicks, } if !reflect.DeepEqual(expectedStats, actualStats.CpuStats.CpuUsage) { From 037668e501c4be1db03dce4bde8b0bab82c35f8a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:20:18 -0700 Subject: [PATCH 06/16] libct/cg/fs2: simplify parseCgroupFromReader For cgroup v2, we always expect /proc/$PID/cgroup contents like this: > 0::/user.slice/user-1000.slice/user@1000.service/app.slice/vte-spawn-f71c3fb8-519d-4e2d-b13e-9252594b1e05.scope So, it does not make sense to parse it using strings.Split, we can just cut the prefix and return the rest. Code tested by TestParseCgroupFromReader. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/defaultpath.go | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/libcontainer/cgroups/fs2/defaultpath.go b/libcontainer/cgroups/fs2/defaultpath.go index 6637ef85c..6c44fd197 100644 --- a/libcontainer/cgroups/fs2/defaultpath.go +++ b/libcontainer/cgroups/fs2/defaultpath.go @@ -83,16 +83,9 @@ func parseCgroupFile(path string) (string, error) { func parseCgroupFromReader(r io.Reader) (string, error) { s := bufio.NewScanner(r) for s.Scan() { - var ( - text = s.Text() - parts = strings.SplitN(text, ":", 3) - ) - if len(parts) < 3 { - return "", fmt.Errorf("invalid cgroup entry: %q", text) - } - // text is like "0::/user.slice/user-1001.slice/session-1.scope" - if parts[0] == "0" && parts[1] == "" { - return parts[2], nil + // "0::/user.slice/user-1001.slice/session-1.scope" + if path, ok := strings.CutPrefix(s.Text(), "0::"); ok { + return path, nil } } if err := s.Err(); err != nil { From 40ce69cc9e155e794fcaa4501e3c6704aead9401 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:25:41 -0700 Subject: [PATCH 07/16] libct/cg/fs2: use strings.Cut in setUnified Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). The code is tested by testCgroupResourcesUnified. [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/fs2.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index 74dfcfdaf..10e4a3ad8 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -237,11 +237,10 @@ func (m *Manager) setUnified(res map[string]string) error { if errors.Is(err, os.ErrPermission) || errors.Is(err, os.ErrNotExist) { // Check if a controller is available, // to give more specific error if not. - sk := strings.SplitN(k, ".", 2) - if len(sk) != 2 { + c, _, ok := strings.Cut(k, ".") + if !ok { return fmt.Errorf("unified resource %q must be in the form CONTROLLER.PARAMETER", k) } - c := sk[0] if _, ok := m.controllers[c]; !ok && c != "cgroup" { return fmt.Errorf("unified resource %q can't be set: controller %q not available", k, c) } From 930cd4944ad8fc6273effbd3dd3f5f66a57ba9d5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:29:51 -0700 Subject: [PATCH 08/16] libct/cg/fs2: use strings.Cut in parsePSIData Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). This code is tested by TestStatCPUPSI. [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/psi.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libcontainer/cgroups/fs2/psi.go b/libcontainer/cgroups/fs2/psi.go index 09f348885..ac765affb 100644 --- a/libcontainer/cgroups/fs2/psi.go +++ b/libcontainer/cgroups/fs2/psi.go @@ -58,12 +58,12 @@ func statPSI(dirPath string, file string) (*cgroups.PSIStats, error) { func parsePSIData(psi []string) (cgroups.PSIData, error) { data := cgroups.PSIData{} for _, f := range psi { - kv := strings.SplitN(f, "=", 2) - if len(kv) != 2 { + key, val, ok := strings.Cut(f, "=") + if !ok { return data, fmt.Errorf("invalid psi data: %q", f) } var pv *float64 - switch kv[0] { + switch key { case "avg10": pv = &data.Avg10 case "avg60": @@ -71,16 +71,16 @@ func parsePSIData(psi []string) (cgroups.PSIData, error) { case "avg300": pv = &data.Avg300 case "total": - v, err := strconv.ParseUint(kv[1], 10, 64) + v, err := strconv.ParseUint(val, 10, 64) if err != nil { - return data, fmt.Errorf("invalid %s PSI value: %w", kv[0], err) + return data, fmt.Errorf("invalid %s PSI value: %w", key, err) } data.Total = v } if pv != nil { - v, err := strconv.ParseFloat(kv[1], 64) + v, err := strconv.ParseFloat(val, 64) if err != nil { - return data, fmt.Errorf("invalid %s PSI value: %w", kv[0], err) + return data, fmt.Errorf("invalid %s PSI value: %w", key, err) } *pv = v } From e9855bdae9918685048423c3a17d7a22b9797686 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:34:03 -0700 Subject: [PATCH 09/16] libct/cg/fscommon: use strings.Cut in RDMA parser Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). Also, use switch in parseRdmaKV. [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/rdma.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/libcontainer/cgroups/fscommon/rdma.go b/libcontainer/cgroups/fscommon/rdma.go index 93e05017c..70d92a000 100644 --- a/libcontainer/cgroups/fscommon/rdma.go +++ b/libcontainer/cgroups/fscommon/rdma.go @@ -17,14 +17,12 @@ import ( func parseRdmaKV(raw string, entry *cgroups.RdmaEntry) error { var value uint32 - parts := strings.SplitN(raw, "=", 3) + k, v, ok := strings.Cut(raw, "=") - if len(parts) != 2 { + if !ok { return errors.New("Unable to parse RDMA entry") } - k, v := parts[0], parts[1] - if v == "max" { value = math.MaxUint32 } else { @@ -34,9 +32,10 @@ func parseRdmaKV(raw string, entry *cgroups.RdmaEntry) error { } value = uint32(val64) } - if k == "hca_handle" { + switch k { + case "hca_handle": entry.HcaHandles = value - } else if k == "hca_object" { + case "hca_object": entry.HcaObjects = value } From f134871206dc85bb104459f6b0f0439699973f3e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Oct 2024 23:39:20 -0700 Subject: [PATCH 10/16] libct/cg/fscommon: ParseKeyValue: use strings.Cut Using strings.Cut (added in Go 1.18, see [1]) results in faster and cleaner code with less allocations (as we're not using a slice). [1]: https://github.com/golang/go/issues/46336 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index f4a51c9e5..d7cc23bfd 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -54,22 +54,22 @@ func ParseUint(s string, base, bitSize int) (uint64, error) { return value, nil } -// ParseKeyValue parses a space-separated "name value" kind of cgroup +// ParseKeyValue parses a space-separated "key value" kind of cgroup // parameter and returns its key as a string, and its value as uint64 -// (ParseUint is used to convert the value). For example, +// (using [ParseUint] to convert the value). For example, // "io_service_bytes 1234" will be returned as "io_service_bytes", 1234. func ParseKeyValue(t string) (string, uint64, error) { - parts := strings.SplitN(t, " ", 3) - if len(parts) != 2 { + key, val, ok := strings.Cut(t, " ") + if !ok { return "", 0, fmt.Errorf("line %q is not in key value format", t) } - value, err := ParseUint(parts[1], 10, 64) + value, err := ParseUint(val, 10, 64) if err != nil { return "", 0, err } - return parts[0], value, nil + return key, value, nil } // GetValueByKey reads a key-value pairs from the specified cgroup file, From d83d533bba70ac094cd3a1ef8851fbfe3516e954 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 13:58:24 -0800 Subject: [PATCH 11/16] libct/cg/fscommon: GetValueByKey: use strings.CutPrefix Using strings.CutPrefix (added in Go 1.20, see [1]) results in faster and cleaner code with less allocations (as the code only allocates memory for the value, and does it once). While at it, improve the function documentation. [1]: https://github.com/golang/go/issues/42537 Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index d7cc23bfd..76d06e498 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -72,20 +72,21 @@ func ParseKeyValue(t string) (string, uint64, error) { return key, value, nil } -// GetValueByKey reads a key-value pairs from the specified cgroup file, -// and returns a value of the specified key. ParseUint is used for value -// conversion. +// GetValueByKey reads space-separated "key value" pairs from the specified +// cgroup file, looking for a specified key, and returns its value as uint64, +// using [ParseUint] for conversion. If the value is not found, 0 is returned. func GetValueByKey(path, file, key string) (uint64, error) { content, err := cgroups.ReadFile(path, file) if err != nil { return 0, err } + key += " " lines := strings.Split(content, "\n") for _, line := range lines { - arr := strings.Split(line, " ") - if len(arr) == 2 && arr[0] == key { - val, err := ParseUint(arr[1], 10, 64) + v, ok := strings.CutPrefix(line, key) + if ok { + val, err := ParseUint(v, 10, 64) if err != nil { err = &ParseError{Path: path, File: file, Err: err} } From ef983f5180ff5026646c8eae69b67f3f1d8beabc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 13:37:52 -0800 Subject: [PATCH 12/16] libct/cg/fscommon: ParseKeyValue: stricter check It makes sense to report an error if a key or a value is empty, as we don't expect anything like this. Reported-by: Sebastiaan van Stijn Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index 76d06e498..9279c064a 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -60,8 +60,8 @@ func ParseUint(s string, base, bitSize int) (uint64, error) { // "io_service_bytes 1234" will be returned as "io_service_bytes", 1234. func ParseKeyValue(t string) (string, uint64, error) { key, val, ok := strings.Cut(t, " ") - if !ok { - return "", 0, fmt.Errorf("line %q is not in key value format", t) + if !ok || key == "" || val == "" { + return "", 0, fmt.Errorf(`line %q is not in "key value" format`, t) } value, err := ParseUint(val, 10, 64) From ecf74300c0a3384dfbdf699f50e88a6aff53a934 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 14:05:56 -0800 Subject: [PATCH 13/16] libct/cg/fscommon: GetCgroupParam*: unify 1. GetCgroupParamUint: drop strings.TrimSpace since it was already done by GetCgroupParamString. 2. GetCgroupParamInt: use GetCgroupParamString, drop strings.TrimSpace. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index 9279c064a..e11e038f2 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -104,7 +104,6 @@ func GetCgroupParamUint(path, file string) (uint64, error) { if err != nil { return 0, err } - contents = strings.TrimSpace(contents) if contents == "max" { return math.MaxUint64, nil } @@ -119,11 +118,10 @@ func GetCgroupParamUint(path, file string) (uint64, error) { // GetCgroupParamInt reads a single int64 value from specified cgroup file. // If the value read is "max", the math.MaxInt64 is returned. func GetCgroupParamInt(path, file string) (int64, error) { - contents, err := cgroups.ReadFile(path, file) + contents, err := GetCgroupParamString(path, file) if err != nil { return 0, err } - contents = strings.TrimSpace(contents) if contents == "max" { return math.MaxInt64, nil } From 259b71c042fead3b7c2e9f7569c06a4616989242 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 20:16:41 -0800 Subject: [PATCH 14/16] libct/utils: stripRoot: rm useless HasPrefix Using strings.HasPrefix with strings.TrimPrefix results in doing the same thing (checking if prefix exists) twice. In this case, using strings.TrimPrefix right away is sufficient. Signed-off-by: Kir Kolyshkin --- libcontainer/utils/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index db420ea68..f3cae5030 100644 --- a/libcontainer/utils/utils.go +++ b/libcontainer/utils/utils.go @@ -77,7 +77,7 @@ func stripRoot(root, path string) string { path = "/" case root == "/": // do nothing - case strings.HasPrefix(path, root+"/"): + default: path = strings.TrimPrefix(path, root+"/") } return CleanPath("/" + path) From 055041e874c95946b50f70002a218a5dcc6d18c7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 Jan 2025 19:48:47 -0800 Subject: [PATCH 15/16] libct: use strings.CutPrefix where possible Using strings.CutPrefix (available since Go 1.20) instead of strings.HasPrefix and/or strings.TrimPrefix makes the code a tad more straightforward. No functional change. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/devices/systemd.go | 3 +-- libcontainer/cgroups/file.go | 4 ++-- libcontainer/cgroups/fs2/freezer.go | 4 +--- libcontainer/cgroups/systemd/user.go | 3 +-- libcontainer/cgroups/utils.go | 4 ++-- libcontainer/configs/validate/rootless.go | 6 +++--- libcontainer/specconv/spec_linux.go | 4 ++-- libcontainer/utils/utils.go | 4 ++-- 8 files changed, 14 insertions(+), 18 deletions(-) diff --git a/libcontainer/cgroups/devices/systemd.go b/libcontainer/cgroups/devices/systemd.go index 9627efd52..47ba7182b 100644 --- a/libcontainer/cgroups/devices/systemd.go +++ b/libcontainer/cgroups/devices/systemd.go @@ -224,8 +224,7 @@ func findDeviceGroup(ruleType devices.Type, ruleMajor int64) (string, error) { continue } - group := strings.TrimPrefix(line, ruleMajorStr) - if len(group) < len(line) { // got it + if group, ok := strings.CutPrefix(line, ruleMajorStr); ok { return prefix + group, nil } } diff --git a/libcontainer/cgroups/file.go b/libcontainer/cgroups/file.go index 78c5bcf0d..f37375f4d 100644 --- a/libcontainer/cgroups/file.go +++ b/libcontainer/cgroups/file.go @@ -151,8 +151,8 @@ func openFile(dir, file string, flags int) (*os.File, error) { if prepareOpenat2() != nil { return openFallback(path, flags, mode) } - relPath := strings.TrimPrefix(path, cgroupfsPrefix) - if len(relPath) == len(path) { // non-standard path, old system? + relPath, ok := strings.CutPrefix(path, cgroupfsPrefix) + if !ok { // Non-standard path, old system? return openFallback(path, flags, mode) } diff --git a/libcontainer/cgroups/fs2/freezer.go b/libcontainer/cgroups/fs2/freezer.go index 7396ac028..1f831500f 100644 --- a/libcontainer/cgroups/fs2/freezer.go +++ b/libcontainer/cgroups/fs2/freezer.go @@ -104,9 +104,7 @@ func waitFrozen(dirPath string) (cgroups.FreezerState, error) { if i == maxIter { return cgroups.Undefined, fmt.Errorf("timeout of %s reached waiting for the cgroup to freeze", waitTime*maxIter) } - line := scanner.Text() - val := strings.TrimPrefix(line, "frozen ") - if val != line { // got prefix + if val, ok := strings.CutPrefix(scanner.Text(), "frozen "); ok { if val[0] == '1' { return cgroups.Frozen, nil } diff --git a/libcontainer/cgroups/systemd/user.go b/libcontainer/cgroups/systemd/user.go index 1e18403ba..4a4348e70 100644 --- a/libcontainer/cgroups/systemd/user.go +++ b/libcontainer/cgroups/systemd/user.go @@ -61,8 +61,7 @@ func DetectUID() (int, error) { scanner := bufio.NewScanner(bytes.NewReader(b)) for scanner.Scan() { s := strings.TrimSpace(scanner.Text()) - if strings.HasPrefix(s, "OwnerUID=") { - uidStr := strings.TrimPrefix(s, "OwnerUID=") + if uidStr, ok := strings.CutPrefix(s, "OwnerUID="); ok { i, err := strconv.Atoi(uidStr) if err != nil { return -1, fmt.Errorf("could not detect the OwnerUID: %w", err) diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index d404647c8..9ef24b1ac 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -332,8 +332,8 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) { for _, file := range fileNames { // example: hugepages-1048576kB - val := strings.TrimPrefix(file, "hugepages-") - if len(val) == len(file) { + val, ok := strings.CutPrefix(file, "hugepages-") + if !ok { // Unexpected file name: no prefix found, ignore it. continue } diff --git a/libcontainer/configs/validate/rootless.go b/libcontainer/configs/validate/rootless.go index 4507d4495..7a30f5c58 100644 --- a/libcontainer/configs/validate/rootless.go +++ b/libcontainer/configs/validate/rootless.go @@ -56,7 +56,7 @@ func rootlessEUIDMount(config *configs.Config) error { // Check that the options list doesn't contain any uid= or gid= entries // that don't resolve to root. for _, opt := range strings.Split(mount.Data, ",") { - if str := strings.TrimPrefix(opt, "uid="); len(str) < len(opt) { + if str, ok := strings.CutPrefix(opt, "uid="); ok { uid, err := strconv.Atoi(str) if err != nil { // Ignore unknown mount options. @@ -65,9 +65,9 @@ func rootlessEUIDMount(config *configs.Config) error { if _, err := config.HostUID(uid); err != nil { return fmt.Errorf("cannot specify uid=%d mount option for rootless container: %w", uid, err) } + continue } - - if str := strings.TrimPrefix(opt, "gid="); len(str) < len(opt) { + if str, ok := strings.CutPrefix(opt, "gid="); ok { gid, err := strconv.Atoi(str) if err != nil { // Ignore unknown mount options. diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index c8446a56a..f4f5f0995 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -685,8 +685,8 @@ func initSystemdProps(spec *specs.Spec) ([]systemdDbus.Property, error) { var sp []systemdDbus.Property for k, v := range spec.Annotations { - name := strings.TrimPrefix(k, keyPrefix) - if len(name) == len(k) { // prefix not there + name, ok := strings.CutPrefix(k, keyPrefix) + if !ok { // prefix not there continue } if err := checkPropertyName(name); err != nil { diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index f3cae5030..d0243db87 100644 --- a/libcontainer/utils/utils.go +++ b/libcontainer/utils/utils.go @@ -88,8 +88,8 @@ func stripRoot(root, path string) string { func SearchLabels(labels []string, key string) (string, bool) { key += "=" for _, s := range labels { - if strings.HasPrefix(s, key) { - return s[len(key):], true + if val, ok := strings.CutPrefix(s, key); ok { + return val, true } } return "", false From 746a5c23c949172d00cbcf1882e0cfac8c5adc1a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 Jan 2025 13:56:09 -0800 Subject: [PATCH 16/16] libcontainer/configs/validate: improve rootlessEUIDMount 1. Avoid splitting mount data into []string if it does not contain options we're interested in. This should result in slightly less garbage to collect. 2. Use if / else if instead of continue, to make it clearer that we're processing one option at a time. 3. Print the whole option as a sting in an error message; practically this should not have any effect, it's just simpler. 4. Improve some comments. Signed-off-by: Kir Kolyshkin --- libcontainer/configs/validate/rootless.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/libcontainer/configs/validate/rootless.go b/libcontainer/configs/validate/rootless.go index 7a30f5c58..88cf1cd60 100644 --- a/libcontainer/configs/validate/rootless.go +++ b/libcontainer/configs/validate/rootless.go @@ -52,9 +52,14 @@ func rootlessEUIDMount(config *configs.Config) error { // convinced that's a good idea. The kernel is the best arbiter of // access control. + // Check that the options list doesn't contain any uid= or gid= entries + // that don't resolve to root. for _, mount := range config.Mounts { - // Check that the options list doesn't contain any uid= or gid= entries - // that don't resolve to root. + // Look for a common substring; skip further processing + // if there can't be any uid= or gid= options. + if !strings.Contains(mount.Data, "id=") { + continue + } for _, opt := range strings.Split(mount.Data, ",") { if str, ok := strings.CutPrefix(opt, "uid="); ok { uid, err := strconv.Atoi(str) @@ -63,18 +68,16 @@ func rootlessEUIDMount(config *configs.Config) error { continue } if _, err := config.HostUID(uid); err != nil { - return fmt.Errorf("cannot specify uid=%d mount option for rootless container: %w", uid, err) + return fmt.Errorf("cannot specify %s mount option for rootless container: %w", opt, err) } - continue - } - if str, ok := strings.CutPrefix(opt, "gid="); ok { + } else if str, ok := strings.CutPrefix(opt, "gid="); ok { gid, err := strconv.Atoi(str) if err != nil { // Ignore unknown mount options. continue } if _, err := config.HostGID(gid); err != nil { - return fmt.Errorf("cannot specify gid=%d mount option for rootless container: %w", gid, err) + return fmt.Errorf("cannot specify %s mount option for rootless container: %w", opt, err) } } }