Errcheck linting fixes #2781

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

@@ -1299,7 +1299,7 @@ func (c *linuxContainer) Restore(process *Process, criuOpts *CriuOpts) error {
 	if err != nil {
 		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 <[email protected]>

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.

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 Outdated 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 kolyshkin and cyphar 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 <[email protected]>

errcheck: libcontainer/configs

7e7ff87

Signed-off-by: Sebastiaan van Stijn <[email protected]>

errcheck: libcontainer/nsenter

463ee5e

Signed-off-by: Sebastiaan van Stijn <[email protected]>

errcheck: libcontainer

b45fbd4

Signed-off-by: Sebastiaan van Stijn <[email protected]>

errcheck: tty

3b31e3e

Signed-off-by: Sebastiaan van Stijn <[email protected]>

errcheck: signals

0c65f83

Signed-off-by: Sebastiaan van Stijn <[email protected]>

errcheck: utils

df0206a

Signed-off-by: Sebastiaan van Stijn <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errcheck linting fixes #2781

Errcheck linting fixes #2781

thaJeztah commented Feb 1, 2021 •

edited

Loading

thaJeztah Feb 1, 2021

kolyshkin Feb 1, 2021

cyphar Feb 1, 2021

thaJeztah commented Feb 1, 2021

kolyshkin Feb 1, 2021

thaJeztah Mar 13, 2021

kolyshkin commented Feb 1, 2021

kolyshkin commented Feb 1, 2021

cyphar left a comment

cyphar Feb 1, 2021 •

edited

Loading

kolyshkin Feb 2, 2021

thaJeztah Mar 13, 2021

thaJeztah commented Apr 1, 2021

thaJeztah commented Apr 13, 2021

AkihiroSuda commented May 20, 2021

thaJeztah commented May 20, 2021

AkihiroSuda commented May 24, 2021

cyphar left a comment

cyphar commented May 25, 2021

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

Errcheck linting fixes #2781

Errcheck linting fixes #2781

Conversation

thaJeztah commented Feb 1, 2021 • edited Loading

thaJeztah Feb 1, 2021

Choose a reason for hiding this comment

kolyshkin Feb 1, 2021

Choose a reason for hiding this comment

cyphar Feb 1, 2021

Choose a reason for hiding this comment

thaJeztah commented Feb 1, 2021

kolyshkin Feb 1, 2021

Choose a reason for hiding this comment

thaJeztah Mar 13, 2021

Choose a reason for hiding this comment

kolyshkin commented Feb 1, 2021

kolyshkin commented Feb 1, 2021

cyphar left a comment

Choose a reason for hiding this comment

cyphar Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

kolyshkin Feb 2, 2021

Choose a reason for hiding this comment

thaJeztah Mar 13, 2021

Choose a reason for hiding this comment

thaJeztah commented Apr 1, 2021

thaJeztah commented Apr 13, 2021

AkihiroSuda commented May 20, 2021

thaJeztah commented May 20, 2021

AkihiroSuda commented May 24, 2021

cyphar left a comment

Choose a reason for hiding this comment

cyphar commented May 25, 2021

thaJeztah commented Feb 1, 2021 •

edited

Loading

cyphar Feb 1, 2021 •

edited

Loading