ronantakizawa/codereview-bench huggingface dataset:
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
	//
	// For example, on a two-socket machine, the schema line could be
	// "MB:0=5000;1=7000" which means 5000 MBps memory bandwidth limit on
	// socket 0 and 7000 MBps memory bandwidth limit on socket 1.
	if container.IntelRdt != nil {
		path := m.GetPath()
		l3CacheSchema := container.IntelRdt.L3CacheSchema
		memBwSchema := container.IntelRdt.MemBwSchema

		// TODO: verify that l3CacheSchema and/or memBwSchema match the
		// existing schemata if ClosID has been specified. This is a more
		// involved than reading the file and doing plain string comparison as
		// the value written in does not necessarily match what gets read out
		// (leading zeros, cache id ordering etc).

		parts := []string{}
		if l3CacheSchema != "" {
			parts = append(parts, l3CacheSchema)
		}
		if memBwSchema != "" {
			parts = append(parts, memBwSchema)
		}
		parts = append(parts, container.IntelRdt.Schemata...)

		// Write a single joint schema string to schemata file
		schemata := strings.Join(parts, "\n")
		if schemata != "" {
			if err := writeFile(path, "schemata", schemata); err != nil {
				return err
			}
		}
	}

	return nil
}

func newLastCmdError(err error) error {
	status, err1 := getLastCmdStatus()
	if err1 == nil {
		return fmt.Errorf("%w, last_cmd_status: %s", err, status)
	}
	return err
}


Review comment (performance):
nit: perhaps you can use `strings.Builder` instead? Something like this:

```go
var schemata strings.Builder
for _, s := range []string{l3CacheSchema, memBwSchema, container.IntelRdt.Schemata...} {
	if s != "" {
		schemata.WriteString(s)
		schemata.WriteString("\n")
	}
}
if schemata.Len() > 0 {
	if err := writeFile(path, "schemata", schemata); err != nil {
		return err
	}
}
```

Code after:
		// the value written in does not necessarily match what gets read out
		// (leading zeros, cache id ordering etc).
		var schemata strings.Builder
		for _, s := range append([]string{r.L3CacheSchema, r.MemBwSchema}, r.Schemata...) {
			if s != "" {
				schemata.WriteString(s)
				schemata.WriteString("\n")
			}
		}

		if schemata.Len() > 0 {
			path := filepath.Join(m.GetPath(), "schemata")
			if err := os.WriteFile(path, []byte(schemata.String()), 0o600); err != nil {
				return newLastCmdError(fmt.Errorf("intelrdt: unable to write %q: %w", schemata.String(), err))
			}
		}
	}

	return nil
}

func newLastCmdError(err error) error {
	status, err1 := getLastCmdStatus()
	if err1 == nil {
		return fmt.Errorf("%w, last_cmd_status: %s", err, status)
	}
	return err
}
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
		return 0, err
	}

	res, err := fscommon.ParseUint(string(bytes.TrimSpace(contents)), 10, 64)
	if err != nil {
		return res, fmt.Errorf("unable to parse %q as a uint from file %q", string(contents), fileName)
	}
	return res, nil
}

// Gets a string value from the specified file
func getIntelRdtParamString(path, file string) (string, error) {
	contents, err := os.ReadFile(filepath.Join(path, file))
	if err != nil {
		return "", err
	}

	return string(bytes.TrimSpace(contents)), nil
}

var writeFile = writeFileFunc(os.WriteFile)

// writeFileFunc is a helper to allow mocking os.WriteFile in tests.
func writeFileFunc(f func(string, []byte, os.FileMode) error) func(string, string, string) error {
	return func(dir, file, data string) error {
		if dir == "" {
			return fmt.Errorf("no such directory for %s", file)
		}
		if err := f(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil {
			return newLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err))
		}
		return nil
	}
}

// Get the read-only L3 cache information
func getL3CacheInfo() (*L3CacheInfo, error) {
	l3CacheInfo := &L3CacheInfo{}

	rootPath, err := Root()
	if err != nil {
		return l3CacheInfo, err
	}

	path := filepath.Join(rootPath, "info", "L3")
	cbmMask, err := getIntelRdtParamString(path, "cbm_mask")
	if err != nil {
		return l3CacheInfo, err
	}
	minCbmBits, err := getIntelRdtParamUint(path, "min_cbm_bits")
	if err != nil {

Review comment (bug):
@marquiz can you explain what was the bug and why we need to change it? It had a way to mock things before, why do we need a different way? I'm fine with changing it, but I'd like to know why.

I *guess* it can be simpler (or just some interface), but I'm not sure why we need this, so maybe it can't.

Code after:
		return 0, err
	}

	res, err := fscommon.ParseUint(string(bytes.TrimSpace(contents)), 10, 64)
	if err != nil {
		return res, fmt.Errorf("unable to parse %q as a uint from file %q", string(contents), fileName)
	}
	return res, nil
}

// Gets a string value from the specified file
func getIntelRdtParamString(path, file string) (string, error) {
	contents, err := os.ReadFile(filepath.Join(path, file))
	if err != nil {
		return "", err
	}

	return string(bytes.TrimSpace(contents)), nil
}

// Get the read-only L3 cache information
func getL3CacheInfo() (*L3CacheInfo, error) {
	l3CacheInfo := &L3CacheInfo{}

	rootPath, err := Root()
	if err != nil {
		return l3CacheInfo, err
	}

	path := filepath.Join(rootPath, "info", "L3")
	cbmMask, err := getIntelRdtParamString(path, "cbm_mask")
	if err != nil {
		return l3CacheInfo, err
	}
	minCbmBits, err := getIntelRdtParamUint(path, "min_cbm_bits")
	if err != nil {
		return l3CacheInfo, err
	}
	numClosids, err := getIntelRdtParamUint(path, "num_closids")
	if err != nil {
		return l3CacheInfo, err
	}

	l3CacheInfo.CbmMask = cbmMask
	l3CacheInfo.MinCbmBits = minCbmBits
	l3CacheInfo.NumClosids = numClosids

	return l3CacheInfo, nil
}

// Get the read-only memory bandwidth information
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
	// "actual memory bandwidth < user specified memory bandwidth".
	//
	// For example, on a two-socket machine, the schema line could be
	// "MB:0=5000;1=7000" which means 5000 MBps memory bandwidth limit on
	// socket 0 and 7000 MBps memory bandwidth limit on socket 1.
	if r := container.IntelRdt; r != nil {
		path := m.GetPath()

		// TODO: verify that l3CacheSchema and/or memBwSchema match the
		// existing schemata if ClosID has been specified. This is a more
		// involved than reading the file and doing plain string comparison as
		// the value written in does not necessarily match what gets read out
		// (leading zeros, cache id ordering etc).

		if r.L3CacheSchema != "" {
			if err := writeFile(path, "schemata", r.L3CacheSchema); err != nil {
				return err
			}
		}
		if r.MemBwSchema != "" {
			if err := writeFile(path, "schemata", r.MemBwSchema); err != nil {
				return err
			}
		}
		if len(r.Schemata) > 0 {
			if err := writeFile(path, "schemata", strings.Join(r.Schemata, "\n")); err != nil {
				return err
			}
		}
	}

	return nil
}

func newLastCmdError(err error) error {
	status, err1 := getLastCmdStatus()
	if err1 == nil {
		return fmt.Errorf("%w, last_cmd_status: %s", err, status)
	}
	return err
}


Review comment (performance):
I wonder if it will be better to rewrite `writeFile` to accept multiple data:
```go
func writeFile(dir, file, data... string) error {
...
    for _, d := range data {
        if d == "" {
            continue
        }
        err := f.WriteString(d)
        ...
    }
    ...
}
```

and then use it like this here:
```go
err := writeFile(path, "schemata", r.L3CacheSchema, r.MemBwSchema, r.Schemata...)
```

Code after:
		// involved than reading the file and doing plain string comparison as
		// the value written in does not necessarily match what gets read out
		// (leading zeros, cache id ordering etc).
		var schemata strings.Builder
		for _, s := range append([]string{r.L3CacheSchema, r.MemBwSchema}, r.Schemata...) {
			if s != "" {
				schemata.WriteString(s)
				schemata.WriteString("\n")
			}
		}

		if schemata.Len() > 0 {
			path := filepath.Join(m.GetPath(), "schemata")
			if err := os.WriteFile(path, []byte(schemata.String()), 0o600); err != nil {
				return newLastCmdError(fmt.Errorf("intelrdt: unable to write %q: %w", schemata.String(), err))
			}
		}
	}

	return nil
}

func newLastCmdError(err error) error {
	status, err1 := getLastCmdStatus()
	if err1 == nil {
		return fmt.Errorf("%w, last_cmd_status: %s", err, status)
	}
	return err
}
Repository: opencontainers/runc
File: libcontainer/configs/intelrdt.go (Go)

Code before:
package configs

type IntelRdt struct {
	// The identity for RDT Class of Service
	ClosID string `json:"closID,omitempty"`

	// Schemata is a generic field to specify schemata file in the resctrl
	// filesystem. Each element represents one line written to the schemata file.
	// NOTE: Overrides hschemas specified in the L3CacheSchema and/or MemBwSchema
	Schemata []string `json:"schemata,omitempty"`

	// The schema for L3 cache id and capacity bitmask (CBM)
	// Format: "L3:<cache_id0>=<cbm0>;<cache_id1>=<cbm1>;..."
	L3CacheSchema string `json:"l3_cache_schema,omitempty"`

	// The schema of memory bandwidth per L3 cache id
	// Format: "MB:<cache_id0>=bandwidth0;<cache_id1>=bandwidth1;..."
	// The unit of memory bandwidth is specified in "percentages" by
	// default, and in "MBps" if MBA Software Controller is enabled.
	MemBwSchema string `json:"memBwSchema,omitempty"`
}


Review comment (performance):
```suggestion
	// NOTE: Overrides hschemas specified in the L3CacheSchema and/or MemBwSchema.
```

Code after:
package configs

type IntelRdt struct {
	// The identity for RDT Class of Service
	ClosID string `json:"closID,omitempty"`

	// Schemata is a generic field to specify schemata file in the resctrl
	// filesystem. Each element represents one line written to the schemata file.
	Schemata []string `json:"schemata,omitempty"`

	// The schema for L3 cache id and capacity bitmask (CBM)
	// Format: "L3:<cache_id0>=<cbm0>;<cache_id1>=<cbm1>;..."
	L3CacheSchema string `json:"l3_cache_schema,omitempty"`

	// The schema of memory bandwidth per L3 cache id
	// Format: "MB:<cache_id0>=bandwidth0;<cache_id1>=bandwidth1;..."
	// The unit of memory bandwidth is specified in "percentages" by
	// default, and in "MBps" if MBA Software Controller is enabled.
	MemBwSchema string `json:"memBwSchema,omitempty"`
}
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
	id     string
	path   string
}

// NewManager returns a new instance of Manager, or nil if the Intel RDT
// functionality is not specified in the config, available from hardware or
// enabled in the kernel.
func NewManager(config *configs.Config, id string, path string) *Manager {
	if config.IntelRdt == nil {
		return nil
	}

	rootPath, err := Root()
	if err != nil {
		return nil
	}
	// NOTE: Should we check if the path provided as arg matches the path
	// constructed below? If not, we're screwed as we've effectively lost resctrl
	// control of the container (e.g. because the resctrl fs was unmounted or
	// remounted elsewhere). All operations are deemed to fail.
	if path == "" {
		clos := id
		if config.IntelRdt.ClosID != "" {
			clos = config.IntelRdt.ClosID
		}
		path = filepath.Join(rootPath, clos)
	}

	return newManager(config, id, path)
}

// newManager is the same as NewManager, except it does not check if the feature
// is actually available. Used by unit tests that mock intelrdt paths.
func newManager(config *configs.Config, id string, path string) *Manager {
	return &Manager{
		config: config,
		id:     id,
		path:   path,
	}
}

const (
	intelRdtTasks = "tasks"
)

var (
	// The flag to indicate if Intel RDT/CAT is enabled
	catEnabled bool
	// The flag to indicate if Intel RDT/MBA is enabled
	mbaEnabled bool


Review comment (security):
I know this is just moved from the previous code, but we need to avoid `filepath.Join` in runc as it is vulnerable to all sorts of attacks.

I think we should at least be using `securejoin.SecureJoin` here...

Code after:
	config *configs.Config
	id     string
	path   string
}

// NewManager returns a new instance of Manager, or nil if the Intel RDT
// functionality is not specified in the config, available from hardware or
// enabled in the kernel.
func NewManager(config *configs.Config, id string, path string) *Manager {
	if config.IntelRdt == nil {
		return nil
	}

	rootPath, err := Root()
	if err != nil {
		return nil
	}
	// NOTE: Should we check if the path provided as arg matches the path
	// constructed below? If not, we're screwed as we've effectively lost resctrl
	// control of the container (e.g. because the resctrl fs was unmounted or
	// remounted elsewhere). All operations are deemed to fail.
	if path == "" {
		clos := id
		if config.IntelRdt.ClosID != "" {
			clos = config.IntelRdt.ClosID
		}
		if path, err = securejoin.SecureJoin(rootPath, clos); err != nil {
			return nil
		}
	}

	return newManager(config, id, path)
}

// newManager is the same as NewManager, except it does not check if the feature
// is actually available. Used by unit tests that mock intelrdt paths.
func newManager(config *configs.Config, id string, path string) *Manager {
	return &Manager{
		config: config,
		id:     id,
		path:   path,
	}
}

const (
	intelRdtTasks = "tasks"
)

var (
	// The flag to indicate if Intel RDT/CAT is enabled
	catEnabled bool
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
			if err := os.Remove(m.GetPath()); err != nil && !os.IsNotExist(err) {
				return err
			}
			m.path = ""
		} else if monPath := m.GetMonPath(); monPath != "" {
			// If ClosID is not specified the possible monintoring group was
			// removed with the CLOS above.
			if err := os.Remove(monPath); err != nil && !os.IsNotExist(err) {
				return err
			}
		}
	}
	return nil
}

// GetPath returns Intel RDT path to save in a state file and to be able to
// restore the object later.
func (m *Manager) GetPath() string {
	return m.path
}

// GetMonPath returns path of the monitoring group of the container. Returns an
// empty string if the container does not have a individual dedicated
// monitoring group.
func (m *Manager) GetMonPath() string {
	if closPath := m.GetPath(); closPath != "" && m.config.IntelRdt.EnableMonitoring {
		path, err := securejoin.SecureJoin(filepath.Join(closPath, "mon_groups"), m.id)
		if err != nil {
			return ""
		}
		return path
	}
	return ""
}

// GetStats returns statistics for Intel RDT.
func (m *Manager) GetStats() (*Stats, error) {
	// If intelRdt is not specified in config
	if m.config.IntelRdt == nil {
		return nil, nil
	}

	m.mu.Lock()
	defer m.mu.Unlock()
	stats := newStats()

	rootPath, err := Root()
	if err != nil {
		return nil, err
	}
	// The read-only L3 cache and memory bandwidth schemata in root

Review comment (refactor):
Checking a `bool` is faster so why don't we check it first? Also, to simplify the code:

```go
if !m.config.IntelRdt.EnableMonitoring {
	return ""
}
closPath := m.GetPath()
if closPath == "" {
	return ""
}
...
```

Code after:
			return err
		}
	}
	return nil
}

// GetPath returns Intel RDT path to save in a state file and to be able to
// restore the object later.
func (m *Manager) GetPath() string {
	return m.path
}

// GetMonPath returns path of the monitoring group of the container. Returns an
// empty string if the container does not have a individual dedicated
// monitoring group.
func (m *Manager) GetMonPath() string {
	if !m.config.IntelRdt.EnableMonitoring {
		return ""
	}
	closPath := m.GetPath()
	if closPath == "" {
		return ""
	}

	return filepath.Join(closPath, "mon_groups", m.id)
}

// GetStats returns statistics for Intel RDT.
func (m *Manager) GetStats() (*Stats, error) {
	// If intelRdt is not specified in config
	if m.config.IntelRdt == nil {
		return nil, nil
	}

	m.mu.Lock()
	defer m.mu.Unlock()
	stats := newStats()

	rootPath, err := Root()
	if err != nil {
		return nil, err
	}
	// The read-only L3 cache and memory bandwidth schemata in root
	tmpRootStrings, err := getIntelRdtParamString(rootPath, "schemata")
	if err != nil {
		return nil, err
	}
	schemaRootStrings := strings.Split(tmpRootStrings, "\n")

	// The L3 cache and memory bandwidth schemata in container's clos group
	containerPath := m.GetPath()
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt.go (Go)

Code before:
				return err
			}
			m.path = ""
		} else if monPath := m.GetMonPath(); monPath != "" {
			// If ClosID is not specified the possible monintoring group was
			// removed with the CLOS above.
			if err := os.Remove(monPath); err != nil && !os.IsNotExist(err) {
				return err
			}
		}
	}
	return nil
}

// GetPath returns Intel RDT path to save in a state file and to be able to
// restore the object later.
func (m *Manager) GetPath() string {
	return m.path
}

// GetMonPath returns path of the monitoring group of the container. Returns an
// empty string if the container does not have a individual dedicated
// monitoring group.
func (m *Manager) GetMonPath() string {
	if closPath := m.GetPath(); closPath != "" && m.config.IntelRdt.EnableMonitoring {
		path, err := securejoin.SecureJoin(filepath.Join(closPath, "mon_groups"), m.id)
		if err != nil {
			return ""
		}
		return path
	}
	return ""
}

// GetStats returns statistics for Intel RDT.
func (m *Manager) GetStats() (*Stats, error) {
	// If intelRdt is not specified in config
	if m.config.IntelRdt == nil {
		return nil, nil
	}

	m.mu.Lock()
	defer m.mu.Unlock()
	stats := newStats()

	rootPath, err := Root()
	if err != nil {
		return nil, err
	}
	// The read-only L3 cache and memory bandwidth schemata in root
	tmpRootStrings, err := getIntelRdtParamString(rootPath, "schemata")

Review comment (suggestion):
This assumes m.id can contain unsafe elements like "../", it looks like this can't be the case since `id` is container ID, which we validate in `validateID` and reject such stuff.

So, this might be simplified to just `path.Join` (or `filepath.Join`), or even `closPath + "/mon_groups/" + m.id`

Code after:
		}
	}
	return nil
}

// GetPath returns Intel RDT path to save in a state file and to be able to
// restore the object later.
func (m *Manager) GetPath() string {
	return m.path
}

// GetMonPath returns path of the monitoring group of the container. Returns an
// empty string if the container does not have a individual dedicated
// monitoring group.
func (m *Manager) GetMonPath() string {
	if !m.config.IntelRdt.EnableMonitoring {
		return ""
	}
	closPath := m.GetPath()
	if closPath == "" {
		return ""
	}

	return filepath.Join(closPath, "mon_groups", m.id)
}

// GetStats returns statistics for Intel RDT.
func (m *Manager) GetStats() (*Stats, error) {
	// If intelRdt is not specified in config
	if m.config.IntelRdt == nil {
		return nil, nil
	}

	m.mu.Lock()
	defer m.mu.Unlock()
	stats := newStats()

	rootPath, err := Root()
	if err != nil {
		return nil, err
	}
	// The read-only L3 cache and memory bandwidth schemata in root
	tmpRootStrings, err := getIntelRdtParamString(rootPath, "schemata")
	if err != nil {
		return nil, err
	}
	schemaRootStrings := strings.Split(tmpRootStrings, "\n")

	// The L3 cache and memory bandwidth schemata in container's clos group
	containerPath := m.GetPath()
	tmpStrings, err := getIntelRdtParamString(containerPath, "schemata")
Repository: opencontainers/runc
File: libcontainer/intelrdt/intelrdt_test.go (Go)

Code before:
	value := values[0]

	if value != memBwScSchemeAfter {
		t.Fatal("Got the wrong value, set 'schemata' failed.")
	}
}

func TestApply(t *testing.T) {
	const closID = "test-clos"

	tests := []struct {
		name     string
		testFunc func(*intelRdtTestUtil)
	}{
		{
			name: "failure because non-pre-existing CLOS",
			testFunc: func(helper *intelRdtTestUtil) {
				helper.config.IntelRdt = &configs.IntelRdt{
					ClosID: closID,
				}

				closPath := filepath.Join(intelRdtRoot, closID)
				intelrdt := newManager(helper.config, "", closPath)
				if err := intelrdt.Apply(1234); err == nil {
					t.Fatal("unexpected success when applying pid")
				}
				if _, err := os.Stat(closPath); err == nil {
					t.Fatal("closid dir should not exist")
				}
			},
		},
		{
			name: "CLOS dir should be created if some schema has been specified",
			testFunc: func(helper *intelRdtTestUtil) {
				helper.config.IntelRdt = &configs.IntelRdt{
					ClosID:        closID,
					L3CacheSchema: "L3:0=f",
				}

				closPath := filepath.Join(intelRdtRoot, closID)
				intelrdt := newManager(helper.config, "", closPath)
				if err := intelrdt.Apply(1235); err != nil {
					t.Fatalf("Apply() failed: %v", err)
				}

				pids, err := getIntelRdtParamString(closPath, "tasks")
				if err != nil {
					t.Fatalf("failed to read tasks file: %v", err)
				}
				if pids != "1235" {
					t.Fatalf("unexpected tasks file, expected '1235', got %q", pids)

Review comment (suggestion):
It seems this can be part of the `for ... range tests` loop,a s it's c&p in every  `testFunc()`. Yes, one needs a slightly different param, but that can be part of the test struct, I think.

Code after:
				},
			},
			schemataAfter: []string{
				"L3:0=80;1=7f",
				"MB:0=2000;1=4000",
				"L2:0=ff00;1=ff",
				"L3:0=c0;1=3f",
			},
		},
	}

	for _, tc := range tcs {
		t.Run(tc.name, func(t *testing.T) {
			helper := NewIntelRdtTestUtil(t)
			helper.config.IntelRdt = tc.config

			intelrdt := newManager(helper.config, "", helper.IntelRdtPath)
			if err := intelrdt.Set(helper.config); err != nil {
				t.Fatal(err)
			}

			tmpStrings, err := getIntelRdtParamString(helper.IntelRdtPath, "schemata")
			if err != nil {
				t.Fatalf("Failed to parse file 'schemata' - %s", err)
			}
			values := strings.Split(tmpStrings, "\n")

			if slices.Compare(values, tc.schemataAfter) != 0 {
				t.Fatalf("Got the wrong value, expected %v, got %v", tc.schemataAfter, values)
			}
		})
	}
}

func TestApply(t *testing.T) {
	const pid = 1234
	tests := []struct {
		name            string
		config          configs.IntelRdt
		precreateClos   bool
		isError         bool
		postApplyAssert func(*Manager)
	}{
		{
			name: "failure because non-pre-existing CLOS",
			config: configs.IntelRdt{
				ClosID: "non-existing-clos",
			},
			isError: true,
			postApplyAssert: func(m *Manager) {
				if _, err := os.Stat(m.path); err == nil {