Use Cut more by kolyshkin · Pull Request #254 · opencontainers/selinux

kolyshkin · 2026-04-17T21:54:38Z

Instead of instantiating slices, use strings.Cut or bytes.Cut where approprate, for simpler code and less allocations.

While at it, do some minor refactoring (switch vs if) in a few places.

Copilot

Pull request overview

This PR refactors several SELinux parsing and option-handling paths to use strings.Cut / bytes.Cut instead of SplitN, simplifying logic and reducing intermediate slice creation during parsing.

Changes:

Replace SplitN-based parsing with strings.Cut / bytes.Cut in config and MLS/MCS parsing helpers.
Refactor a few conditional chains into switch/clearer branching for readability.
Update corresponding tests to match the new parsing style.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
go-selinux/selinux_linux_test.go	Refactors `DupSecOpt` assertions to use `strings.Cut` and a `switch` on the parsed key.
go-selinux/selinux_linux.go	Uses `bytes.Cut` / `strings.Cut` in config parsing, category parsing, level parsing, and range parsing.
go-selinux/label/label_linux.go	Uses `strings.Cut` for label option parsing and simplifies validation/assignment logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR refactors several SELinux parsing/code paths to use strings.Cut / bytes.Cut instead of SplitN/manual indexing, aiming to simplify logic and reduce allocations.

Changes:

Replace several SplitN/Index-based parsing patterns with strings.Cut / bytes.Cut.
Refactor a test loop to use a switch on parsed option keys.
Minor restructuring of MLS range parsing for readability.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
go-selinux/selinux_linux_test.go	Refactors `DupSecOpt` test parsing to use `strings.Cut` + `switch`.
go-selinux/selinux_linux.go	Updates config parsing, category parsing, and MLS range parsing to use `Cut`.
go-selinux/label/label_linux.go	Refactors label option parsing in `InitLabels` to use `strings.Cut`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kolyshkin · 2026-04-17T23:31:00Z

+		key, val, ok := bytes.Cut(line, []byte{'='})
+		if !ok {
 			continue
 		}
-		if bytes.Equal(fields[0], []byte(target)) {
-			return string(bytes.Trim(fields[1], `"`))
+		if string(key) == target {
+			return string(bytes.Trim(val, `"`))


Nope, go escape analysis actually handle this and optimizes out the string conversion.

kolyshkin · 2026-04-17T23:32:42Z

+		key, val, _ := strings.Cut(opt, ":")
+		switch key {
+		case "user":
+			if val != "system_u" {
 				t.Errorf("DupSecOpt Failed user incorrect")
 			}
-			continue
-		}
-		if con[0] == "role" {
-			if con[1] != "system_r" {
+		case "role":
+			if val != "system_r" {
 				t.Errorf("DupSecOpt Failed role incorrect")
 			}
-			continue
-		}
-		if con[0] == "type" {
-			if con[1] != "container_t" {
+		case "type":
+			if val != "container_t" {
 				t.Errorf("DupSecOpt Failed type incorrect")
 			}
-			continue
-		}
-		if con[0] == "level" {
-			if con[1] != "s0:c1,c2" {
+		case "level":
+			if val != "s0:c1,c2" {
 				t.Errorf("DupSecOpt Failed level incorrect")
 			}
-			continue
+		default:
+			t.Errorf("DupSecOpt failed: invalid field %q", key)


Hmm, it will fail either way, so the test case works fine with or without ok check. Or am I missing something?

thaJeztah · 2026-04-18T10:29:52Z

+			if k == "level" || k == "user" {
+				mcon[k] = v
 			}
 		}


Perhaps we should use a switch here as well; it makes it more explicit and easier to read. For example the pcon[k] = v is set unconditionally (for any valid option), which may be intentional, but is also one that can easily slip through if it's not desired.

We can then make the switch exhaustive, making sure we handle all possible options;

k, v, ok := strings.Cut(opt, ":") if !ok || !validOptions[k] { return "", "", fmt.Errorf("bad label option %q, valid options 'disable' or \n'user, role, level, type, filetype' followed by ':' and a value", opt) } switch k { case "filetype": mcon["type"] = v case "level", "user": pcon[k] = v mcon[k] = v default: pcon[k] = v }

And looking at that, I think there may be a corner-case; using disable:<any value> (with a colon) will be silently ignored, because "disable" is a valid option, but we don't handle it after splitting.

If we do, we should also consider defining a type and consts of valid options; in that case, we can enable the "exhaustive" linter, which would invalidate things if we happened to miss handling an option; https://golangci-lint.run/docs/linters/configuration/#exhaustive

type labelOpt string // Valid Label Options const ( optDisable labelOpt = "disable" optType labelOpt = "type" optFiletype labelOpt = "filetype" optUser labelOpt = "user" optRole labelOpt = "role" optLevel labelOpt = "level" )

I tried making the switch exhaustive, but left some TODOs in here, because we need to decide how to handle these cases. I left the special-casing for disable for now, but probably we could roll that up into the switch as well if we check value to be non-empty (per switch) instead of checking for the colon (?);

for _, opt := range options { if labelOpt(opt) == optDisable { // "disable" doesn't expect a value. selinux.ReleaseLabel(mountLabel) return "", selinux.PrivContainerMountLabel(), nil } k, v, ok := strings.Cut(opt, ":") if !ok { // TODO: should value be checked (to be non-empty, or empty (per branch))? return "", "", fmt.Errorf("bad label option %q, valid options 'disable' or \n'user, role, level, type, filetype' followed by ':' and a value", opt) } switch labelOpt(k) { case optFiletype: mcon["type"] = v case optLevel, optUser: pcon[k] = v mcon[k] = v case optType, optRole: pcon[k] = v case optDisable: // This is reached when passing "disable:[<some value>]". // FIXME: decide whether to ignore the value or handle the value, and produce an error. selinux.ReleaseLabel(mountLabel) return "", selinux.PrivContainerMountLabel(), nil default: return "", "", fmt.Errorf("bad label option %q, valid options 'disable' or \n'user, role, level, type, filetype' followed by ':' and a value", opt) } }

I have many unanswered questions while looking at this code. Because of that, and the fact I barely know selinux, I am very hesitant of changing it.

I applied the early return and the second pcon.Get call elimination optimizations that you've suggested below, but frankly I don't want to change it further.

I also think switch here makes things more complicated. Those five lines seem to be fine as they are:

if k == "filetype" { mcon["type"] = v continue } pcon[k] = v if k == "level" || k == "user" { mcon[k] = v }

Yeah that's fair; I might still open a follow-up to get more eyes; I think there's benefits to the switch; having verification that no options are missed, and it makes it more explicit what fields/values are used.

thaJeztah · 2026-04-18T10:52:47Z

We can save some cycles here as well (call pcon.Get() only once); perhaps ever-so-slightly more correct to update processLabel after reserving.

Suggested change

if p := pcon.Get(); p != processLabel {

if pcon["level"] != mcsLevel {

selinux.ReleaseLabel(processLabel)

}

selinux.ReserveLabel(p)

processLabel = p

}

And perhaps use an early return, because none of this code is ran if the container's processLabel was empty;

selinux/go-selinux/label/label_linux.go

Lines 33 to 34 in 4368688

processLabel, mountLabel := selinux.ContainerLabels()

if processLabel != "" {

(assuming that's all intentional, and the mountLabel to be returned as-is);

processLabel, mountLabel := selinux.ContainerLabels() if processLabel == "" { // TODO: some comment here mentioning why we skip all other processing based on processLabel(?) return "", mountLabel, nil }

thaJeztah

LGTM

kolyshkin · 2026-04-22T19:24:43Z

Rebased to include CI bumps from PR #255.

thaJeztah

still LGTM

Copilot

Pull request overview

Refactors several string/byte parsing sites to use strings.Cut / bytes.Cut instead of SplitN, aiming to simplify parsing logic and reduce intermediate allocations in the SELinux label/config handling code.

Changes:

Replace SplitN-based parsing with Cut in config/label parsing helpers.
Refactor MLS range parsing flow to avoid switch len(...) on split results.
Update related tests and label option parsing to match the new parsing approach.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
go-selinux/selinux_linux_test.go	Updates `DupSecOpt` parsing assertions to use `strings.Cut` and a `switch` on key.
go-selinux/selinux_linux.go	Replaces multiple `SplitN` calls with `Cut` for config parsing and MLS/category parsing.
go-selinux/label/label_linux.go	Refactors label option parsing to use `strings.Cut` and streamlines control flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kolyshkin · 2026-04-22T23:04:06Z

-		con := strings.SplitN(opt, ":", 2)
-		if con[0] == "user" {
-			if con[1] != "system_u" {
+		key, val, _ := strings.Cut(opt, ":")


The test will still fail as we want it to.

kolyshkin · 2026-04-22T23:08:31Z

+			return "", selinux.PrivContainerMountLabel(), nil
+		}
+		k, v, ok := strings.Cut(opt, ":")
+		if !ok || !validOptions[k] {


Yes, this would be behavioral change which I want to avoid for now.

The fix is just to remove "disable" from the validOptions map -- so "disable" will be accepted and anything with "disable:" prefix will be rejected. Let's do that in a separate PR.

One other change I though about is when "disable" is specified, no other fields should be specified. Again, this is a potentially breaking change and thus I'm not doing it here.

Yeah; I think this is one of the things we should look at in a follow-up (related to my mention of the switch + better defined enum). It's the case I mentioned earlier;

And looking at that, I think there may be a corner-case; using disable:<any value> (with a colon) will be silently ignored, because "disable" is a valid option, but we don't handle it after splitting.

rhatdan · 2026-04-23T13:45:00Z

Needs rebase

Instead of instantiating slices, use strings.Cut or bytes.Cut where appropriate, for simpler code and less allocations. While at it, do some minor refactoring in a few places. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Use an early return instead of hefty "if" body. 2. Avoid a second call to pcon.Get. Best reviewed with --ignore-all-space. Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2026-04-23T23:45:00Z

Needs rebase

rebased

thaJeztah

LGTM

kolyshkin requested review from cyphar, mrunalp, rhatdan, runcom and thaJeztah as code owners April 17, 2026 21:54

kolyshkin requested a review from Copilot April 17, 2026 21:54

Copilot started reviewing on behalf of kolyshkin April 17, 2026 21:55 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Comment thread go-selinux/selinux_linux.go

kolyshkin force-pushed the cut branch 2 times, most recently from e0b31a4 to 82a5b44 Compare April 17, 2026 22:02

kolyshkin requested a review from Copilot April 17, 2026 23:06

Copilot started reviewing on behalf of kolyshkin April 17, 2026 23:06 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

thaJeztah reviewed Apr 18, 2026

View reviewed changes

kolyshkin force-pushed the cut branch from 82a5b44 to ea3fb4a Compare April 20, 2026 18:13

kolyshkin requested a review from thaJeztah April 21, 2026 23:07

thaJeztah approved these changes Apr 22, 2026

View reviewed changes

kolyshkin force-pushed the cut branch from ea3fb4a to c8967d5 Compare April 22, 2026 19:23

thaJeztah previously approved these changes Apr 22, 2026

View reviewed changes

kolyshkin requested a review from Copilot April 22, 2026 20:32

Copilot started reviewing on behalf of kolyshkin April 22, 2026 20:32 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

kolyshkin and others added 2 commits April 23, 2026 16:06

Use Cut more

d0f37ab

Instead of instantiating slices, use strings.Cut or bytes.Cut where appropriate, for simpler code and less allocations. While at it, do some minor refactoring in a few places. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

InitLabels: refactor

581ebad

1. Use an early return instead of hefty "if" body. 2. Avoid a second call to pcon.Get. Best reviewed with --ignore-all-space. Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin dismissed thaJeztah’s stale review via 581ebad April 23, 2026 23:06

kolyshkin force-pushed the cut branch from c8967d5 to 581ebad Compare April 23, 2026 23:06

thaJeztah approved these changes Apr 23, 2026

View reviewed changes

+		if p := pcon.Get(); p != processLabel {
+			if pcon["level"] != mcsLevel {
+				selinux.ReleaseLabel(processLabel)
+			}
+			selinux.ReserveLabel(p)
+			processLabel = p
+		}

	processLabel, mountLabel := selinux.ContainerLabels()
	if processLabel != "" {

Conversation

kolyshkin commented Apr 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Apr 22, 2026

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 23, 2026

Uh oh!

kolyshkin commented Apr 23, 2026

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah Apr 18, 2026 •

edited

Loading

thaJeztah Apr 22, 2026 •

edited

Loading