Skip to content

Use strings.Cut and strings.CutPrefix where possible #4470

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

Merged
merged 16 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,27 +117,36 @@ 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
}
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 {
Copy link
Member

@lifubang lifubang Dec 20, 2024

Choose a reason for hiding this comment

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

I havn't checked that whether this will lead to some regressions or not, for example, if c == "a:b:c:d:e":
When using SplitN, cs[1] will be "b";
When using Cut, path will be "b:c:d:e".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the old code would error out when c == "a:b:c:d:e".

The new code would not, assigning b:c:d:e to path.

To me, the new code is (ever so slightly) more correct, since : is a valid symbol which should be allowed in path.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, was looking at that as well; agreed, looks like new code is more correct here.

I still need to have a quick look at the splitting on commas, and the if len(args) != 1 { check below. Probably will check out this branch locally

// 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 <controller> 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 (cgroup v2, a single path).
if len(args) != 1 {
return nil, fmt.Errorf("invalid --cgroup argument: %s (missing <controller>: 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
Expand Down
3 changes: 1 addition & 2 deletions libcontainer/cgroups/devices/systemd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/cgroups/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
29 changes: 11 additions & 18 deletions libcontainer/cgroups/fs/cpuacct.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -80,7 +73,7 @@ func getCpuUsageBreakdown(path string) (uint64, uint64, error) {
const (
userField = "user"
systemField = "system"
file = cgroupCpuacctStat
file = "cpuacct.stat"
)

// Expected format:
Expand All @@ -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) {
Expand All @@ -112,7 +105,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 {
Expand All @@ -126,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) {
Expand All @@ -140,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}
Expand Down
8 changes: 4 additions & 4 deletions libcontainer/cgroups/fs/cpuacct_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
19 changes: 8 additions & 11 deletions libcontainer/cgroups/fs/cpuset.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
13 changes: 3 additions & 10 deletions libcontainer/cgroups/fs2/defaultpath.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 1 addition & 3 deletions libcontainer/cgroups/fs2/freezer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
5 changes: 2 additions & 3 deletions libcontainer/cgroups/fs2/fs2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
14 changes: 7 additions & 7 deletions libcontainer/cgroups/fs2/psi.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,29 +58,29 @@ 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":
pv = &data.Avg60
case "avg300":
pv = &data.Avg300
case "total":
v, err := strconv.ParseUint(kv[1], 10, 64)
v, err := strconv.ParseUint(val, 10, 64)
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR, but I started to look in some other codebases to dismantle these errors. The default error returned is something like;

invalid total PSI value: strconv.ParseUint: parsing "some invalid value": invalid syntax

Which tends to be a bit "implementation detail", and strconv.ParseUint is not very relevant to the user. So in some cases, dismantling the error can be useful; https://cs.opensource.google/go/go/+/refs/tags/go1.23.4:src/strconv/atoi.go;l=20-49

https://go.dev/play/p/E9Dx1YBzPiN

var numErr *strconv.NumError
if errors.As(err, &numErr) {
	fmt.Printf("invalid  %s PS value (%s): %w", key, numErr.Num, numErr.Err)
}

Which would be something like;

invalid total PSI value (some invalid value): invalid syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strconv.ParseUint is not very relevant to the user.

Indeed it isn't, but at least it says which function returned the error, and any developer (and some advanced users) will deduce that an unsigned integer was expected.

Generally speaking, to make this error more user-friendly, the error text should say something like "expected unsigned integer value, got %q: %w", and the best place to fix this is the actual strconv package.

In this case, though, we're parsing kernel-generated data (and we actually check that it comes from the kernel, by checking the filesystem type in cgroups.OpenFile), and so the chance of getting a non-number here (and thus receive an error) is close to 0 here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it really was just me rambling a bit, having dealt too many times with users "I have the same error" (because the strconv.ParseUint .... didn't resonate with them, and they thought it was a bug, not recognising "invalid input here".

Probably something for a rainy afternoon to look if I can come with some nice approach for these. 😄

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
}
Expand Down
11 changes: 5 additions & 6 deletions libcontainer/cgroups/fscommon/rdma.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down
31 changes: 15 additions & 16 deletions libcontainer/cgroups/fscommon/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,38 +54,39 @@ 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 {
return "", 0, fmt.Errorf("line %q is not in key value format", t)
key, val, ok := strings.Cut(t, " ")
if !ok || key == "" || val == "" {
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,
// 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}
}
Expand All @@ -103,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
}
Expand All @@ -118,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
}
Expand Down
Loading