Errcheck linting fixes by thaJeztah · Pull Request #2781 · opencontainers/runc

thaJeztah · 2021-02-01T18:42:41Z

Split it to to smaller commit for easier review, but happy to squash if wanted

addresses part of #2627

libcontainer/factory_linux_test.go

thaJeztah · 2021-02-01T18:45:36Z

tty.go

@@ -175,19 +175,19 @@ func (t *tty) ClosePostStart() error {
 func (t *tty) Close() error {


Note that this function never returns an error; wondering if it should either not have the error return, or if one (or more) of the errors should actually be returned (or a multi-error used)

Currently no callers of this care about the error, so I'd make it not return anything.

Having Close() return an error is mostly so that you can map it to io.Closer as well as being "good Go practice". I think there is actually a bug here that we aren't returning Close() errors from any of the Closes we call in the function, but since very few projects check Close() errors I guess it isn't that bad... 🤷‍♂️

thaJeztah · 2021-02-01T18:46:11Z

@kolyshkin @AkihiroSuda ptal

libcontainer/integration/checkpoint_test.go

libcontainer/integration/exec_test.go

kolyshkin · 2021-02-01T19:41:57Z

libcontainer/configs/config_test.go

-	fHook.Run(state)
+	err := fHook.Run(state)
+	if err != nil {
+		t.Fatal(err)


Not important, but the code used to keep checking other cases even after one of them failed, and now it's not.

Fine either way for me, just noticing.

Hmm... but there's only a single check in this test, and if that would fail it would already fail the test.

I think it's correct with this change, so I'll keep it, but feel free to comment if I overlooked something 😅

libcontainer/factory_linux.go

libcontainer/factory_linux_test.go

kolyshkin · 2021-02-01T19:57:16Z

Mostly LGTM; left some nits.

kolyshkin · 2021-02-01T19:58:16Z

Overall, I don't think we should try to make master CI green again before rc93, so I'd rather have this one merged after the release.

cyphar

My main comment is that I'm not sure defer func() { _ = ... }() is worth the annoyance over // nolint -- especially since I'm pretty sure it captures variables differently to a naked defer.

cyphar · 2021-02-01T20:30:49Z

libcontainer/container_linux.go

 		return err
 	}
-	defer unix.Unmount(root, unix.MNT_DETACH)
+	defer func() { _ = unix.Unmount(root, unix.MNT_DETACH) }()


For the defer funcWhichReturnsErr() cases I would go with // nolint because the alternative is quite a bit uglier (and I think in some cases may have strange behaviour with variable values -- because IIRC closures capture variables slightly differently to defer statements).

I was thinking about suggesting something like that, too. Having to add a whole function call to satisfy a linter is borderline overkill.

changed those to use //nolint: errcheck

libcontainer/nsenter/nsenter_test.go

thaJeztah · 2021-04-01T08:34:12Z

rebased

thaJeztah · 2021-04-13T09:04:16Z

@cyphar @kolyshkin ptal

AkihiroSuda · 2021-05-20T06:49:18Z

Needs rebase

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2021-05-20T12:26:48Z

rebased 👍

AkihiroSuda · 2021-05-24T03:56:18Z

@kolyshkin @cyphar PTAL, v1.0.0 should be released with green CI

cyphar

LGTM, though we should probably also enable the golint tests for PRs if it's green now.

cyphar · 2021-05-25T02:35:35Z

Seems that this didn't fix all the lints:

  Running [/home/runner/golangci-lint-1.36.0-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  Error: SA4000: identical expressions on the left and right side of the '-' operator (staticcheck)
  Error: `archStringToNative` is unused (deadcode)
  Error: `memoryAssigned` is unused (deadcode)
  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 `container.Destroy` is not checked (errcheck)
  Error: ineffectual assignment to `bpfType` (ineffassign)

I'll send a PR to fix these today.

This PR contains the following updates: | Package | Type | Update | Change | OpenSSF | |---|---|---|---|---| | [github.com/opencontainers/runc](https://github.com/opencontainers/runc) | require | patch | `v1.4.0` → `v1.4.1` | [![OpenSSF Scorecard](https://api.securityscorecards.dev/projects/github.com/opencontainers/runc/badge)](https://securityscorecards.dev/viewer/?uri=github.com/opencontainers/runc) | --- > ⚠️ **Warning** > > Some dependencies could not be looked up. Check the [Dependency Dashboard](issues/23) for more information. --- ### Release Notes <details> <summary>opencontainers/runc (github.com/opencontainers/runc)</summary> ### [`v1.4.1`](https://github.com/opencontainers/runc/blob/HEAD/CHANGELOG.md#100---2021-06-22) [Compare Source](opencontainers/runc@v1.4.0...v1.4.1) > A wizard is never late, nor is he early, he arrives precisely when he means > to. As runc follows Semantic Versioning, we will endeavour to not make any breaking changes without bumping the major version number of runc. However, it should be noted that Go API usage of runc's internal implementation (libcontainer) is *not* covered by this policy. ##### Removed - Removed libcontainer/configs.Device\* identifiers (deprecated since rc94, use libcontainer/devices). ([#2999](opencontainers/runc#2999)) - Removed libcontainer/system.RunningInUserNS function (deprecated since rc94, use libcontainer/userns). ([#2999](opencontainers/runc#2999)) ##### Deprecated - The usage of relative paths for mountpoints will now produce a warning (such configurations are outside of the spec, and in future runc will produce an error when given such configurations). ([#2917](opencontainers/runc#2917), [#3004](opencontainers/runc#3004)) ##### Fixed - cgroupv2: devices: rework the filter generation to produce consistent results with cgroupv1, and always clobber any existing eBPF program(s) to fix `runc update` and avoid leaking eBPF programs (resulting in errors when managing containers). ([#2951](opencontainers/runc#2951)) - cgroupv2: correctly convert "number of IOs" statistics in a cgroupv1-compatible way. ([#2965](opencontainers/runc#2965), [#2967](opencontainers/runc#2967), [#2968](opencontainers/runc#2968), [#2964](opencontainers/runc#2964)) - cgroupv2: support larger than 32-bit IO statistics on 32-bit architectures. - cgroupv2: wait for freeze to finish before returning from the freezing code, optimize the method for checking whether a cgroup is frozen. ([#2955](opencontainers/runc#2955)) - cgroups/systemd: fixed "retry on dbus disconnect" logic introduced in rc94 - cgroups/systemd: fixed returning "unit already exists" error from a systemd cgroup manager (regression in rc94). ([#2997](opencontainers/runc#2997), [#2996](opencontainers/runc#2996)) ##### Added - cgroupv2: support SkipDevices with systemd driver. ([#2958](opencontainers/runc#2958), [#3019](opencontainers/runc#3019)) - cgroup1: blkio: support BFQ weights. ([#3010](opencontainers/runc#3010)) - cgroupv2: set per-device io weights if BFQ IO scheduler is available. ([#3022](opencontainers/runc#3022)) ##### Changed - cgroup/systemd: return, not ignore, stop unit error from Destroy. ([#2946](opencontainers/runc#2946)) - Fix all golangci-lint failures. ([#2781](opencontainers/runc#2781), [#2962](opencontainers/runc#2962)) - Make `runc --version` output sane even when built with `go get` or otherwise outside of our build scripts. ([#2962](opencontainers/runc#2962)) - cgroups: set SkipDevices during runc update (so we don't modify cgroups at all during `runc update`). ([#2994](opencontainers/runc#2994))  [Unreleased]: opencontainers/runc@v1.3.0-rc.1...HEAD [1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0 [1.2.0]: opencontainers/runc@v1.2.0-rc.1...v1.2.0 [1.1.0]: opencontainers/runc@v1.1.0-rc.1...v1.1.0 [1.0.0]: https://github.com/opencontainers/runc/releases/tag/v1.0.0  [Unreleased 1.0.z]: opencontainers/runc@v1.0.3...release-1.0 [1.0.3]: opencontainers/runc@v1.0.2...v1.0.3 [1.0.2]: opencontainers/runc@v1.0.1...v1.0.2 [1.0.1]: opencontainers/runc@v1.0.0...v1.0.1  [Unreleased 1.1.z]: opencontainers/runc@v1.1.15...release-1.1 [1.1.15]: opencontainers/runc@v1.1.14...v1.1.15 [1.1.14]: opencontainers/runc@v1.1.13...v1.1.14 [1.1.13]: opencontainers/runc@v1.1.12...v1.1.13 [1.1.12]: opencontainers/runc@v1.1.11...v1.1.12 [1.1.11]: opencontainers/runc@v1.1.10...v1.1.11 [1.1.10]: opencontainers/runc@v1.1.9...v1.1.10 [1.1.9]: opencontainers/runc@v1.1.8...v1.1.9 [1.1.8]: opencontainers/runc@v1.1.7...v1.1.8 [1.1.7]: opencontainers/runc@v1.1.6...v1.1.7 [1.1.6]: opencontainers/runc@v1.1.5...v1.1.6 [1.1.5]: opencontainers/runc@v1.1.4...v1.1.5 [1.1.4]: opencontainers/runc@v1.1.3...v1.1.4 [1.1.3]: opencontainers/runc@v1.1.2...v1.1.3 [1.1.2]: opencontainers/runc@v1.1.1...v1.1.2 [1.1.1]: opencontainers/runc@v1.1.0...v1.1.1 [1.1.0-rc.1]: opencontainers/runc@v1.0.0...v1.1.0-rc.1  [Unreleased 1.2.z]: opencontainers/runc@v1.2.9...release-1.2 [1.2.9]: opencontainers/runc@v1.2.8...v1.2.9 [1.2.8]: opencontainers/runc@v1.2.7...v1.2.8 [1.2.7]: opencontainers/runc@v1.2.6...v1.2.7 [1.2.6]: opencontainers/runc@v1.2.5...v1.2.6 [1.2.5]: opencontainers/runc@v1.2.4...v1.2.5 [1.2.4]: opencontainers/runc@v1.2.3...v1.2.4 [1.2.3]: opencontainers/runc@v1.2.2...v1.2.3 [1.2.2]: opencontainers/runc@v1.2.1...v1.2.2 [1.2.1]: opencontainers/runc@v1.2.0...v1.2.1 [1.2.0-rc.3]: opencontainers/runc@v1.2.0-rc.2...v1.2.0-rc.3 [1.2.0-rc.2]: opencontainers/runc@v1.2.0-rc.1...v1.2.0-rc.2 [1.2.0-rc.1]: opencontainers/runc@v1.1.0...v1.2.0-rc.1  [Unreleased 1.3.z]: opencontainers/runc@v1.3.4...release-1.3 [1.3.4]: opencontainers/runc@v1.3.3...v1.3.4 [1.3.3]: opencontainers/runc@v1.3.2...v1.3.3 [1.3.2]: opencontainers/runc@v1.3.1...v1.3.2 [1.3.1]: opencontainers/runc@v1.3.0...v1.3.1 [1.3.0]: opencontainers/runc@v1.3.0-rc.2...v1.3.0 [1.3.0-rc.2]: opencontainers/runc@v1.3.0-rc.1...v1.3.0-rc.2 [1.3.0-rc.1]: opencontainers/runc@v1.2.0...v1.3.0-rc.1  [Unreleased 1.4.z]: opencontainers/runc@v1.4.1...release-1.4 [1.4.1]: opencontainers/runc@v1.4.0...v1.4.1 [1.4.0]: opencontainers/runc@v1.4.0-rc.3...v1.4.0 [1.4.0-rc.3]: opencontainers/runc@v1.4.0-rc.2...v1.4.0-rc.3 [1.4.0-rc.2]: opencontainers/runc@v1.4.0-rc.1...v1.4.0-rc.2 [1.4.0-rc.1]: opencontainers/runc@v1.3.0...v1.4.0-rc.1  [Unreleased 1.5.z]: opencontainers/runc@v1.5.0-rc.1...release-1.5 [1.5.0-rc.1]: opencontainers/runc@v1.4.0...v1.5.0-rc.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At 12:00 AM through 04:59 AM and 10:00 PM through 11:59 PM, Monday through Friday ( * 0-4,22-23 * * 1-5 ), Only on Sunday and Saturday ( * * * * 0,6 ) (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://altlinux.space/stapler/stplr/pulls/361 Co-authored-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space> Co-committed-by: Renovate Bot <stapler-helper-bot@noreply.altlinux.space>

thaJeztah commented Feb 1, 2021

View reviewed changes

kolyshkin reviewed Feb 1, 2021

View reviewed changes

libcontainer/integration/checkpoint_test.go Outdated Show resolved Hide resolved

kolyshkin reviewed Feb 1, 2021

View reviewed changes

libcontainer/integration/exec_test.go Outdated Show resolved Hide resolved

kolyshkin reviewed Feb 1, 2021

View reviewed changes

libcontainer/factory_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Feb 1, 2021

View reviewed changes

libcontainer/factory_linux_test.go Outdated Show resolved Hide resolved

cyphar requested changes Feb 1, 2021

View reviewed changes

thaJeztah force-pushed the errcheck_fixes branch from 06d41c8 to 8f47285 Compare March 13, 2021 15:00

thaJeztah commented Mar 18, 2021

View reviewed changes

libcontainer/nsenter/nsenter_test.go Show resolved Hide resolved

thaJeztah force-pushed the errcheck_fixes branch from 8f47285 to ae3291c Compare March 18, 2021 11:22

thaJeztah force-pushed the errcheck_fixes branch from ae3291c to ded8a28 Compare April 1, 2021 08:33

thaJeztah mentioned this pull request Apr 1, 2021

Review and address golang-ci linting errors #2627

Closed

thaJeztah requested review from cyphar and kolyshkin April 2, 2021 07:51

AkihiroSuda previously approved these changes Apr 21, 2021

View reviewed changes

thaJeztah dismissed AkihiroSuda’s stale review via 9e05d0a April 29, 2021 19:47

thaJeztah force-pushed the errcheck_fixes branch from ded8a28 to 9e05d0a Compare April 29, 2021 19:47

AkihiroSuda added the status/needs-rebase label May 20, 2021

thaJeztah added 6 commits May 20, 2021 14:17

errcheck: libcontainer/integration

a899505

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: libcontainer/configs

7e7ff87

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: libcontainer/nsenter

463ee5e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: libcontainer

b45fbd4

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: tty

3b31e3e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: signals

0c65f83

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

errcheck: utils

df0206a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the errcheck_fixes branch from 9e05d0a to df0206a Compare May 20, 2021 12:21

AkihiroSuda removed the status/needs-rebase label May 20, 2021

AkihiroSuda approved these changes May 20, 2021

View reviewed changes

AkihiroSuda added this to the 1.0.0 milestone May 24, 2021

cyphar approved these changes May 25, 2021

View reviewed changes

cyphar closed this in ed47810 May 25, 2021

cyphar merged commit ed47810 into opencontainers:master May 25, 2021

cyphar mentioned this pull request May 25, 2021

*: clean up remaining golangci-lint failures #2962

Merged

thaJeztah deleted the errcheck_fixes branch May 25, 2021 07:22

		@@ -175,19 +175,19 @@ func (t *tty) ClosePostStart() error {
		func (t *tty) Close() error {

Conversation

thaJeztah commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

kolyshkin Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cyphar Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 1, 2021

Uh oh!

Uh oh!

Uh oh!

kolyshkin Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Feb 1, 2021

Uh oh!

kolyshkin commented Feb 1, 2021

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

cyphar Feb 1, 2021 • 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!

kolyshkin Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Apr 1, 2021

Uh oh!

thaJeztah commented Apr 13, 2021

Uh oh!

AkihiroSuda commented May 20, 2021

Uh oh!

thaJeztah commented May 20, 2021

Uh oh!

AkihiroSuda commented May 24, 2021

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

cyphar commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Feb 1, 2021 •

edited

Loading

cyphar Feb 1, 2021 •

edited

Loading