Conversation
- Replace Popen with subprocess.run() for cleaner code and better resource handling across all devd scripts
- Use proper raw string prefix (r"") for regex patterns
- Remove deprecated close_fds parameter (default True in Python 3)
- Change devd priority from 100 to 5 for faster event handling
auto-switch.py:
- Create marker file /tmp/link-down-{nic} for coordination with link-up.py
- Replace os.system() calls with subprocess.run()
link-up.py:
- Simplify logic using marker file from auto-switch.py
- Remove complex /tmp/network-{nic} state tracking
- Use explicit service calls instead of shell command strings
setup-nic.py:
- Use sysrc command instead of manual rc.conf file writing
- Fix wlan configuration logic to check wlans_{nic} only once
- Use appropriate pccard_ether action (startchildren for WiFi, start forEthernet)
Contributor
Reviewer's GuideRefactors the devd helper scripts to use subprocess.run and more explicit service invocations, simplifies NIC/link state handling via a shared marker file, switches NIC setup to sysrc and proper pccard_ether actions, tightens regex usage, lowers devd notification priority to improve responsiveness, and bumps the package version to 6.8. Sequence diagram for updated link state handling between auto-switch.py and link-up.pysequenceDiagram
participant devd
participant networkmgr_conf
participant auto_switch_py as auto-switch.py
participant link_up_py as link-up.py
participant netif_service as service_netif
participant routing_service as service_routing
participant dhclient_service as service_dhclient
participant ifconfig_cmd as ifconfig
participant sysrc_cmd as sysrc
participant marker_file as tmp_link_down_nic
devd->>networkmgr_conf: IFNET LINK_DOWN event
networkmgr_conf->>auto_switch_py: exec auto-switch.py nic
auto_switch_py->>ifconfig_cmd: ifconfig nic
auto_switch_py->>sysrc_cmd: sysrc -n ifconfig_nic
auto_switch_py->>auto_switch_py: evaluate link status and DHCP
alt nic inactive and another nic active
auto_switch_py->>netif_service: service netif stop nic
auto_switch_py->>marker_file: create /tmp/link-down-nic
alt nic uses DHCP
auto_switch_py->>ifconfig_cmd: ifconfig other_nic
auto_switch_py->>dhclient_service: service dhclient restart other_nic
else nic uses static config
auto_switch_py->>routing_service: service routing restart
end
else nic still active or no fallback
auto_switch_py-->>devd: exit without changes
end
devd->>networkmgr_conf: IFNET LINK_UP event
networkmgr_conf->>link_up_py: exec link-up.py nic
alt marker file exists
link_up_py->>marker_file: check /tmp/link-down-nic
link_up_py->>ifconfig_cmd: ifconfig nic
alt nic has no inet address
link_up_py->>netif_service: service netif start nic
end
link_up_py->>routing_service: service routing restart
link_up_py->>dhclient_service: service dhclient restart nic
link_up_py->>marker_file: remove /tmp/link-down-nic
else no marker file (boot or cold plug)
link_up_py->>dhclient_service: service dhclient quietstart nic
end
Flow diagram for updated setup-nic.py NIC configuration logicflowchart TD
A[start setup-nic.py with nic] --> B[Load rc_conf_content]
B --> C[Check if nic matches wifi_driver_regex]
C -->|wifi driver| D[Ensure wpa_supplicant.conf exists and permissions set]
C -->|not wifi driver| K[Ethernet branch]
D --> E[Check if wlans_nic entry exists in rc_conf_content]
E -->|missing| F[For wlanNum in range 0..8]
F --> G[Check if wlanN not in rc_conf_content]
G -->|free wlanN| H[sysrc wlans_nic='wlanN']
H --> I[sysrc ifconfig_wlanN='WPA DHCP']
I --> J[Break loop]
G -->|in use| F
E -->|exists| L
J --> L[Wifi configured]
L --> M[Run /etc/pccard_ether nic startchildren]
M --> N[exit]
K --> O[Check if ifconfig_nic exists in rc_conf_content]
O -->|missing| P[sysrc ifconfig_nic=DHCP]
O -->|exists| Q
P --> Q[Ethernet configured]
Q --> R[Run /etc/pccard_ether nic start]
R --> S[exit]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In link-up.py, the new subprocess.run(['ifconfig', nic], ...) call never requests a PIPE/captured output, so nic_ifconfig becomes None and the subsequent 'inet ' checks will fail; add stdout=PIPE (or capture_output=True) as done in the other scripts.
- The updated not_nics_regex in auto-switch.py looks less strict than before (e.g.,
[0-9]instead of[0-9]+, andvm-[a-z]instead ofvm-[a-z]+), which may cause valid interfaces to be filtered incorrectly; consider tightening this regex to match the previous intent. - In setup-nic.py, the sysrc call for Ethernet uses ifconfig_{nic}=DHCP without quotes while the WiFi-related sysrc calls use quoted values; align the Ethernet call (e.g., ifconfig_{nic}="DHCP") to avoid unexpected parsing differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In link-up.py, the new subprocess.run(['ifconfig', nic], ...) call never requests a PIPE/captured output, so nic_ifconfig becomes None and the subsequent 'inet ' checks will fail; add stdout=PIPE (or capture_output=True) as done in the other scripts.
- The updated not_nics_regex in auto-switch.py looks less strict than before (e.g., `[0-9]` instead of `[0-9]+`, and `vm-[a-z]` instead of `vm-[a-z]+`), which may cause valid interfaces to be filtered incorrectly; consider tightening this regex to match the previous intent.
- In setup-nic.py, the sysrc call for Ethernet uses ifconfig_{nic}=DHCP without quotes while the WiFi-related sysrc calls use quoted values; align the Ethernet call (e.g., ifconfig_{nic}="DHCP") to avoid unexpected parsing differences.
## Individual Comments
### Comment 1
<location> `src/link-up.py:22-26` </location>
<code_context>
exit(0)
-nic_ifconfig = Popen(
+nic_ifconfig = run(
['ifconfig', nic],
stdout=PIPE,
- close_fds=True,
universal_newlines=True
-).stdout.read()
+).stdout
-dhcp = Popen(
</code_context>
<issue_to_address>
**issue (bug_risk):** run() call for ifconfig never captures stdout, so nic_ifconfig will be None and the inet check will fail at runtime.
Because `run()` is called without `stdout=PIPE` (or `capture_output=True`), `CompletedProcess.stdout` will be `None`, and `'inet ' not in nic_ifconfig` will raise a `TypeError`. Add `stdout=PIPE` (and optionally `check=True`) to the `run()` call to match the previous `Popen` behavior of capturing output.
</issue_to_address>
### Comment 2
<location> `src/auto-switch.py:15-16` </location>
<code_context>
-not_nics_regex = r"(enc|lo|fwe|fwip|tap|plip|pfsync|pflog|ipfw|tun|sl|faith|wlan" \
- r"ppp|bridge|wg)[0-9]+(\s*)|vm-[a-z]+(\s*)"
+not_nics_regex = r"(enc|lo|fwe|fwip|tap|plip|pfsync|pflog|ipfw|tun|sl|faith|" \
+ r"ppp|bridge|wg)[0-9]|vm-[a-z]"
-default_nic = Popen(
</code_context>
<issue_to_address>
**issue (bug_risk):** The updated not_nics_regex no longer matches full interface names correctly and is likely too permissive.
The previous pattern used `[0-9]+(\s*)` and `vm-[a-z]+(\s*)`, while the new one uses only `[0-9]` and `vm-[a-z]`. This now matches only single-digit suffixes / single-letter names, drops the optional trailing whitespace, and allows partial matches (e.g. only `lo1` from `lo10`). Restoring the `+` quantifier (and whitespace handling) or anchoring to the full interface name would avoid these incorrect and overly permissive matches.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Updated subprocess.run with `stdout=PIPE` for capturing output - Removed unnecessary `close_fds` parameter (default True in Python 3)
github-project-automation
bot
moved this from To triage
to Done
in NetworkMGR Issue Traker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
auto-switch.py:
link-up.py:
setup-nic.py:
Summary by Sourcery
Streamline devd-based NIC management scripts for link up/down handling and configuration, and bump the application version.
New Features:
Bug Fixes:
Enhancements:
Chores: