-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix linting issues #185
Fix linting issues #185
Conversation
@@ -1,8 +1,8 @@ | |||
module github.com/crc-org/vfkit/tools | |||
|
|||
go 1.21 | |||
go 1.22.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau once we take this in and then update vfkit
for crc then on crc side also go 1.22.x will be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in vfkit/tools
, not in vfkit
, crc
does not use vfkit/tools
so this should not force a 1.22.x update in crc.
I tried with this in crc: go mod edit --replace github.com/crc-org/vfkit=github.com/cfergeau/vfkit@lint
followed by go mod tidy
, and go.mod is still using go 1.21
.
looks like it need rebase other than that /lgtm |
Rebased, which cleared the /lgtm /o\ |
Does this macOS-13 build failure is expected? |
Also I just merged #164 one so let's rebase on top of it? |
It's not expected, let's see if re-running it will succeed (rebase does not seem required) |
golangci-lint v1.60.3 reports potential int overflows when values are downcasted to a 'smaller' int type. This commit addresses conversion of uintptr file descriptors to int. It assumes casting an uintptr file descriptor to int will always be fine on Unix-like systems. This fixes: pkg/vf/virtio.go:205:37: G115: integer overflow conversion uintptr -> int (gosec) attr, _ := unix.IoctlGetTermios(int(f.Fd()), unix.TIOCGETA) ^ pkg/vf/virtio.go:219:33: G115: integer overflow conversion uintptr -> int (gosec) return unix.IoctlSetTermios(int(f.Fd()), unix.TIOCSETA, attr) ^ pkg/vf/virtionet.go:67:37: G115: integer overflow conversion uintptr -> int (gosec) if err = syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, syscall.SO_SNDBUF, 1*1024*1024); err != nil { Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
vsock port is an uint32, and there are potential overflow issues when converting from int/uint to uint32. This commit tries to address this. The `port` argument to pkg/config.TimeSyncNew and pkg/config.VirtioVsockNew is still an `uint` to avoid breaking external API which is used by github.com/crc-org/crc/pkg/drivers/vfkit/driver_darwin.go The uint -> uint32 changes in the pkg/config.TimeSync and pkg/config.VirtioVsock are also API breaks, but hopefully there are no impacted users, and go recommendations allow such breakage in 0.x versions. If this causes too many issues to external users, this can be reverted. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
This commit should fix these errors from recent golangci-lint: Error: pkg/config/virtio.go:273:23: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(fmt.Sprintf("unexpected value for virtio-input %s option: %s", option.key, option.value)) ^ Error: pkg/config/virtio.go:318:23: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(fmt.Sprintf("Invalid value for virtio-gpu %s: %s", option.key, option.value)) ^ Error: pkg/config/virtio.go:325:23: printf: non-constant format string in call to fmt.Errorf (govet) return fmt.Errorf(fmt.Sprintf("Invalid value for virtio-gpu %s: %s", option.key, option.value)) ^ Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
While golangci/golangci-lint-action is convenient to use, it has the side-effect of always using the latest golangci-lint release, which can introduce new failures at any time. If we use `make lint`, the golangci-lint version we tested with will be used, and we can control when to updated to newer golangci-lint version, and address the new warnings. For example, golangci-lint v1.60.2 introduces new checks for int overflows, which I'd rather address at a later time. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Instead of using the latest go stable release, we can tell the setup-go action to use the go version specified in the go.mod file. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
All green this time! |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: praveenkumar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR updates golangci-lint to the latest version, and fixes the various new issues reported by new version, most notably the int overflow ones.
This also stops using the golangci-lint action to avoid automatic updates of golangci-lint, since these often need code changes which we might not be ready to make.
The changes will be done instead through a go.mod update. The PRs will be created automatically by dependabot, but we can choose when to merge the change.