Skip to content
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

Merged
merged 6 commits into from
Aug 30, 2024
Merged

Fix linting issues #185

merged 6 commits into from
Aug 30, 2024

Conversation

cfergeau
Copy link
Collaborator

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.

@@ -1,8 +1,8 @@
module github.com/crc-org/vfkit/tools

go 1.21
go 1.22.1
Copy link
Member

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.

Copy link
Collaborator Author

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.

@praveenkumar
Copy link
Member

looks like it need rebase

other than that /lgtm

@cfergeau
Copy link
Collaborator Author

looks like it need rebase

other than that /lgtm

Rebased, which cleared the /lgtm /o\

@praveenkumar
Copy link
Member

Does this macOS-13 build failure is expected?

@praveenkumar
Copy link
Member

Also I just merged #164 one so let's rebase on top of it?

@cfergeau
Copy link
Collaborator Author

cfergeau commented Aug 29, 2024

Does this macOS-13 build failure is expected?

It's not expected, let's see if re-running it will succeed (rebase does not seem required)
EDIT: rebase would get rid of the stale macos11 build, so I'll do this now :)

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>
@cfergeau
Copy link
Collaborator Author

It's not expected, let's see if re-running it will succeed

All green this time!

@praveenkumar
Copy link
Member

/lgtm
/approve

Copy link

openshift-ci bot commented Aug 30, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 8207112 into crc-org:main Aug 30, 2024
6 checks passed
@cfergeau cfergeau deleted the lint branch August 30, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants