Skip to content

Conversation

@rsmarples
Copy link
Member

As this will change periodically.
We only care if the state (address) has been added.

Fixes #440 thanks to JognSmit.

As this will change periodically.
We only care if the state (address) has been added.

Fixes #440 thanks to JognSmit.
@coderabbitai
Copy link

coderabbitai bot commented Nov 21, 2025

Walkthrough

The inet_dhcproutes guard condition in src/ipv4.c was simplified from requiring both DHS_BOUND state and a non-zero added flag to only checking if the STATE_ADDED bit is set. This decouples route processing from the interface's bound state.

Changes

Cohort / File(s) Summary
IPv4 DHCP routes guard condition
src/ipv4.c
Changed inet_dhcproutes check from state->state != DHS_BOUND && !state->added to !(state->added & STATE_ADDED). Removes dependency on DHS_BOUND state; now processes routes when STATE_ADDED bit flag is set regardless of bound state.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Rationale for condition change: Verify that decoupling route processing from DHS_BOUND state resolves the issue without introducing unintended side effects in other renewal scenarios
  • Bitwise flag semantics: Confirm STATE_ADDED bit semantics and ensure the change properly distinguishes added vs. non-added routes
  • Test coverage: Check if existing tests cover DHCP renewal with multiple interfaces to prevent regression of similar race conditions

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'IPv4: Ignore DHCP state when building routes' clearly and accurately summarizes the main code change, which simplifies the inet_dhcproutes check condition by removing the DHS_BOUND state requirement.
Description check ✅ Passed The PR description is related to the changeset, explaining the rationale ('We only care if the state (address) has been added') and linking to issue #440 which documents the problem being fixed.
Linked Issues check ✅ Passed The code change addresses the root cause in issue #440: removing the DHS_BOUND check allows routes to be added/maintained based on the STATE_ADDED flag regardless of DHCP state transitions, preventing unintended route deletion during concurrent interface renewals.
Out of Scope Changes check ✅ Passed The change is narrowly scoped to the inet_dhcproutes check condition in src/ipv4.c, modifying only the guard clause logic to match the linked issue's objective of preventing route loss during state changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dhcp-routes-added

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c48f4c and d25c026.

📒 Files selected for processing (1)
  • src/ipv4.c (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: netbsd (--disable-ipv6)
  • GitHub Check: netbsd (--disable-ipv4, -DSMALL)
  • GitHub Check: netbsd (--disable-arp)
  • GitHub Check: netbsd (--disable-ipv6, -DSMALL)
  • GitHub Check: netbsd (--disable-ipv4)
  • GitHub Check: netbsd (--disable-arp, -DSMALL)
  • GitHub Check: netbsd (--disable-dhcp6, -DSMALL)
  • GitHub Check: netbsd (--disable-ipv4ll, -DSMALL)
  • GitHub Check: netbsd
  • GitHub Check: netbsd (--disable-dhcp6)
  • GitHub Check: freebsd
  • GitHub Check: netbsd (--disable-ipv4ll)
  • GitHub Check: netbsd (-DSMALL)
  • GitHub Check: openbsd
🔇 Additional comments (1)
src/ipv4.c (1)

294-294: LGTM! This change correctly addresses issue #440.

The simplified guard now only checks whether the address has been added (STATE_ADDED bit), removing the DHS_BOUND state requirement. This prevents route deletion during DHCP renewal state transitions (e.g., DHS_RENEWING, DHS_REBINDING), which was causing the "Network is unreachable" errors on multi-interface systems.

The bitwise check !(state->added & STATE_ADDED) is also more appropriate for the flags field and consistent with other usages throughout the code (lines 268-270, 430, 445).

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

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.

Failure to renew with "ps_root_recvmsg: Network is unreachable"

2 participants