Skip to content

handle: add NewHandleWithOptions(), deprecate DisableVFInfoCollection()#1174

Open
ti-mo wants to merge 3 commits intovishvananda:mainfrom
ti-mo:tb/handle-options
Open

handle: add NewHandleWithOptions(), deprecate DisableVFInfoCollection()#1174
ti-mo wants to merge 3 commits intovishvananda:mainfrom
ti-mo:tb/handle-options

Conversation

@ti-mo
Copy link
Copy Markdown
Contributor

@ti-mo ti-mo commented Mar 18, 2026

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

  • Added a global configuration function for handle initialization with one-time setup verification.
  • Introduced new handle options for namespace management and VF information collection control.

Changes

  • Refactored handle creation methods to consolidate configuration through the new options system.
  • Updated internal lookup mechanisms for improved namespace handling.

Deprecated

  • Removed the RetryInterrupted() fluent configuration method.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 18, 2026

📝 Walkthrough

Walkthrough

A refactoring of the netlink package's handle initialization system introducing a global ConfigureHandle function with one-time initialization semantics, restructuring HandleOptions to expose configuration fields (DisableVFInfoCollection, RetryInterrupted, NetNS), adding NewHandleWithOptions constructor, and reimplementing existing constructors to use the new pattern while updating internal logic for attribute inclusion decisions.

Changes

Cohort / File(s) Summary
Handle initialization refactoring
handle_linux.go
Added global ConfigureHandle function with mutex-protected one-time initialization. Restructured HandleOptions with exported fields (DisableVFInfoCollection, RetryInterrupted, NetNS) and removed internal fields from public API. Introduced NewHandleWithOptions constructor. Reimplemented NewHandle, NewHandleAt, and NewHandleAtFrom to route through updated initialization logic. Modified DisableVFInfoCollection() fluent method and removed RetryInterrupted() fluent setter.
Handle configuration testing
handle_linux_test.go
Added test for ConfigureHandle to verify one-time initialization behavior and that subsequent calls return an error.
Link lookup logic
link_linux.go
Updated LinkByName, LinkByAlias, and LinkByIndex to read from h.lookupByDump field instead of h.options.lookupByDump. Changed VF info attribute inclusion condition from h.options.collectVFInfo to !h.options.DisableVFInfoCollection. Modified error handling in execGetLink to set h.lookupByDump instead of mutating options.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Poem

🐰 A handle refined with gentle care,
Global states shared everywhere,
Options now dance in the light,
One-time init gets it right,
Refactored links feel so fair!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: adding NewHandleWithOptions() and deprecating DisableVFInfoCollection(), which are the primary API modifications in this changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ti-mo added 3 commits April 3, 2026 11:16
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>
@ti-mo ti-mo force-pushed the tb/handle-options branch from 1c0d4e6 to 1b5de88 Compare April 3, 2026 09:32
@ti-mo ti-mo marked this pull request as ready for review April 3, 2026 09:32
Copilot AI review requested due to automatic review settings April 3, 2026 09:32
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 188504c and 1b5de88.

📒 Files selected for processing (3)
  • handle_linux.go
  • handle_linux_test.go
  • link_linux.go

Comment on lines +21 to +27
func TestConfigureHandle(t *testing.T) {
orig := pkgHandle
assert.NoError(t, ConfigureHandle(HandleOptions{}))
assert.NotEqual(t, orig, pkgHandle)

assert.Error(t, ConfigureHandle(HandleOptions{}))
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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=go

Repository: 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 -100

Repository: 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 -A10

Repository: vishvananda/netlink

Length of output: 664


🏁 Script executed:

# View the beginning of netlink_test.go to see test setup
head -100 netlink_test.go

Repository: 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.go

Repository: 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 -A3

Repository: 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 -l

Repository: 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 -A10

Repository: 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.go

Repository: 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 -20

Repository: 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 -10

Repository: vishvananda/netlink

Length of output: 45


🏁 Script executed:

# Check the order tests run - look at test file structure
ls -la *_test.go | head -20

Repository: vishvananda/netlink

Length of output: 1376


🏁 Script executed:

# Verify the visibility of configDone and configMu
rg "var config|export" handle_linux.go -B1 -A1

Repository: 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.go

Repository: 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:

  1. Running with go test -count=2 will fail on the second iteration (second call to ConfigureHandle happens at module init, not in test)
  2. Any future test that calls ConfigureHandle will 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.

Suggested change
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.

Comment on lines +69 to +70

lookupByDump bool
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 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=go

Repository: vishvananda/netlink

Length of output: 825


🏁 Script executed:

#!/bin/bash
# Find the Handle struct definition
rg -n "type Handle struct" --type=go

Repository: 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 50

Repository: 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 20

Repository: 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.go

Repository: 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 20

Repository: vishvananda/netlink

Length of output: 497


🏁 Script executed:

#!/bin/bash
# Find where lookupByDump is written/assigned
rg -n "lookupByDump\s*=" --type=go

Repository: vishvananda/netlink

Length of output: 148


🏁 Script executed:

#!/bin/bash
# Check the complete LinkByName implementation including error handling
sed -n '2022,2055p' link_linux.go

Repository: vishvananda/netlink

Length of output: 1016


🏁 Script executed:

#!/bin/bash
# Check the complete LinkByAlias implementation including error handling
sed -n '2070,2110p' link_linux.go

Repository: 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 30

Repository: 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 20

Repository: 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 5

Repository: 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.go

Repository: 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.

Comment on lines +197 to +201
// 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...)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
// 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.

Copy link
Copy Markdown

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 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 repurposed HandleOptions to carry exported, user-facing settings (DisableVFInfoCollection, RetryInterrupted, NetNS).
  • Added ConfigureHandle(opts HandleOptions) to configure the package-wide pkgHandle once per process.
  • Deprecated (*Handle).DisableVFInfoCollection() in favor of HandleOptions.DisableVFInfoCollection, removed the unreleased RetryInterrupted() 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.

Comment on lines +197 to +198
// NewHandleWithOptions returns a Handle created using the specified options. If
// opts is nil, default options will be used.
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +41
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
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +24
orig := pkgHandle
assert.NoError(t, ConfigureHandle(HandleOptions{}))
assert.NotEqual(t, orig, pkgHandle)
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
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