From d167be2925b891706c84820f62ac216be7188d73 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Sep 2020 18:05:22 -0700 Subject: [PATCH 1/5] libct/cgroups/fs2/statHugeTlb: error message nits 1. Don't wrap the error from fscommon.GetCgroupParamUint as it already contains the file name. 2. Don't put file name when wrapping the error from ioutil.ReadFile since it already has it. 3. Don't reconstruct file name, use existing one since it's available. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/hugetlb.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go index 4a399aaec..b7adf8710 100644 --- a/libcontainer/cgroups/fs2/hugetlb.go +++ b/libcontainer/cgroups/fs2/hugetlb.go @@ -43,7 +43,7 @@ func statHugeTlb(dirPath string, stats *cgroups.Stats) error { usage := strings.Join([]string{"hugetlb", pagesize, "current"}, ".") value, err := fscommon.GetCgroupParamUint(dirPath, usage) if err != nil { - return errors.Wrapf(err, "failed to parse hugetlb.%s.current file", pagesize) + return err } hugetlbStats.Usage = value @@ -51,11 +51,11 @@ func statHugeTlb(dirPath string, stats *cgroups.Stats) error { filePath := filepath.Join(dirPath, fileName) contents, err := ioutil.ReadFile(filePath) if err != nil { - return errors.Wrapf(err, "failed to parse hugetlb.%s.events file", pagesize) + return errors.Wrap(err, "failed to read stats") } _, value, err = fscommon.GetCgroupParamKeyValue(string(contents)) if err != nil { - return errors.Wrapf(err, "failed to parse hugetlb.%s.events file", pagesize) + return errors.Wrap(err, "failed to parse "+fileName) } hugetlbStats.Failcnt = value From aac4d1f5c3ba0aa52f01378343e75c339b527919 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Sep 2020 19:42:56 -0700 Subject: [PATCH 2/5] libct/cg/fscommon/GetCgroupParamKeyValue: nits 2. Fix wrapping the error to not have the value as it's already part of the error returned from ParseUint. 3. Fix/improve doc. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index 46c3c7799..236c75a07 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -36,15 +36,16 @@ func ParseUint(s string, base, bitSize int) (uint64, error) { return value, nil } -// Parses a cgroup param and returns as name, value -// i.e. "io_service_bytes 1234" will return as io_service_bytes, 1234 +// GetCgroupParamKeyValue parses a space-separated "name value" kind of cgroup +// parameter and returns its components. For example, "io_service_bytes 1234" +// will return as "io_service_bytes", 1234. func GetCgroupParamKeyValue(t string) (string, uint64, error) { parts := strings.Fields(t) switch len(parts) { case 2: value, err := ParseUint(parts[1], 10, 64) if err != nil { - return "", 0, fmt.Errorf("unable to convert param value (%q) to uint64: %v", parts[1], err) + return "", 0, fmt.Errorf("unable to convert to uint64: %v", err) } return parts[0], value, nil From e76ac1c05493fcd2c3a89996675a7c0b792de52c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Sep 2020 19:50:36 -0700 Subject: [PATCH 3/5] libct/cg/fscommon.GetCgroupParamString: use ReadFile 1. Use own ReadFile wrapper instead of ioutils.ReadFile. This makes it use the security measures of ReadFile. 2. Improve doc. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index 236c75a07..99c31bb4a 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -73,9 +73,9 @@ func GetCgroupParamUint(cgroupPath, cgroupFile string) (uint64, error) { return res, nil } -// Gets a string value from the specified cgroup file -func GetCgroupParamString(cgroupPath, cgroupFile string) (string, error) { - contents, err := ioutil.ReadFile(filepath.Join(cgroupPath, cgroupFile)) +// GetCgroupParamString reads a string from the specified cgroup file. +func GetCgroupParamString(path, file string) (string, error) { + contents, err := ReadFile(path, file) if err != nil { return "", err } From 31f0f5b7e03a20c75760d235694ffbfd5f924514 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 22 Sep 2020 19:53:06 -0700 Subject: [PATCH 4/5] libct/cg/fscommon.GetCgroupParamUint: improve 1. Use GetCgroupParamString as the initial part of both functions are the same and we can reuse it. This also gives us whatever security measures GetCgroupParamString has (see previous commit). 2. Fix the error wrapping to not add the value, as it is already a part of the error returned by ParseUint. 3. Improve docstring. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/utils.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index 99c31bb4a..aa281aca8 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -5,9 +5,7 @@ package fscommon import ( "errors" "fmt" - "io/ioutil" "math" - "path/filepath" "strconv" "strings" ) @@ -54,21 +52,21 @@ func GetCgroupParamKeyValue(t string) (string, uint64, error) { } } -// Gets a single uint64 value from the specified cgroup file. -func GetCgroupParamUint(cgroupPath, cgroupFile string) (uint64, error) { - fileName := filepath.Join(cgroupPath, cgroupFile) - contents, err := ioutil.ReadFile(fileName) +// GetCgroupParamUint reads a single uint64 value from the specified cgroup file. +// If the value read is "max", the math.MaxUint64 is returned. +func GetCgroupParamUint(path, file string) (uint64, error) { + contents, err := GetCgroupParamString(path, file) if err != nil { return 0, err } - trimmed := strings.TrimSpace(string(contents)) - if trimmed == "max" { + contents = strings.TrimSpace(contents) + if contents == "max" { return math.MaxUint64, nil } - res, err := ParseUint(trimmed, 10, 64) + res, err := ParseUint(contents, 10, 64) if err != nil { - return res, fmt.Errorf("unable to parse %q as a uint from Cgroup file %q", string(contents), fileName) + return res, fmt.Errorf("unable to parse file %q", path+"/"+file) } return res, nil } From 6c83d23ffc553200e382f403130ba6203441ef17 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Sep 2020 15:51:49 -0700 Subject: [PATCH 5/5] libcontainer/cgroups/fscommon: improve doc Document ReadFile and WriteFile. Fix doc for ParseUint to be in canonical form. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fscommon/fscommon.go | 4 ++++ libcontainer/cgroups/fscommon/utils.go | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/libcontainer/cgroups/fscommon/fscommon.go b/libcontainer/cgroups/fscommon/fscommon.go index 446b9a1f0..8f7298c88 100644 --- a/libcontainer/cgroups/fscommon/fscommon.go +++ b/libcontainer/cgroups/fscommon/fscommon.go @@ -12,6 +12,8 @@ import ( "golang.org/x/sys/unix" ) +// WriteFile writes data to a cgroup file in dir. +// It is supposed to be used for cgroup files only. func WriteFile(dir, file, data string) error { if dir == "" { return errors.Errorf("no directory specified for %s", file) @@ -26,6 +28,8 @@ func WriteFile(dir, file, data string) error { return nil } +// ReadFile reads data from a cgroup file in dir. +// It is supposed to be used for cgroup files only. func ReadFile(dir, file string) (string, error) { if dir == "" { return "", errors.Errorf("no directory specified for %s", file) diff --git a/libcontainer/cgroups/fscommon/utils.go b/libcontainer/cgroups/fscommon/utils.go index aa281aca8..614891a7c 100644 --- a/libcontainer/cgroups/fscommon/utils.go +++ b/libcontainer/cgroups/fscommon/utils.go @@ -14,8 +14,9 @@ var ( ErrNotValidFormat = errors.New("line is not a valid key value format") ) -// Saturates negative values at zero and returns a uint64. -// Due to kernel bugs, some of the memory cgroup stats can be negative. +// ParseUint converts a string to an uint64 integer. +// Negative values are returned at zero as, due to kernel bugs, +// some of the memory cgroup stats can be negative. func ParseUint(s string, base, bitSize int) (uint64, error) { value, err := strconv.ParseUint(s, base, bitSize) if err != nil {