Skip to content

Errcheck linting fixes #2781

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 25, 2021
Merged

Errcheck linting fixes #2781

merged 7 commits into from
May 25, 2021

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 1, 2021 •
edited
Loading

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

addresses part of #2627

thaJeztah
thaJeztah commented Feb 1, 2021
View reviewed changes
tty.go

// Close closes all open fds for the tty and/or restores the original
// stdin state to what it was prior to the container execution
func (t *tty) Close() error {
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@kolyshkin @AkihiroSuda ptal

kolyshkin
kolyshkin reviewed Feb 1, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Feb 1, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Feb 1, 2021
View reviewed changes
libcontainer/configs/config_test.go
fHook.Run(state)
err := fHook.Run(state)
if err != nil {
t.Fatal(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 😅

kolyshkin
kolyshkin reviewed Feb 1, 2021
View reviewed changes
kolyshkin
kolyshkin reviewed Feb 1, 2021
View reviewed changes
@kolyshkin
Copy link
Contributor

Mostly LGTM; left some nits.

@kolyshkin
Copy link
Contributor

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
cyphar requested changes Feb 1, 2021
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

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.

libcontainer/container_linux.go Outdated
return err
}
defer unix.Unmount(root, unix.MNT_DETACH)
defer func() { _ = unix.Unmount(root, unix.MNT_DETACH) }()
Copy link
Member

@cyphar cyphar Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

changed those to use //nolint: errcheck

thaJeztah
thaJeztah commented Mar 18, 2021
View reviewed changes
@thaJeztah
Copy link
Member Author

rebased

@thaJeztah thaJeztah mentioned this pull request Apr 1, 2021
Closed
@thaJeztah thaJeztah requested review from cyphar and kolyshkin April 2, 2021 07:51
@thaJeztah
Copy link
Member Author

@cyphar @kolyshkin ptal

AkihiroSuda
AkihiroSuda previously approved these changes Apr 21, 2021
View reviewed changes
@AkihiroSuda
Copy link
Member

Needs rebase

thaJeztah added 6 commits May 20, 2021 14:17
@thaJeztah
errcheck: libcontainer/integration
a899505 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: libcontainer/configs
7e7ff87 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: libcontainer/nsenter
463ee5e 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: libcontainer
b45fbd4 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: tty
3b31e3e 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: signals
0c65f83 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
errcheck: utils
df0206a 
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

rebased 👍

@AkihiroSuda AkihiroSuda added this to the 1.0.0 milestone May 24, 2021
@AkihiroSuda
Copy link
Member

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

cyphar
cyphar approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

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

@cyphar cyphar closed this in ed47810 May 25, 2021
@cyphar cyphar merged commit ed47810 into opencontainers:master May 25, 2021
@cyphar
Copy link
Member

cyphar commented May 25, 2021

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.

@cyphar cyphar mentioned this pull request May 25, 2021
Merged
@thaJeztah thaJeztah deleted the errcheck_fixes branch May 25, 2021 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cyphar cyphar cyphar approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@kolyshkin kolyshkin Awaiting requested review from kolyshkin

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @kolyshkin @AkihiroSuda @cyphar