Skip to content

lib: fix swapped values, bad setsockopt, and intermittent test failure#21214

Open
choppsv1 wants to merge 2 commits intoFRRouting:masterfrom
LabNConsulting:chopps/fix-swapped-vals-and-setsockopt
Open

lib: fix swapped values, bad setsockopt, and intermittent test failure#21214
choppsv1 wants to merge 2 commits intoFRRouting:masterfrom
LabNConsulting:chopps/fix-swapped-vals-and-setsockopt

Conversation

@choppsv1
Copy link
Contributor

@choppsv1 choppsv1 commented Mar 17, 2026

We had reversed the setting of max read and write message count values for mgmtd connections (both server and client). This effectively allowed clients to send more than they received and the server to process more than it sent (i.e., backwards of what we probably want to keep a loaded system working properly)

Also for the mgmtd loopback connection (i.e., sending CLI based YANG config or reciving oper-state) we were actually setting the recv and send kernel buf (setsockopt() which is given in bytes) to these number of message values. In this case the kernel was ignoring us and using a default value instead which was actually 128K. :)

Finally, fix an intermittent failure in mgmt_notify test that we (again) hit during CI testing of these changes.

@choppsv1 choppsv1 marked this pull request as draft March 17, 2026 18:13
@greptile-apps
Copy link

greptile-apps bot commented Mar 17, 2026

Greptile Summary

This PR fixes two distinct bugs in the mgmt message connection layer (lib/mgmt_msg.c):

  1. Swapped buffer limitsmgmt_msg_init was assigning max_write_buf to ms->max_read_buf and vice versa, meaning every caller had its read/write message-processing limits inverted. This is now correctly aligned.

  2. Wrong setsockopt argument typemsg_client_connect_short_circuit was passing mstate.max_write_buf / mstate.max_read_buf (which count messages, not bytes) to setsockopt_so_sendbuf / setsockopt_so_recvbuf, which expect a byte value. The kernel would have clamped these tiny values to its minimum. The fix replaces them with the dedicated MSG_CONN_SEND_BUF_SIZE / MSG_CONN_RECV_BUF_SIZE constants (now 128 KB each, up from 64 KB), matching what the regular client path (mgmt_msg_connect) and the server acceptance path (msg_conn_accept_init) already used.

Both fixes are clearly correct and the changes are minimal and surgical. No new logic is introduced.

Confidence Score: 5/5

  • This PR is safe to merge — all three changes are unambiguous bug fixes with no side-effects beyond correcting the broken behaviour.
  • The diff is small, isolated to one file, and each change directly corrects a clearly documented bug. The swapped field assignment is trivially verified against the parameter names. The setsockopt fix brings the short-circuit path in line with the already-correct regular client and server-accept paths. The buffer-size increase (64 KB → 128 KB) is conservative and the previous values were effectively meaningless to the kernel anyway.
  • No files require special attention.

Important Files Changed

Filename Overview
lib/mgmt_msg.c Three targeted bug fixes: (1) corrects swapped max_read_buf/max_write_buf assignments in mgmt_msg_init, (2) doubles kernel socket buffer constants from 64KB to 128KB, and (3) fixes the short-circuit connection setsockopt calls to use byte-size constants instead of message-count values.

Sequence Diagram

sequenceDiagram
    participant C as msg_client
    participant SC as short_circuit path
    participant S as msg_server

    Note over C,S: Short-circuit connection setup (fixed)
    C->>SC: msg_client_connect_short_circuit()
    SC->>SC: socketpair(AF_UNIX, SOCK_STREAM, 0, sockets)
    SC->>SC: set_nonblocking(sockets[0])
    SC->>SC: setsockopt_so_sendbuf(sockets[0], MSG_CONN_SEND_BUF_SIZE ✅)
    Note right of SC: Previously: mstate.max_write_buf (msg count, not bytes ❌)
    SC->>SC: setsockopt_so_recvbuf(sockets[0], MSG_CONN_RECV_BUF_SIZE ✅)
    Note right of SC: Previously: mstate.max_read_buf (msg count, not bytes ❌)
    SC->>S: server->create(sockets[1], &su) → msg_conn_accept_init()
    S->>S: setsockopt_so_sendbuf(fd, MSG_CONN_SEND_BUF_SIZE) [already correct]
    S->>S: setsockopt_so_recvbuf(fd, MSG_CONN_RECV_BUF_SIZE) [already correct]

    Note over C,S: mgmt_msg_init assignment (fixed)
    C->>C: mgmt_msg_init(ms, max_read_buf, max_write_buf, ...)
    C->>C: ms->max_read_buf = max_read_buf ✅
    Note right of C: Previously assigned max_write_buf here ❌
    C->>C: ms->max_write_buf = max_write_buf ✅
    Note right of C: Previously assigned max_read_buf here ❌
Loading

Last reviewed commit: "lib: fix swapped val..."

@choppsv1 choppsv1 force-pushed the chopps/fix-swapped-vals-and-setsockopt branch from 2b6c583 to 47f58aa Compare March 19, 2026 17:35
@github-actions github-actions bot added size/S and removed size/XS labels Mar 19, 2026
@choppsv1 choppsv1 marked this pull request as ready for review March 19, 2026 17:35
@github-actions github-actions bot added the rebase PR needs rebase label Mar 19, 2026
@choppsv1 choppsv1 force-pushed the chopps/fix-swapped-vals-and-setsockopt branch from 47f58aa to a27068a Compare March 21, 2026 19:42
@frrbot frrbot bot added tests Topotests, make check, etc zebra labels Mar 21, 2026
@github-actions github-actions bot added size/L and removed size/S labels Mar 21, 2026
@choppsv1 choppsv1 force-pushed the chopps/fix-swapped-vals-and-setsockopt branch from a27068a to 9471ad3 Compare March 21, 2026 20:38
@choppsv1 choppsv1 changed the title lib: fix swapped values and bad setsockopt lib: fix swapped values, bad setsockopt, and intermittent test failure Mar 21, 2026
We had reversed the setting of max read and write message count values
for mgmtd connections (both server and client). This effectively allowed
clients to send more than they received and the server to process more
than it sent (i.e., backwards of what we probably want to keep a loaded
system working properly)

Also for the mgmtd loopback connection (i.e., sending CLI based YANG
config or reciving oper-state) we were actually setting the recv and
send kernel buf (setsockopt() which is given in bytes) to these number
of message values (so less than 1K). In this case the kernel was
ignoring us and using a default value instead which was actually 128K.
Go ahead and use the same 128K value for our buffers as well.

Signed-off-by: Christian Hopps <[email protected]>
Read and wait on live output from the clients rather than running the
client to completion after it receives N notifications. This allows us
to wait for the notifications we expect and not be fragile to receiving
other notifications in between.

Add flush() to stdout of FE_CLIENT otherwise we wait on line output
from print() that never comes b/c it's buffered.

Signed-off-by: Christian Hopps <[email protected]>
@choppsv1 choppsv1 force-pushed the chopps/fix-swapped-vals-and-setsockopt branch from 9471ad3 to 7598309 Compare March 21, 2026 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix libfrr master rebase PR needs rebase size/L tests Topotests, make check, etc zebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant