Skip to content

Conversation

@ym
Copy link
Contributor

@ym ym commented Jun 30, 2025

@ym ym requested a review from Copilot June 30, 2025 17:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a mutex to protect the logSuppressionCounter map in UsbGadget, ensuring thread-safe updates.

  • Added a sync.Mutex field (logSuppressionLock) to the UsbGadget struct.
  • Wrapped logWithSupression and resetLogSuppressionCounter in lock/unlock calls.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
internal/usbgadget/utils.go Wrapped counter access in logWithSupression and resetLogSuppressionCounter with a mutex
internal/usbgadget/usbgadget.go Added logSuppressionLock sync.Mutex to UsbGadget
Comments suppressed due to low confidence (1)

internal/usbgadget/utils.go:84

  • There are no tests covering concurrent use of logWithSupression and resetLogSuppressionCounter. Please add test cases that simulate concurrent calls to verify thread-safety and correct suppression behavior.
func (u *UsbGadget) logWithSupression(counterName string, every int, logger *zerolog.Logger, err error, msg string, args ...interface{}) {

Comment on lines 109 to 111
if _, ok := u.logSuppressionCounter[counterName]; !ok {
u.logSuppressionCounter[counterName] = 0
}
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

In resetLogSuppressionCounter, you only set the counter to zero when the key is missing. To truly reset the counter, assign u.logSuppressionCounter[counterName] = 0 unconditionally (remove the if check).

Suggested change
if _, ok := u.logSuppressionCounter[counterName]; !ok {
u.logSuppressionCounter[counterName] = 0
}
u.logSuppressionCounter[counterName] = 0

Copilot uses AI. Check for mistakes.
Comment on lines +85 to +86
u.logSuppressionLock.Lock()
defer u.logSuppressionLock.Unlock()
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The mutex is held for the entire logWithSupression call, including the actual logging I/O. Consider narrowing the lock scope to just the map reads/writes before releasing it, so you don't block other goroutines during logging.

Suggested change
u.logSuppressionLock.Lock()
defer u.logSuppressionLock.Unlock()
var counter int
u.logSuppressionLock.Lock()

Copilot uses AI. Check for mistakes.
@ym ym merged commit 73f5659 into dev Jul 1, 2025
5 checks passed
@IDisposable IDisposable deleted the fix/logWithSupressionlock branch September 22, 2025 23:40
ym added a commit to ym/jetkvm-kvm that referenced this pull request Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants