[dhcpmon] Apply small packet and C++ hardening fixes by Xichen96 · Pull Request #96 · sonic-net/sonic-dhcpmon

Xichen96 · 2026-07-26T06:30:35Z

Description of PR

Collect the small packet-path and C++ consistency fixes found during the F2
review:

Bound each raw-socket libevent callback to 64 packets.
Reset the recvfrom() address length before every receive.
Rename three file-scope helpers whose names begin with _.
Replace four nullptr comparisons with the daemon's established NULL
style.

These changes are independent of CONFIG_DB listening and do not add or remove
command-line options.

Work item tracking

Microsoft ADO (number only): 38615412

Type of change

Approach

What is the motivation for this PR?

Draining an unbounded packet backlog in one callback can monopolize its socket
event loop. Reinitializing the value-result socklen_t follows the
recvfrom() contract.

C++ reserves names beginning with _ in the global namespace for the
implementation. The three affected helpers already use static linkage, so
descriptive names identify their purpose without relying on reserved
identifiers.

The four nullptr comparisons were the remaining pointer-style outliers from
merged PR #62.

How did you do it?

Process at most 64 queued packets per callback invocation.
Reset socklen_t to sizeof(sockaddr_ll) before each recvfrom().
Leave queued packets readable so libevent schedules another callback.
Rename:
- _increase_cache_counter() to increase_single_cache_counter().
- _addr_is_primary() to addr_is_primary_impl().
- _compile_bpf_prog() to compile_bpf_prog().
Replace the four CONFIG_DB result-pointer nullptr comparisons with NULL.

How did you verify/test it?

Azure PR CI will validate the consolidated head on amd64, arm64, and armhf.
No local compilation is used.

Any platform specific information?

No.

Back port request

None. This targets master only.

Tested branch

master

Test result

The earlier behavioral stack passed regular master DHCPv4/DHCPv6 regression
and F2 traffic, convergence, and counter-clear scenarios without a dhcpmon
restart. Exact-head acceptance awaits the consolidated PR build.

Documentation

Not applicable.

Review

Review the single commit:

076df9a [dhcpmon]: Apply small packet and C++ hardening fixes

mssonicbld · 2026-07-26T06:30:43Z

/azp run

azure-pipelines · 2026-07-26T06:30:44Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-26T06:30:52Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR improves sonic-dhcpmon’s raw-socket packet receive path to keep the libevent loop responsive under bursty traffic, while also correcting recvfrom() address-length handling.

Changes:

Limit each libevent read callback to processing at most 64 packets, leaving remaining packets queued so other events can run.
Reset the socklen_t address length to sizeof(sockaddr_ll) before each recvfrom() call to avoid reusing a modified length.

mssonicbld · 2026-07-26T08:13:32Z

/azp run

azure-pipelines · 2026-07-26T08:13:42Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

mssonicbld · 2026-07-31T17:58:02Z

/azp run

azure-pipelines · 2026-07-31T17:58:11Z

Azure Pipelines: Successfully started running 1 pipeline(s).

azure-pipelines · 2026-07-31T17:58:22Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

🟢 Ready to approve

The change is small, self-contained, and corrects a concrete fairness issue (bounded draining) plus a correctness issue (socklen_t reuse) without introducing new risky behavior.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Xichen96 · 2026-07-31T18:13:13Z

-    while ((buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT, (struct sockaddr *)&sll, &slen)) > 0) 
-    {
+    for (int packet_count = 0; packet_count < MAX_PACKETS_PER_CALLBACK; packet_count++) {
+        slen = sizeof(sll);


Moved the declaration and initialization into the loop. The per-iteration initialization remains necessary because recvfrom() treats socklen_t as a value-result parameter and may modify it.

Xichen96 · 2026-07-31T18:13:35Z

+    for (int packet_count = 0; packet_count < MAX_PACKETS_PER_CALLBACK; packet_count++) {
+        slen = sizeof(sll);
+        buffer_sz = recvfrom(fd, sock_info.buffer, sock_info.snaplen, MSG_DONTWAIT,
+                             (struct sockaddr *)&sll, &slen);


the following lines can be shortened, like the previous version

Updated to assign recvfrom() inside the break condition, matching the previous compact form.

mssonicbld · 2026-07-31T18:17:17Z

/azp run

azure-pipelines · 2026-07-31T18:17:27Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🟢 Ready to approve

The change is small, locally scoped to the callback loop, aligns with libevent EV_READ|EV_PERSIST behavior, and matches the stated goal without introducing observable correctness issues.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Files reviewed: 1/1 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Bound each raw-socket callback to 64 packets and reset the recvfrom address length for every receive attempt. Rename three file-scope helpers that begin with underscores because C++ reserves those identifiers in the global namespace; static linkage already marks them as internal. Replace four nullptr comparisons with NULL to match the daemon's established style. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39f979be-d826-4d5c-949a-f20abb58bb83 Signed-off-by: Xichen96 <lukelin0907@gmail.com>

mssonicbld · 2026-07-31T19:20:40Z

/azp run

azure-pipelines · 2026-07-31T19:20:50Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🟡 Not ready to approve

The updated addr_is_primary_impl() documentation claims “false if … not found” but the implementation returns true on “not found,” which is misleading for future maintenance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/util.cpp:40

The @return documentation doesn’t match the function behavior: addr_is_primary_impl() returns true when the address is not found in ConfigDB (falls through to return true;). Please update the comment to reflect the actual semantics so callers aren’t misled.

 * @return          true if the address is primary, false if secondary or not found

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 26, 2026 06:30

Copilot started reviewing on behalf of Xichen96 July 26, 2026 06:31 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 08:13

Copilot started reviewing on behalf of Xichen96 July 26, 2026 08:13 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Xichen96 changed the title ~~[dhcpmon] Bound raw packet callback work~~ [dhcpmon] Prevent packet bursts from starving event processing Jul 27, 2026

Xichen96 force-pushed the dev/xichenlin/bound-packet-callback-work branch from faa1813 to 7f6f687 Compare July 31, 2026 17:57

Xichen96 marked this pull request as ready for review July 31, 2026 17:58

Copilot AI review requested due to automatic review settings July 31, 2026 17:58

Copilot started reviewing on behalf of Xichen96 July 31, 2026 17:58 View session

Copilot AI previously approved these changes Jul 31, 2026

View reviewed changes

Xichen96 commented Jul 31, 2026

View reviewed changes

Xichen96 force-pushed the dev/xichenlin/bound-packet-callback-work branch from 7f6f687 to 1ee6d6a Compare July 31, 2026 18:16

Copilot AI review requested due to automatic review settings July 31, 2026 18:16

Copilot started reviewing on behalf of Xichen96 July 31, 2026 18:16 View session

Copilot AI dismissed their stale review, a newer Copilot review was requested July 31, 2026 18:18

Copilot AI approved these changes Jul 31, 2026

View reviewed changes

Xichen96 force-pushed the dev/xichenlin/bound-packet-callback-work branch from 1ee6d6a to 076df9a Compare July 31, 2026 19:20

Copilot AI review requested due to automatic review settings July 31, 2026 19:20

Xichen96 changed the title ~~[dhcpmon] Prevent packet bursts from starving event processing~~ [dhcpmon] Apply small packet and C++ hardening fixes Jul 31, 2026

This was referenced Jul 31, 2026

[dhcpmon] Use NULL for DB result pointers #106

Closed

[dhcpmon] Track physical members beneath VLAN PortChannels #99

Open

Copilot started reviewing on behalf of Xichen96 July 31, 2026 19:21 View session

Copilot AI reviewed Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

Xichen96 commented Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Work item tracking

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Back port request

Tested branch

Test result

Documentation

Review

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 26, 2026

Uh oh!

azure-pipelines Bot commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Ready to approve

Uh oh!

Xichen96 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Xichen96 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Xichen96 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Xichen96 Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Ready to approve

Uh oh!

mssonicbld commented Jul 31, 2026

Uh oh!

azure-pipelines Bot commented Jul 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Not ready to approve

Suppressed comments (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Xichen96 commented Jul 26, 2026 •

edited

Loading