handle: add NewHandleWithOptions(), deprecate DisableVFInfoCollection()#1174
handle: add NewHandleWithOptions(), deprecate DisableVFInfoCollection()#1174ti-mo wants to merge 3 commits intovishvananda:mainfrom
Conversation
📝 WalkthroughWalkthroughA refactoring of the netlink package's handle initialization system introducing a global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5797a29 to
03c80dd
Compare
This is not really a configuration option, but rather an internal cache for remembering the lookup fallback behaviour. Signed-off-by: Timo Beckers <timo@incline.eu>
This commit repurposes HandleOptions as a declarative configuration mechanism for Handle. It can be passed to NewHandleWithOptions. It also removes the unreleased Handle.RetryInterrupted and deprecates DisableVFInfoCollection in favor of its HandleOptions counterpart. Signed-off-by: Timo Beckers <timo@incline.eu>
This commit introduces the funcion ConfigureHandle to replace the global package handle with one configured using the given HandleOptions. It can only be called once per process. Signed-off-by: Timo Beckers <timo@incline.eu>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@handle_linux_test.go`:
- Around line 21-27: TestConfigureHandle mutates package globals (pkgHandle and
configDone) and never restores them, breaking isolation; update the test to save
orig := pkgHandle and defer restoring pkgHandle and clearing configDone under
configMu (acquire configMu, set configDone = false, release) so the global state
is returned to its original condition after the test, then run ConfigureHandle
assertions as before. Ensure you reference and restore pkgHandle, and protect
writes to configDone using configMu to avoid races.
In `@handle_linux.go`:
- Around line 197-201: The comment for NewHandleWithOptions is misleading
because it states "If opts is nil" although opts is passed by value as
HandleOptions; update the docstring to reflect that the zero value or empty
HandleOptions will trigger defaults instead of nil. Edit the comment above
NewHandleWithOptions to say something like "If opts is the zero value, default
options will be used" and ensure references to opts elsewhere (if any) use the
same non-nil wording; target the NewHandleWithOptions function and the
HandleOptions type in your update.
- Around line 69-70: The lookupByDump boolean must be made concurrency-safe:
change the Handle.lookupByDump field from a plain bool to sync/atomic.Bool and
add the "sync/atomic" import; then replace direct reads/writes in LinkByName and
LinkByAlias with h.lookupByDump.Load() and h.lookupByDump.Store(true/false)
respectively (use Load where the code currently reads the flag and Store where
it writes it) so concurrent goroutines no longer race on that field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1aebd270-4458-430c-a1b4-86a5efc154a6
📒 Files selected for processing (3)
handle_linux.gohandle_linux_test.golink_linux.go
| func TestConfigureHandle(t *testing.T) { | ||
| orig := pkgHandle | ||
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | ||
| assert.NotEqual(t, orig, pkgHandle) | ||
|
|
||
| assert.Error(t, ConfigureHandle(HandleOptions{})) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other tests that might call ConfigureHandle or depend on pkgHandle state
rg -n "ConfigureHandle|pkgHandle" --type=go -g '*_test.go'Repository: vishvananda/netlink
Length of output: 1161
🏁 Script executed:
# First, let's look at the ConfigureHandle implementation
fd -t f "handle_linux.go" --type=goRepository: vishvananda/netlink
Length of output: 232
🏁 Script executed:
# Get the ConfigureHandle implementation and related global state
rg -n "ConfigureHandle|configDone|configMu" --type=go -B2 -A8 | head -100Repository: vishvananda/netlink
Length of output: 1985
🏁 Script executed:
# Check netlink_test.go setup to understand the cleanup pattern
rg -n "func.*Test|pkgHandle|configDone" handle_linux_test.go --type=go -B1 -A10Repository: vishvananda/netlink
Length of output: 664
🏁 Script executed:
# View the beginning of netlink_test.go to see test setup
head -100 netlink_test.goRepository: vishvananda/netlink
Length of output: 2186
🏁 Script executed:
# Check if there's an init function or test setup in handle_linux_test.go
cat handle_linux_test.goRepository: vishvananda/netlink
Length of output: 694
🏁 Script executed:
# Search for test init or setup patterns
rg -n "func init|func TestMain|func setup|t.Cleanup" --type f -B1 -A3Repository: vishvananda/netlink
Length of output: 88
🏁 Script executed:
# Check for any test files that might initialize ConfigureHandle or set up state
rg "ConfigureHandle|configDone" --type f -lRepository: vishvananda/netlink
Length of output: 88
🏁 Script executed:
# Check if there's a TestMain function that might handle test setup
rg "TestMain" --type f -B2 -A10Repository: vishvananda/netlink
Length of output: 88
🏁 Script executed:
# Look at the full handle_linux_test.go to see all test functions
wc -l handle_linux_test.go && cat handle_linux_test.goRepository: vishvananda/netlink
Length of output: 718
🏁 Script executed:
# Search for any calls to ConfigureHandle in all test files
rg "ConfigureHandle" --glob="*.go"Repository: vishvananda/netlink
Length of output: 430
🏁 Script executed:
# Check if there are any Go test configurations like build tags or test file organization
fd . -name "*.go" -path "*_test.go" | head -20Repository: vishvananda/netlink
Length of output: 293
🏁 Script executed:
# Check the go.mod or test runner configuration
find . -name "go.mod" -o -name "pytest.ini" -o -name "*.yaml" | grep -E "(test|ci)" | head -10Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
# Check the order tests run - look at test file structure
ls -la *_test.go | head -20Repository: vishvananda/netlink
Length of output: 1376
🏁 Script executed:
# Verify the visibility of configDone and configMu
rg "var config|export" handle_linux.go -B1 -A1Repository: vishvananda/netlink
Length of output: 108
🏁 Script executed:
# Check if any test might call ConfigureHandle more than once or in sequence
rg -A20 "func Test" handle_linux_test.goRepository: vishvananda/netlink
Length of output: 607
Test pollutes global state without cleanup, causing failures on test reruns.
TestConfigureHandle sets configDone = true permanently. This breaks test isolation:
- Running with
go test -count=2will fail on the second iteration (second call toConfigureHandlehappens at module init, not in test) - Any future test that calls
ConfigureHandlewill fail if run after this test
Since handle_linux_test.go is in the same package as handle_linux.go, the unexported configDone and configMu are accessible for cleanup.
Suggested fix
func TestConfigureHandle(t *testing.T) {
orig := pkgHandle
+ origDone := configDone
+ t.Cleanup(func() {
+ configMu.Lock()
+ pkgHandle = orig
+ configDone = origDone
+ configMu.Unlock()
+ })
+
assert.NoError(t, ConfigureHandle(HandleOptions{}))
assert.NotEqual(t, orig, pkgHandle)
assert.Error(t, ConfigureHandle(HandleOptions{}))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestConfigureHandle(t *testing.T) { | |
| orig := pkgHandle | |
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | |
| assert.NotEqual(t, orig, pkgHandle) | |
| assert.Error(t, ConfigureHandle(HandleOptions{})) | |
| } | |
| func TestConfigureHandle(t *testing.T) { | |
| orig := pkgHandle | |
| origDone := configDone | |
| t.Cleanup(func() { | |
| configMu.Lock() | |
| pkgHandle = orig | |
| configDone = origDone | |
| configMu.Unlock() | |
| }) | |
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | |
| assert.NotEqual(t, orig, pkgHandle) | |
| assert.Error(t, ConfigureHandle(HandleOptions{})) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@handle_linux_test.go` around lines 21 - 27, TestConfigureHandle mutates
package globals (pkgHandle and configDone) and never restores them, breaking
isolation; update the test to save orig := pkgHandle and defer restoring
pkgHandle and clearing configDone under configMu (acquire configMu, set
configDone = false, release) so the global state is returned to its original
condition after the test, then run ConfigureHandle assertions as before. Ensure
you reference and restore pkgHandle, and protect writes to configDone using
configMu to avoid races.
|
|
||
| lookupByDump bool |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about Handle thread-safety
rg -n -i "concurrent|thread.safe|goroutine" handle_linux.go
# Check if sync/atomic is used elsewhere in the codebase for similar patterns
rg -n "atomic\." --type=goRepository: vishvananda/netlink
Length of output: 825
🏁 Script executed:
#!/bin/bash
# Find the Handle struct definition
rg -n "type Handle struct" --type=goRepository: vishvananda/netlink
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Look at the context around line 69-70 in handle_linux.go
head -n 100 handle_linux.go | tail -n 50Repository: vishvananda/netlink
Length of output: 1710
🏁 Script executed:
#!/bin/bash
# Find LinkByName and LinkByAlias function implementations
rg -n "func.*LinkByName|func.*LinkByAlias" --type=go -A 20Repository: vishvananda/netlink
Length of output: 8871
🏁 Script executed:
#!/bin/bash
# Check for any mutex or synchronization on Handle
rg -n "mutex|RWMutex|Lock|Unlock" handle_linux.goRepository: vishvananda/netlink
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Look for any Handle documentation or comments about concurrency
rg -n -B 5 "type Handle" handle_linux.go | head -n 20Repository: vishvananda/netlink
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Find where lookupByDump is written/assigned
rg -n "lookupByDump\s*=" --type=goRepository: vishvananda/netlink
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Check the complete LinkByName implementation including error handling
sed -n '2022,2055p' link_linux.goRepository: vishvananda/netlink
Length of output: 1016
🏁 Script executed:
#!/bin/bash
# Check the complete LinkByAlias implementation including error handling
sed -n '2070,2110p' link_linux.goRepository: vishvananda/netlink
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Check if there's any Handle concurrency documentation
rg -n -B 10 "type Handle struct" handle_linux.go | head -n 30Repository: vishvananda/netlink
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation saying Handle is not for concurrent use
rg -n -i "not.*concurrent|not.*thread.safe|concurrent.*not|not.*safe" --type=go | head -n 20Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look at package documentation in any doc.go or README-like files
find . -name "*.go" -o -name "README*" -o -name "*.md" | xargs grep -l "Handle" | head -n 5Repository: vishvananda/netlink
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Check if there are any other bool fields in Handle that are modified
rg -n "h\.[a-zA-Z]*\s*=" handle_linux.go | grep -v "h\.options\|h\.sockets"Repository: vishvananda/netlink
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for any concurrency/synchronization patterns in Handle methods
rg -n "Lock|Unlock|mu\." handle_linux.goRepository: vishvananda/netlink
Length of output: 110
Add synchronization for lookupByDump field to prevent data races.
The lookupByDump field is read in LinkByName (line 2023) and LinkByAlias (line 2071) and written in both methods (lines 2047 and 2092) without synchronization. Concurrent access from multiple goroutines can cause data races when the fallback is triggered.
Use sync/atomic.Bool for this field to ensure safe concurrent access, or document that Handle is not safe for concurrent use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@handle_linux.go` around lines 69 - 70, The lookupByDump boolean must be made
concurrency-safe: change the Handle.lookupByDump field from a plain bool to
sync/atomic.Bool and add the "sync/atomic" import; then replace direct
reads/writes in LinkByName and LinkByAlias with h.lookupByDump.Load() and
h.lookupByDump.Store(true/false) respectively (use Load where the code currently
reads the flag and Store where it writes it) so concurrent goroutines no longer
race on that field.
| // NewHandleWithOptions returns a Handle created using the specified options. If | ||
| // opts is nil, default options will be used. | ||
| func NewHandleWithOptions(opts HandleOptions, nlFamilies ...int) (*Handle, error) { | ||
| return newHandle(netns.None(), opts, nlFamilies...) | ||
| } |
There was a problem hiding this comment.
Fix misleading docstring: opts is not a pointer.
The docstring says "If opts is nil, default options will be used" but opts is passed by value (HandleOptions), not as a pointer, so it cannot be nil.
📝 Suggested fix
-// NewHandleWithOptions returns a Handle created using the specified options. If
-// opts is nil, default options will be used.
+// NewHandleWithOptions returns a Handle created using the specified options.
+// Pass an empty HandleOptions{} to use default options.
func NewHandleWithOptions(opts HandleOptions, nlFamilies ...int) (*Handle, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewHandleWithOptions returns a Handle created using the specified options. If | |
| // opts is nil, default options will be used. | |
| func NewHandleWithOptions(opts HandleOptions, nlFamilies ...int) (*Handle, error) { | |
| return newHandle(netns.None(), opts, nlFamilies...) | |
| } | |
| // NewHandleWithOptions returns a Handle created using the specified options. | |
| // Pass an empty HandleOptions{} to use default options. | |
| func NewHandleWithOptions(opts HandleOptions, nlFamilies ...int) (*Handle, error) { | |
| return newHandle(netls.None(), opts, nlFamilies...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@handle_linux.go` around lines 197 - 201, The comment for NewHandleWithOptions
is misleading because it states "If opts is nil" although opts is passed by
value as HandleOptions; update the docstring to reflect that the zero value or
empty HandleOptions will trigger defaults instead of nil. Edit the comment above
NewHandleWithOptions to say something like "If opts is the zero value, default
options will be used" and ensure references to opts elsewhere (if any) use the
same non-nil wording; target the NewHandleWithOptions function and the
HandleOptions type in your update.
There was a problem hiding this comment.
Pull request overview
This PR introduces a declarative HandleOptions configuration flow for netlink Handle creation via NewHandleWithOptions, and adds a one-time configuration entrypoint for the package-global default handle used by the package-level APIs.
Changes:
- Added
NewHandleWithOptions(opts HandleOptions, ...)and repurposedHandleOptionsto carry exported, user-facing settings (DisableVFInfoCollection,RetryInterrupted,NetNS). - Added
ConfigureHandle(opts HandleOptions)to configure the package-widepkgHandleonce per process. - Deprecated
(*Handle).DisableVFInfoCollection()in favor ofHandleOptions.DisableVFInfoCollection, removed the unreleasedRetryInterrupted()handle mutator, and updated link lookup code paths accordingly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
handle_linux.go |
Adds new handle configuration APIs and reshapes HandleOptions usage across handle creation/request construction. |
link_linux.go |
Switches VF info fetching to the new DisableVFInfoCollection option and moves dump-fallback state to Handle.lookupByDump. |
handle_linux_test.go |
Adds a unit test covering the new one-time ConfigureHandle behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewHandleWithOptions returns a Handle created using the specified options. If | ||
| // opts is nil, default options will be used. |
There was a problem hiding this comment.
The doc comment for NewHandleWithOptions says "If opts is nil" but opts is a non-pointer HandleOptions, so it can never be nil. Please adjust the comment (or change the signature to accept *HandleOptions) and clarify what constitutes "default options" (e.g., zero-value options).
| // NewHandleWithOptions returns a Handle created using the specified options. If | |
| // opts is nil, default options will be used. | |
| // NewHandleWithOptions returns a Handle created using the specified options. | |
| // If opts is the zero value of HandleOptions, default options will be used. |
| var configMu sync.Mutex | ||
| var configDone bool | ||
|
|
||
| // ConfigureHandle configures the default, package-wide netlink handle used by | ||
| // the netlink package's global functions like [LinkList] and [AddrList] with | ||
| // the given opts. It does not affect any existing or future [Handle] returned | ||
| // by the library. | ||
| // | ||
| // Returns an error if called more than once per process. | ||
| func ConfigureHandle(opts HandleOptions) error { | ||
| configMu.Lock() | ||
| defer configMu.Unlock() | ||
|
|
||
| if configDone { | ||
| return fmt.Errorf("netlink package handle already configured") | ||
| } | ||
|
|
||
| h, err := NewHandleWithOptions(opts) | ||
| if err != nil { | ||
| return fmt.Errorf("creating handle: %w", err) | ||
| } | ||
|
|
||
| configDone = true | ||
| pkgHandle = h | ||
|
|
||
| return nil |
There was a problem hiding this comment.
ConfigureHandle updates the package-global pkgHandle under a mutex, but pkgHandle is read by many package-level functions without synchronization. Calling ConfigureHandle concurrently with any netlink operation will be a data race under the Go memory model. Consider publishing pkgHandle via an atomic pointer/value (or sync.Once + atomic), or clearly document that ConfigureHandle must be called during single-threaded init before any other goroutine uses the package API.
| orig := pkgHandle | ||
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | ||
| assert.NotEqual(t, orig, pkgHandle) |
There was a problem hiding this comment.
TestConfigureHandle mutates global state (pkgHandle and the hidden configDone flag) and never restores/cleans up. This can leak open netlink sockets and can interfere with other tests in this package (including those that run in parallel). Please add t.Cleanup to close the configured handle and restore pkgHandle and configDone (under configMu) to their original values.
| orig := pkgHandle | |
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | |
| assert.NotEqual(t, orig, pkgHandle) | |
| configMu.Lock() | |
| origHandle := pkgHandle | |
| origConfigDone := configDone | |
| configMu.Unlock() | |
| t.Cleanup(func() { | |
| configMu.Lock() | |
| configuredHandle := pkgHandle | |
| pkgHandle = origHandle | |
| configDone = origConfigDone | |
| configMu.Unlock() | |
| if configuredHandle != nil && configuredHandle != origHandle { | |
| _ = configuredHandle.Close() | |
| } | |
| }) | |
| assert.NoError(t, ConfigureHandle(HandleOptions{})) | |
| assert.NotEqual(t, origHandle, pkgHandle) |
Repurpose HandleOptions as a declarative configuration mechanism for Handle. It can be passed to NewHandleWithOptions.
It also removes the unreleased Handle.RetryInterrupted and deprecates DisableVFInfoCollection in favor of its HandleOptions counterpart.
Summary by CodeRabbit
New Features
Changes
Deprecated
RetryInterrupted()fluent configuration method.