From f13a93257078dccd0c9471d167775fcbcc17ed62 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Wed, 6 Oct 2021 13:50:12 -0700
Subject: [PATCH] libct/cg: HugePageSizes: simplify code and test

1. Instead of distinguishing between errors and warnings, let's treat all
   errors as warnings, thus simplifying the code. This changes the
   function behaviour for input like hugepages-BadNumberKb --
   previously, the error from Atoi("BadNumber") was considered fatal,
   now it's just another warnings.

2. Move the warning logging to HugePageSizes, thus simplifying the test
   case, which no longer needs to read what logrus writes. Note that we
   do not want to log all the warnings (as chances are very low we'll
   get any, and if we do this means the code need to be updated), only
   the first one.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/cgroups/utils.go      | 24 ++++++++++++-----
 libcontainer/cgroups/utils_test.go | 41 +++++++++++++-----------------
 2 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go
index 1577c41ef..13ebf52ab 100644
--- a/libcontainer/cgroups/utils.go
+++ b/libcontainer/cgroups/utils.go
@@ -319,7 +319,10 @@ func HugePageSizes() []string {
 			return
 		}
 
-		hugePageSizes, _ = getHugePageSizeFromFilenames(files)
+		hugePageSizes, err = getHugePageSizeFromFilenames(files)
+		if err != nil {
+			logrus.Warn("HugePageSizes: ", err)
+		}
 	})
 
 	return hugePageSizes
@@ -327,24 +330,33 @@ func HugePageSizes() []string {
 
 func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
 	pageSizes := make([]string, 0, len(fileNames))
+	var warn error
 
 	for _, file := range fileNames {
 		// example: hugepages-1048576kB
 		val := strings.TrimPrefix(file, "hugepages-")
 		if len(val) == len(file) {
-			// unexpected file name: no prefix found
+			// Unexpected file name: no prefix found, ignore it.
 			continue
 		}
-		// The suffix is always "kB" (as of Linux 5.9)
+		// The suffix is always "kB" (as of Linux 5.13). If we find
+		// something else, produce an error but keep going.
 		eLen := len(val) - 2
 		val = strings.TrimSuffix(val, "kB")
 		if len(val) != eLen {
-			logrus.Warnf("HugePageSizes: %s: invalid filename suffix (expected \"kB\")", file)
+			// Highly unlikely.
+			if warn == nil {
+				warn = errors.New(file + `: invalid suffix (expected "kB")`)
+			}
 			continue
 		}
 		size, err := strconv.Atoi(val)
 		if err != nil {
-			return nil, err
+			// Highly unlikely.
+			if warn == nil {
+				warn = fmt.Errorf("%s: %w", file, err)
+			}
+			continue
 		}
 		// Model after https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574
 		// but in our case the size is in KB already.
@@ -358,7 +370,7 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
 		pageSizes = append(pageSizes, val)
 	}
 
-	return pageSizes, nil
+	return pageSizes, warn
 }
 
 // GetPids returns all pids, that were added to cgroup at path.
diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go
index 708015d44..259313ba5 100644
--- a/libcontainer/cgroups/utils_test.go
+++ b/libcontainer/cgroups/utils_test.go
@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/moby/sys/mountinfo"
-	"github.com/sirupsen/logrus"
 )
 
 const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
@@ -467,7 +466,6 @@ func TestGetHugePageSizeImpl(t *testing.T) {
 		input  []string
 		output []string
 		isErr  bool
-		isWarn bool
 	}{
 		{
 			input:  []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"},
@@ -487,25 +485,29 @@ func TestGetHugePageSizeImpl(t *testing.T) {
 		{ // invalid prefix (silently skipped)
 			input: []string{"whatever-1024kB"},
 		},
-		{ // invalid suffix (skipped with a warning)
-			input:  []string{"hugepages-1024gB"},
-			isWarn: true,
+		{ // invalid suffix
+			input: []string{"hugepages-1024gB"},
+			isErr: true,
 		},
-		{ // no suffix (skipped with a warning)
-			input:  []string{"hugepages-1024"},
-			isWarn: true,
+		{ // no suffix
+			input: []string{"hugepages-1024"},
+			isErr: true,
+		},
+		{ // mixed valid and invalid entries
+			input:  []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"},
+			output: []string{"4GB", "2MB", "64KB"},
+			isErr:  true,
+		},
+		{ // mixed valid and invalid entries
+			input:  []string{"hugepages-2048kB", "hugepages-kB", "hugepages-64kB"},
+			output: []string{"2MB", "64KB"},
+			isErr:  true,
 		},
 	}
 
-	// Need to catch warnings.
-	savedOut := logrus.StandardLogger().Out
-	defer logrus.SetOutput(savedOut)
-	warns := new(bytes.Buffer)
-	logrus.SetOutput(warns)
-
 	for _, c := range testCases {
-		warns.Reset()
 		output, err := getHugePageSizeFromFilenames(c.input)
+		t.Log("input:", c.input, "; output:", output, "; err:", err)
 		if err != nil {
 			if !c.isErr {
 				t.Errorf("input %v, expected nil, got error: %v", c.input, err)
@@ -515,15 +517,6 @@ func TestGetHugePageSizeImpl(t *testing.T) {
 		}
 		if c.isErr {
 			t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output)
-			// no more checks
-			continue
-		}
-		// check for warnings
-		if c.isWarn && warns.Len() == 0 {
-			t.Errorf("input %v, expected a warning, got none", c.input)
-		}
-		if !c.isWarn && warns.Len() > 0 {
-			t.Errorf("input %v, unexpected warning: %s", c.input, warns.String())
 		}
 		// check output
 		if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) {