Review and address golang-ci linting errors

[thaJeztah]

We recently added golang-ci-lint, which runs on pull requests and on master.

Currently, looks like the linter on PR's only checks the changed lines, but found various issues in the existing code on master (see #2618 (comment)); the linter picked up the problem in https://github.com/opencontainers/runc/runs/1190924966

Looks like that output is a good list of things to work on;

Error: `userFromOS` is unused (deadcode)
Error: `groupFromOS` is unused (deadcode)
Error: `loadConfig` is unused (deadcode)
Error: `removePath` is unused (deadcode)
Error: `wildcard` is unused (deadcode)
Error: Error return value of `os.Mkdir` is not checked (errcheck)
Error: Error return value of `os.Symlink` is not checked (errcheck)
Error: Error return value of `console.ClearONLCR` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `io.Copy` is not checked (errcheck)
Error: Error return value of `cmd.Process.Kill` is not checked (errcheck)
Error: Error return value of `cmd.Wait` is not checked (errcheck)
Error: Error return value of `fHook.Run` is not checked (errcheck)
Error: Error return value of `join` is not checked (errcheck)
Error: Error return value of `os.Symlink` is not checked (errcheck)
Error: Error return value of `dbusConnection.ResetFailedUnit` is not checked (errcheck)
Error: Error return value of `dbusConnection.ResetFailedUnit` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `criuClientCon.CloseWrite` is not checked (errcheck)
Error: Error return value of `Cgroupfs` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `m.Freeze` is not checked (errcheck)
Error: Error return value of `p.wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.cmd.Wait` is not checked (errcheck)
Error: Error return value of `p.manager.Destroy` is not checked (errcheck)
Error: Error return value of `p.intelRdtManager.Destroy` is not checked (errcheck)
Error: Error return value of `p.wait` is not checked (errcheck)
Error: Error return value of `signalAllProcesses` is not checked (errcheck)
Error: Error return value of `unix.Unmount` is not checked (errcheck)
Error: Error return value of `cleanupTmp` is not checked (errcheck)
Error: Error return value of `selinux.SetKeyLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetExecLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetKeyLabel` is not checked (errcheck)
Error: Error return value of `selinux.SetExecLabel` is not checked (errcheck)
Error: Error return value of `i.c.initProcess.signal` is not checked (errcheck)
Error: Error return value of `p.Wait` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `showFile` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `container.Destroy` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `unmountOp` is not checked (errcheck)
Error: Error return value of `container1.Destroy` is not checked (errcheck)
Error: Error return value of `container2.Destroy` is not checked (errcheck)
Error: Error return value of `container1.Destroy` is not checked (errcheck)
Error: Error return value of `container2.Destroy` is not checked (errcheck)
Error: Error return value of `console.ClearONLCR` is not checked (errcheck)
Error: Error return value of `h.Write` is not checked (errcheck)
Error: Error return value of `client.Write` is not checked (errcheck)
Error: ineffectual assignment to `paths` (ineffassign)
Error: `si_code` is unused (structcheck)
Error: `pad` is unused (structcheck)
Error: `si_signo` is unused (structcheck)
Error: `si_errno` is unused (structcheck)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `ok` (gosimple)
Error: S1002: should omit comparison to bool constant, can be simplified to `!symLink` (gosimple)
Error: S1032: should use sort.Ints(...) instead of sort.Sort(sort.IntSlice(...)) (gosimple)
Error: S1032: should use sort.Ints(...) instead of sort.Sort(sort.IntSlice(...)) (gosimple)
Error: S1023: redundant `return` statement (gosimple)
Error: S1023: redundant `return` statement (gosimple)
Error: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
Error: S1017: should replace this `if` statement with an unconditional `strings.TrimPrefix` (gosimple)
Error: S1006: should use for {} instead of for true {} (gosimple)
Error: S1008: should use 'return err == nil' instead of 'if err != nil { return false }; return true' (gosimple)
Error: S1021: should merge variable declaration with assignment on next line (gosimple)
Error: S1021: should merge variable declaration with assignment on next line (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout2.String() instead of string(stdout2.Bytes()) (gosimple)
Error: S1030: should use stdout.String() instead of string(stdout.Bytes()) (gosimple)
Error: S1030: should use stdout2.String() instead of string(stdout2.Bytes()) (gosimple)
Error: SA4006: this value of `process` is never used (staticcheck)
Error: SA4000: identical expressions on the left and right side of the '-' operator (staticcheck)
Error: SA5001: should check returned error before deferring file.Close() (staticcheck)
Error: SA9002: file mode '777' evaluates to 01411; did you mean '0777'? (staticcheck)
Error: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead.  (staticcheck)
Error: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead.  (staticcheck)
Error: SA1019: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (staticcheck)
Error: func `(*linuxContainer).deleteState` is unused (unused)

Some notes there;

• I'm not sure it the logs output all errors, or a limited list; usually, golang-ci-lint is configured to limit some issues if they occur too many times
• Depending on the above, may consider disabling errcheck, or configure a rule/regex for error-handling we think is acceptable to ignore

There's also some false positives #2625 (comment)

Error: userFromOS is unused (deadcode)
Error: groupFromOS is unused (deadcode)

I think those functions are actually used for windows environment. Doesn't the current CI script recognize windows environment?

w.r.t Windows: it may be only checking for the platform it runs on. IIRC, there's also an option to specify which build tags to use when linting. (so multiple combinations of build-tags could be needed to lint everything).

#2625 (comment)

These should also be ignored likely (but could potentially be candidates for inclusion in golang.org/x/sys);

Error: `si_code` is unused (structcheck)
Error: `pad` is unused (structcheck)
Error: `si_signo` is unused (structcheck)
Error: `si_errno` is unused (structcheck)

Action items:

• write some instructions how to run the linter locally, and how to run it to show all errors (not omitting repeated errors)
• review the linter configuration (decide on what linters can be ignored/disabled, perhaps what linters should be added)
• split the list up to a checkbox-style list, so that things can be worked by contributors in parallel
• look for code that could be moved elsewhere (e.g. if there's low-level system things that are generic enough for inclusion in golang.org/x/sys)

[thaJeztah]

@kolyshkin @AkihiroSuda @KentaTada

[thaJeztah]

Interestingly, the output doesn't show the file location where errors occurred; is there a different location where logs can be viewed with more details?

[thaJeztah]

Looks like this is a starting point, but haven't found yet how to see annotations on the correct lines; https://github.com/opencontainers/runc/actions?query=workflow%3Agolangci-lint

[thaJeztah]

Quick try to see how the linter picks up changes in PR's #2628

[AkihiroSuda]

Currently, looks like the linter on PR's only checks the changed lines, but found various issues in the existing code on master

I think we can just disable linter for master and call it a day

[thaJeztah]

Why disable? I think there's various issues it picked up that could be actual things to look for. (not all of them, but there are some that could improve code quality at least)

[ehsundar]

Hi everyone. I go for it.

[kolyshkin]

Addressed by #2694, #2695, #2696.

What's left is #2699, google protobuf issue (checkpoint-restore/go-criu#36), and a bunch of errcheck warnings.

[AkihiroSuda]

@kolyshkin @cyphar

CI has been failing for merge commits into master: https://github.com/opencontainers/runc/runs/1802320146

Do we want to make this green before rc93? I think we can just add // nolint and call it for a day.

[thaJeztah]

opened #2779 to fix the "deadcode" linting. Build is currently broken on master (also #2778); I can have a look after that to see what's remaining. If it's only the errcheck linter, we can have a quick look over those and see if there's actual errors should not be ignoring, otherwise either nolint, or probably better, add a _ = foo.ProducesError() to explicitly ignore if it's not too many

[thaJeztah]

Do we know why the linter doesn't print file/line numbers for the failures? Current output in GH actions isn't very useful currently 🤔

[cyphar]

I would prefer _ = as a fix rather than nolint.

[thaJeztah]

Working on a branch to fix (at least some) of the failures (there's probably quite a lot, so may split it in a couple of PR's to spread the work a bit)

[kolyshkin]

Working on a branch to fix

@thaJeztah make sure to pick up #2696

Do we know why the linter doesn't print file/line numbers for the failures?

Github annotations are used where possible (i.e. in PRs). I thought it works on master, too, but apparently annotations are just for diffs. See also golangci/golangci-lint-action#119

[thaJeztah]

@thaJeztah make sure to pick up #2696

I tried rebasing my branch on top of that one, and it rebased cleanly, so I based it on master again; thanks for the heads up 👍

[kolyshkin]

Do we want to make this green before rc93?

We can't make it entirely green; as per my earlier comment

what's left is #2699, google protobuf issue (checkpoint-restore/go-criu#36), and a bunch of errcheck warnings.

I would not address any of that before rc93 -- the first two are too major and would definitely delay rc93 further than we want to, and the last one (errcheck) is too trivial (so I'd rather have it right after rc93, not before).

[cyphar]

Yeah I wouldn't block rc93 on this. If cleanups get in before we merge everything else, great. If not, it can make it next release.

[kolyshkin]

google protobuf issue (checkpoint-restore/go-criu#36)

Should be fixed by checkpoint-restore/go-criu#43, and once it's in we'll just bump protobuf to 1.4.x

[kolyshkin]

once it's in we'll just bump protobuf to 1.4.x
#2807

The rest of it can wait until post-rc94 I think

[thaJeztah]

The rest of it can wait until post-rc94 I think

#2781 is still pending, and fixes a bunch of them

[kolyshkin]

Looks like this is fully fixed by #2962 and #2962, so closing.