From 825335b2b7da9622877a54a029f71e231fd3ec48 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 8 Jun 2021 19:21:54 -0700 Subject: [PATCH] libct/cg/fs2: fix/unify parsing errors This builds on top of recently introduced fscommon.ParseError. Errors returned from parsers (mostly ones used by GetStats()) are all different, and many are incomplete. For example, in many cases errors from strconv.ParseUint are returned as is, meaning there is no context telling which file we were reading. Similarly, errors from fscommon.ParseKeyValue should be wrapped to add more context. Same is true for scanner.Err(). OTOH, errors from fscommon.GetCgroup* do have enough context and there is no need to wrap them. Fix all the above. While at it, add missing scanner.Err() checks. [v2: use parseError, not ParseError] Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs2/cpu.go | 8 ++++++-- libcontainer/cgroups/fs2/fs2.go | 2 ++ libcontainer/cgroups/fs2/hugetlb.go | 2 +- libcontainer/cgroups/fs2/io.go | 11 ++++++----- libcontainer/cgroups/fs2/memory.go | 22 +++++++++++++--------- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/libcontainer/cgroups/fs2/cpu.go b/libcontainer/cgroups/fs2/cpu.go index 25c47c961..83578a340 100644 --- a/libcontainer/cgroups/fs2/cpu.go +++ b/libcontainer/cgroups/fs2/cpu.go @@ -49,7 +49,8 @@ func setCpu(dirPath string, r *configs.Resources) error { } func statCpu(dirPath string, stats *cgroups.Stats) error { - f, err := cgroups.OpenFile(dirPath, "cpu.stat", os.O_RDONLY) + const file = "cpu.stat" + f, err := cgroups.OpenFile(dirPath, file, os.O_RDONLY) if err != nil { return err } @@ -59,7 +60,7 @@ func statCpu(dirPath string, stats *cgroups.Stats) error { for sc.Scan() { t, v, err := fscommon.ParseKeyValue(sc.Text()) if err != nil { - return err + return &parseError{Path: dirPath, File: file, Err: err} } switch t { case "usage_usec": @@ -81,5 +82,8 @@ func statCpu(dirPath string, stats *cgroups.Stats) error { stats.CpuStats.ThrottlingData.ThrottledTime = v * 1000 } } + if err := sc.Err(); err != nil { + return &parseError{Path: dirPath, File: file, Err: err} + } return nil } diff --git a/libcontainer/cgroups/fs2/fs2.go b/libcontainer/cgroups/fs2/fs2.go index afba0ab1c..605b8683b 100644 --- a/libcontainer/cgroups/fs2/fs2.go +++ b/libcontainer/cgroups/fs2/fs2.go @@ -13,6 +13,8 @@ import ( "github.com/pkg/errors" ) +type parseError = fscommon.ParseError + type manager struct { config *configs.Cgroup // dirPath is like "/sys/fs/cgroup/user.slice/user-1001.slice/session-1.scope" diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go index 3f513975b..bf5ed5412 100644 --- a/libcontainer/cgroups/fs2/hugetlb.go +++ b/libcontainer/cgroups/fs2/hugetlb.go @@ -46,7 +46,7 @@ func statHugeTlb(dirPath string, stats *cgroups.Stats) error { fileName := "hugetlb." + pagesize + ".events" value, err = fscommon.GetValueByKey(dirPath, fileName, "max") if err != nil { - return errors.Wrap(err, "failed to read stats") + return err } hugetlbStats.Failcnt = value diff --git a/libcontainer/cgroups/fs2/io.go b/libcontainer/cgroups/fs2/io.go index fd3f0993e..2771e1ef3 100644 --- a/libcontainer/cgroups/fs2/io.go +++ b/libcontainer/cgroups/fs2/io.go @@ -117,13 +117,14 @@ func readCgroup2MapFile(dirPath string, name string) (map[string][]string, error ret[parts[0]] = parts[1:] } if err := scanner.Err(); err != nil { - return nil, err + return nil, &parseError{Path: dirPath, File: name, Err: err} } return ret, nil } func statIo(dirPath string, stats *cgroups.Stats) error { - values, err := readCgroup2MapFile(dirPath, "io.stat") + const file = "io.stat" + values, err := readCgroup2MapFile(dirPath, file) if err != nil { return err } @@ -136,11 +137,11 @@ func statIo(dirPath string, stats *cgroups.Stats) error { } major, err := strconv.ParseUint(d[0], 10, 64) if err != nil { - return err + return &parseError{Path: dirPath, File: file, Err: err} } minor, err := strconv.ParseUint(d[1], 10, 64) if err != nil { - return err + return &parseError{Path: dirPath, File: file, Err: err} } for _, item := range v { @@ -177,7 +178,7 @@ func statIo(dirPath string, stats *cgroups.Stats) error { value, err := strconv.ParseUint(d[1], 10, 64) if err != nil { - return err + return &parseError{Path: dirPath, File: file, Err: err} } entry := cgroups.BlkioStatEntry{ diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go index 53e8f1e93..a8c31a936 100644 --- a/libcontainer/cgroups/fs2/memory.go +++ b/libcontainer/cgroups/fs2/memory.go @@ -75,8 +75,8 @@ func setMemory(dirPath string, r *configs.Resources) error { } func statMemory(dirPath string, stats *cgroups.Stats) error { - // Set stats from memory.stat. - statsFile, err := cgroups.OpenFile(dirPath, "memory.stat", os.O_RDONLY) + const file = "memory.stat" + statsFile, err := cgroups.OpenFile(dirPath, file, os.O_RDONLY) if err != nil { return err } @@ -86,10 +86,13 @@ func statMemory(dirPath string, stats *cgroups.Stats) error { for sc.Scan() { t, v, err := fscommon.ParseKeyValue(sc.Text()) if err != nil { - return errors.Wrapf(err, "failed to parse memory.stat (%q)", sc.Text()) + return &parseError{Path: dirPath, File: file, Err: err} } stats.MemoryStats.Stats[t] = v } + if err := sc.Err(); err != nil { + return &parseError{Path: dirPath, File: file, Err: err} + } stats.MemoryStats.Cache = stats.MemoryStats.Stats["file"] // Unlike cgroup v1 which has memory.use_hierarchy binary knob, // cgroup v2 is always hierarchical. @@ -139,13 +142,13 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) { // swapaccount=0 kernel boot parameter is given. return cgroups.MemoryData{}, nil } - return cgroups.MemoryData{}, errors.Wrapf(err, "failed to parse %s", usage) + return cgroups.MemoryData{}, err } memoryData.Usage = value value, err = fscommon.GetCgroupParamUint(path, limit) if err != nil { - return cgroups.MemoryData{}, errors.Wrapf(err, "failed to parse %s", limit) + return cgroups.MemoryData{}, err } memoryData.Limit = value @@ -153,7 +156,8 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) { } func statsFromMeminfo(stats *cgroups.Stats) error { - f, err := os.Open("/proc/meminfo") + const file = "/proc/meminfo" + f, err := os.Open(file) if err != nil { return err } @@ -190,7 +194,7 @@ func statsFromMeminfo(stats *cgroups.Stats) error { vStr := strings.TrimSpace(strings.TrimSuffix(parts[1], " kB")) *p, err = strconv.ParseUint(vStr, 10, 64) if err != nil { - return errors.Wrap(err, "parsing /proc/meminfo "+k) + return &parseError{File: file, Err: errors.New("bad value for " + k)} } found++ @@ -199,8 +203,8 @@ func statsFromMeminfo(stats *cgroups.Stats) error { break } } - if sc.Err() != nil { - return sc.Err() + if err := sc.Err(); err != nil { + return &parseError{Path: "", File: file, Err: err} } stats.MemoryStats.SwapUsage.Usage = (swap_total - swap_free) * 1024