Skip to content

Add support for RFC 2132 Message option in NAK handling - #718

Open
ColinMcInnes wants to merge 4 commits into
NetworkConfiguration:masterfrom
ColinMcInnes:master
Open

Add support for RFC 2132 Message option in NAK handling#718
ColinMcInnes wants to merge 4 commits into
NetworkConfiguration:masterfrom
ColinMcInnes:master

Conversation

@ColinMcInnes

Copy link
Copy Markdown
Contributor

If the dhcpcd server sends Option 56 (message) as part of a DHCPv4 NAK, pass it on to the exit hook environment so it can be acted on if necessary.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 4d17d77c-8b87-48c0-8e3d-061077bec664

📥 Commits

Reviewing files that changed from the base of the PR and between 324f1e1 and 860739c.

📒 Files selected for processing (2)
  • src/dhcp.c
  • src/dhcp.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/dhcp.h
  • src/dhcp.c

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


Walkthrough

DHCP NAK responses now store RFC 2132 option 56 text in DHCP state. make_env exports the text as message for NAK hooks, and the hook documentation describes the variable.

Changes

DHCP NAK message export

Layer / File(s) Summary
Capture and manage NAK messages
src/dhcp.h, src/dhcp.c
The DHCP state stores the NAK message. NAK handling captures the DHO_MESSAGE option, and DHCP cleanup frees the stored text.
Export and document the message
src/script.c, hooks/dhcpcd-run-hooks.8.in
make_env exports the stored message as message=<value> for NAK events, then clears it. The hook documentation describes the variable.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 86073

This localized change passes the DHCPv4 NAK message option to the exit-hook environment; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant DHCP_Server as DHCP server
  participant dhcp_handledhcp
  participant dhcp_state
  participant make_env
  participant Script_Hook as script hook
  DHCP_Server->>dhcp_handledhcp: Send NAK with DHO_MESSAGE
  dhcp_handledhcp->>dhcp_state: Store message text
  dhcp_handledhcp->>dhcp_state: Drop lease
  make_env->>dhcp_state: Read message text
  make_env->>Script_Hook: Export message=<value>
  make_env->>dhcp_state: Free and clear message
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies support for the RFC 2132 Message option during DHCP NAK handling, which matches the main changeset.
Description check ✅ Passed The description accurately states that DHCPv4 NAK Option 56 values are passed to the exit hook environment.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ 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.

@ColinMcInnes

Copy link
Copy Markdown
Contributor Author

recheck

Comment thread src/dhcp.c
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.

1 participant