-
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
Use strings.Cut where possible #4470
base: main
Are you sure you want to change the base?
Changes from all commits
0e2d611
d9e1005
a8da6c6
a4ed2d8
32fa40a
486c451
b33fe69
4c86035
cddca40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 <controller>: prefix)", c) | ||
} | ||
paths[""] = c | ||
Comment on lines
+133
to
137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was trying to somewhat grasp what this case is for;
So; # valid: single arg
--cgroup a,b:/hello-world
# valid: single arg without controller
--cgroup /hello-world
# invalid: multiple args, one without controller
--cgroup a,b:/hello-world --cgroup /hello-world
--cgroup /hello-world --cgroup a,b:/hello-world So, if no controller is passed, it's for everything which is only allowed if no other (per-controller) paths are specified, correct? (so not allowed to be combined). ☝️ Perhaps that's something we should capture in a comment on the function Also;
|
||
} else { | ||
// There may be a few comma-separated controllers. | ||
for _, ctrl := range strings.Split(cs[0], ",") { | ||
paths[ctrl] = cs[1] | ||
} | ||
} | ||
} | ||
return paths, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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 { | ||
|
@@ -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) { | ||
|
@@ -140,22 +132,24 @@ 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 | ||
Comment on lines
+136
to
138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was considering if we had to check for more than 3 fields to happen here, but I guess that would error already once we try to OTOTH, I wonder if there could ever be more columns added here? Old code would do a Not sure if that behavior was correct, or if it should've processed the first 3 columns (and only ignored the 4th+ columns)? Is there any chance there would ever be more columns added? |
||
} | ||
|
||
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) | ||
|
||
Comment on lines
+151
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accidental empty line added here |
||
} | ||
if err := scanner.Err(); err != nil { | ||
return nil, nil, &parseError{Path: path, File: file, Err: err} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
Comment on lines
-90
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we missing an error condition for lines that are invalid? Do we need to handle, say |
||
return path, nil | ||
} | ||
} | ||
if err := s.Err(); err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
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 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of these could be improved as well; in this case we know it's missing a We could check here if it's empty and return that no value is specified; if !ok || v == "" {
return errors.New("Unable to parse RDMA entry: no value specified")
} |
||
} | ||
|
||
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 | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,24 +56,24 @@ func ParseUint(s string, base, bitSize int) (uint64, error) { | |
|
||
// ParseKeyValue parses a space-separated "name 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily for this PR, but we could check for empty values here as well to return a nicer error than "strconv.ParseUint failed to parse ..."; 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 | ||
// and returns a value of the specified key. [ParseUint] is used for value | ||
// conversion. | ||
func GetValueByKey(path, file, key string) (uint64, error) { | ||
content, err := cgroups.ReadFile(path, file) | ||
|
@@ -83,9 +83,9 @@ func GetValueByKey(path, file, key string) (uint64, error) { | |
|
||
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) | ||
k, v, ok := strings.Cut(line, " ") | ||
if ok && k == key { | ||
Comment on lines
-86
to
+87
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if relevant (and probably new code is more correct?); previously, we would ignore cases with more than 2 fields. I.e., That said; see my other comment; this would potentially be an issue if we try to parse something that may currently have 2 cols, and in future 3 cols; not sure if that should error or silently ignore |
||
val, err := ParseUint(v, 10, 64) | ||
if err != nil { | ||
err = &ParseError{Path: path, File: file, Err: err} | ||
} | ||
|
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 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".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.
So, the old code would error out when
c == "a:b:c:d:e"
.The new code would not, assigning
b:c:d:e
topath
.To me, the new code is (ever so slightly) more correct, since
:
is a valid symbol which should be allowed inpath
.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.
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