go-selinux: some cleanup/refactor and improve test coverage #190
Conversation
thaJeztah
requested review from
kolyshkin,
mrunalp,
rhatdan and
runcom
as code owners
thaJeztah
force-pushed
the
selinux_cleanup
branch
from
420baf2
to
849474f
Compare
go-selinux/selinux_linux_test.go
Outdated
| if err := SetFileLabel(tmp, con); err != nil { | ||
| err = SetFileLabel(tmp, con) | ||
| if err != nil { |
There was a problem hiding this comment.
I don't understand the reason for this change.
There was a problem hiding this comment.
not a huge one; my editor's linter warns about variables that are shadowed; in this case an err already existed. I could just remove the :, but thought it's more clear to break it in two lines if we're reusing the variable.
There was a problem hiding this comment.
So, in this case the linter warning should be ignored. Yes, we have a variable named err already, and yes, here we create another err which shadows the original one. No, there is no issue here.
Or maybe just remove the :.
go-selinux/selinux_linux_test.go
Outdated
| if err := Chcon(dir, con, true); err != nil { | ||
| err = Chcon(dir, con, true) | ||
| if err != nil { |
go-selinux/selinux_linux.go
Outdated
| @@ -549,30 +549,30 @@ func (l *level) parseLevel(levelStr string) error { | |||
|
|
|||
| // rangeStrToMLSRange marshals a string representation of a range. | |||
| func rangeStrToMLSRange(rangeStr string) (*mlsRange, error) { | |||
| mlsRange := &mlsRange{} | |||
| mlsrange := &mlsRange{} | |||
There was a problem hiding this comment.
can we just use range (or even r) here?
There was a problem hiding this comment.
range is a reserved keyboard. Can use r if we think it's clear enough
go-selinux/selinux_linux.go
Outdated
| @@ -712,11 +712,11 @@ func readWriteCon(fpath string, val string) (string, error) { | |||
|
|
|||
| // peerLabel retrieves the label of the client on the other side of a socket | |||
| func peerLabel(fd uintptr) (string, error) { | |||
| label, err := unix.GetsockoptString(int(fd), unix.SOL_SOCKET, unix.SO_PEERSEC) | |||
| lbl, err := unix.GetsockoptString(int(fd), unix.SOL_SOCKET, unix.SO_PEERSEC) | |||
go-selinux/selinux_linux.go
Outdated
| if lvl := c["level"]; lvl != "" { | ||
| return c["user"] + ":" + c["role"] + ":" + c["type"] + ":" + lvl |
go-selinux/selinux_linux.go
Outdated
| @@ -606,17 +606,17 @@ func bitsetToStr(c *big.Int) string { | |||
| return str | |||
| } | |||
|
|
|||
| func (l1 *level) equal(l2 *level) bool { | |||
There was a problem hiding this comment.
I like l1 and l2 more in this case, since we compare them. A linter is not always right.
|
One thing that bothers me if those linter warnings you fix here will resurface after some time, as apparently we don't have those linters enabled in CI. So, if there is a linter in golangci-lint that warns of these, the PR would make more sense. |
The (set)xxxLabel() functions all were shallow wrappers around writeCon() and readCon(), which as a result both added a bit too much abstraction, but also had to all be stubbed separately. This patch inlines their code to reduce the abstraction and to reduce the stubs. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Rename variables that collided with imports, package variables or types/functions, and updated some code that was shadowing other variables. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- add missing asserts - use t.Error() instead of t.Fatal() to not fail early Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
selinux_cleanup
branch
from
849474f
to
0ee8872
Compare
|
@kolyshkin updated; moved some of the changes to #191, which enables more linters (some of which catching the shadowing issues, although I haven't enabled "strict" mode, which seems to closer match what my IDE shows) |
|
LGTM |
See individual commits for details