golangci-lint: migrate configuration to v2 schema #510
golangci-lint: migrate configuration to v2 schema #510openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
Conversation
8c57e59 to
f42d4ee
Compare
eb0be84 to
bf6d445
Compare
cfergeau
left a comment
There was a problem hiding this comment.
Overall looks good, some parts can likely be improved, either to avoid "ignore" annotations, or to avoid some checks by refactoring the code.
| log.Warnf("size exceeds max limit. Resetting to: %d", math.MaxInt32) | ||
| size = math.MaxUint32 | ||
| } | ||
| binary.BigEndian.PutUint32(buf, uint32(size)) //#nosec: G115. Safely checked |
There was a problem hiding this comment.
slightly worried about adding too much code in Write, txBuf, … as this could have a performance impact (but I haven’t measured it, it’s likely to not be impactful).
Haven’t given much thought to this part of the PR, so I don’t know if we can handle this differently.
There was a problem hiding this comment.
We could change the signature of Write method to Write(buf []byte, size uint32). This would mean we would only need to check in hyperkitProtocol.Write for uint16
There was a problem hiding this comment.
I suspect these lines
gvisor-tap-vsock/pkg/tap/switch.go
Lines 169 to 170 in f341afd
uint32 before calling into the function?
There was a problem hiding this comment.
yes, that's right. We can perform conversion here and then call the Write function. We can return an error in case of an invalid value. However, there's still hyperkitProtocol which needs uint16. We could also skip Write in case of an invalid size (but I think this is not correct):
if size > math.MaxUint32 {
log.Errorf("size exceeds max limit.")
} else {
binary.BigEndian.PutUint32(buf, uint32(size))
}
ff7795f to
1799cfb
Compare
|
/retest |
cfergeau
left a comment
There was a problem hiding this comment.
2 minor comments left :)
Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>
Signed-off-by: Gunjan Vyas <vyasgun20@gmail.com>
cfergeau
left a comment
There was a problem hiding this comment.
/lgtm
/approve
/hold
(until the last round of CI clears)
pkg/tap/switch.go
Outdated
| return err | ||
| } | ||
| atomic.AddUint64(&e.Sent, uint64(pkt.Size())) | ||
| atomic.AddUint64(&e.Sent, uint64(size)) //#nosec:G115. Safely checked |
There was a problem hiding this comment.
This nosec annotation is not needed on my system (I pushed this change to your branch)
| return nil, errors.Wrap(err, "cannot create tap endpoint") | ||
| } | ||
| networkSwitch := tap.NewSwitch(configuration.Debug, configuration.MTU) | ||
| networkSwitch := tap.NewSwitch(configuration.Debug, mtu) |
There was a problem hiding this comment.
Interestingly, tap.Switch no longer seem to be using the mtu, all use is now in the LinkEndpoint. This could be cleaned up in a followup PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau, vyasgun The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
|
/unhold |
7db13d1
into
containers:main
Migrate configuration to golangci-lint v2 schema