Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc linter nits #2695

Merged
merged 7 commits into from
Dec 4, 2020
Merged

Misc linter nits #2695

merged 7 commits into from
Dec 4, 2020

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Nov 30, 2020

This fixes some sensible linter warnings, and silence a few that do not make much sense. Please see individual commits for details.

With this other PRs (#2694, #2696, #2700), we are down to 3 warnings (plus a lot of errcheck ones which are to be addressed separately):

$ golangci-lint run -D errcheck ./...
libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead.  (staticcheck)
	if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
	          ^
libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead.  (staticcheck)
		if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
		          ^
libcontainer/container_linux.go:32:2: SA1019: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (staticcheck)
	"github.com/golang/protobuf/proto"
	^

The protobuf one needs coordinated changes in go-criu (cc @adrianreber).

The ebfp one is related to cilium/ebpf@5727d65 and probably cilium/ebpf#110

Related to #2627

thaJeztah
thaJeztah previously approved these changes Dec 1, 2020
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

left questions w.r.t. the Windows changes (change doesn't really affect current code, but perhaps we need a tracking issue for it and/or remove the windows code altogether?)

libcontainer/user/lookup_windows.go Outdated Show resolved Hide resolved
libcontainer/user/lookup_windows.go Outdated Show resolved Hide resolved
TBBle
TBBle previously approved these changes Dec 3, 2020
AkihiroSuda
AkihiroSuda previously approved these changes Dec 3, 2020
> libcontainer/cgroups/utils.go:282:4: SA4006: this value of `paths` is never used (staticcheck)
>			paths = make(map[string]string)

Signed-off-by: Kir Kolyshkin <[email protected]>
Commit 4415446 introduces this function which is never used.
Remove it.

This fixes

> libcontainer/container_linux.go:1813:26: func `(*linuxContainer).deleteState` is unused (unused)

Signed-off-by: Kir Kolyshkin <[email protected]>
This fixes the following warnings:

> libcontainer/integration/exec_test.go:369:18: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
>	outputStatus := string(stdout.Bytes())
>	                ^
> libcontainer/integration/exec_test.go:422:18: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
>	outputStatus := string(stdout.Bytes())
>	                ^
> libcontainer/integration/exec_test.go:486:18: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
>	outputGroups := string(stdout.Bytes())
>	                ^
> libcontainer/integration/execin_test.go:191:18: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
>	outputGroups := string(stdout.Bytes())
>	                ^
> libcontainer/integration/execin_test.go:474:9: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
>	out := string(stdout.Bytes())
>	       ^

Signed-off-by: Kir Kolyshkin <[email protected]>
... use the one from unix instead.

Coincidentally, this fixes this warning from gosimple linter:

> libcontainer/integration/exec_test.go:448:2: S1021: should merge variable declaration with assignment on next line (gosimple)
>	var netAdminBit uint
>	^

Signed-off-by: Kir Kolyshkin <[email protected]>
> libcontainer/container_linux.go:683:2: S1021: should merge variable declaration with assignment on next line (gosimple)
> 	var t criurpc.CriuReqType
> 	^

Signed-off-by: Kir Kolyshkin <[email protected]>
> libcontainer/container_linux.go:768:2: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
> 	if strings.HasPrefix(mountDest, c.config.Rootfs) {
> 	^
> libcontainer/container_linux.go:1150:2: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
> 	if strings.HasPrefix(mountDest, c.config.Rootfs) {
> 	^

Signed-off-by: Kir Kolyshkin <[email protected]>
> libcontainer/intelrdt/monitoring.go:24:2: SA5001: should check returned error before deferring file.Close() (staticcheck)
> 	defer file.Close()
> 	^

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

As per conversation in moby/moby#41746 (comment), I separated out the windows part of this to #2700.

@AkihiroSuda AkihiroSuda merged commit 4b055ff into opencontainers:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants