pimd: BSR/C-RP fixes with expanded topotest coverage

[Jafaral]

see individual commit msgs.

[greptile-apps]

Greptile Summary

This PR fixes several BSR/C-RP bugs in pimd and adds topotest coverage for BSR priority changes and IPv6 candidate-BSR JSON output. The fixes address an inverted error-counter condition, missing stats tracking, a wrong sizeof type, a FIXME in priority-only BSR updates, and duplicate header declarations.

• pim_pim.c: Inverts the pim_sock_read_helper return-value check so pim_ifstat_hello_recvfail is incremented on error (!= 0) rather than on success (== 0), correcting a counter that was always wrong at runtime.
• pim_bsm.c: Guards unicast_sock creation failure with an early return and a warning log; moves pim_crp_db_clear to the top of pim_bsm_proc_free; adds a >= 0 check before close and resets the fd to -1.
• pim_bsr_rpdb.c: Uncomments BSM receive/drop stat counters and fixes the bounds check to use sizeof(pim_encoded_group) instead of the IPv4-specific struct, which matters for IPv6 correctness.

Confidence Score: 4/5

Safe to merge; the changes are targeted bug fixes with no new complexity introduced, though the hello receive-failure counter inversion has been silently misbehaving in all existing deployments.

The pim_ifstat_hello_recvfail counter in pim_pim.c was incremented on every successful packet read and never on actual errors, meaning operator dashboards that rely on this counter have been showing incorrect data. While it does not affect routing correctness, it represents a real present defect in the changed code path that is now fixed.

pimd/pim_pim.c contains the most impactful single-line fix; pimd/pim_bsm.c deserves a second look to confirm the partial-init / early-return path is fully safe under all VRF teardown orderings.

Important Files Changed

Filename	Overview
pimd/pim_pim.c	Single-line inversion fix: pim_ifstat_hello_recvfail was incremented on success instead of failure; corrected to != 0.
pimd/pim_bsm.c	Added guard for unicast_sock creation failure with early return; moved pim_crp_db_clear before socket close in free path; added >= 0 guard and reset to -1 on close.
pimd/pim_bsr_rpdb.c	Uncommented BSM stats counters; fixed sizeof type from struct pim_encoded_group_ipv4 to pim_encoded_group for correct IPv4/IPv6 handling.
pimd/pim_nb_config.c	Replaced candidate_bsr_addrsel call (which had a FIXME) with pim_cand_bsr_apply for correct priority-only BSR updates.
pimd/pim_cmd_common.c	Replaced inet_ntop + json_object_string_add with json_object_string_addf using %pPA format for cleaner BSR address serialization.
pimd/pim_bsm.h	Removed two duplicate DECLARE_MTYPE(PIM_BSM_FRAG) declarations that would have caused compile errors or warnings.
tests/topotests/pim_cand_rp_bsr/test_pim_cand_rp_bsr.py	Added test_pim_bsr_priority_modify and test_pimv6_bsr_cand_bsr_r2 tests; updated test_pim_bsr_election_fallback_r2 to reflect priority 250 set by the new earlier test.

Sequence Diagram

sequenceDiagram
    participant VRF as VRF init/teardown
    participant INIT as pim_bsm_proc_init
    participant SOCK as pim_socket_raw
    participant FREE as pim_bsm_proc_free
    participant CRPDB as pim_crp_db_clear

    VRF->>INIT: pim_bsm_proc_init(pim)
    INIT->>INIT: "memset scope (unicast_sock=0)"
    INIT->>SOCK: pim_socket_raw(IPPROTO_PIM)
    alt socket failure
        SOCK-->>INIT: "fd < 0"
        INIT->>INIT: zlog_warn + return (NEW)
    else socket success
        SOCK-->>INIT: valid fd
        INIT->>INIT: set_nonblocking, setsockopt, event_add_read
    end

    VRF->>FREE: pim_bsm_proc_free(pim)
    FREE->>CRPDB: pim_crp_db_clear(scope) (MOVED HERE)
    FREE->>FREE: event_cancel(unicast_read)
    alt "unicast_sock >= 0 (NEW guard)"
        FREE->>FREE: close(unicast_sock)
    end
    FREE->>FREE: "scope->unicast_sock = -1 (NEW)"
    FREE->>FREE: pim_bs_timer_stop, pim_bsm_frags_free, route_table_finish

    note over SOCK,FREE: pim_pim.c: pim_sock_read_helper != 0 => recvfail++ (FIXED)

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
pimd/pim_pim.c:531-532
**Inverted failure-counter condition (pre-existing bug fixed here)**

The original `== 0` check incremented `pim_ifstat_hello_recvfail` whenever `pim_sock_read_helper` returned *success* (0), so every successfully received PIM Hello was counted as a receive failure, and every actual socket error was silently ignored in the counter. The new `!= 0` check is correct: it increments the stat only when the helper returns a non-zero error value.

_{Reviews (1): Last reviewed commit: "tests: expand PIM candidate BSR/RP topot..." | Re-trigger Greptile}

[greptile-apps]

Inverted failure-counter condition (pre-existing bug fixed here)

The original == 0 check incremented pim_ifstat_hello_recvfail whenever pim_sock_read_helper returned success (0), so every successfully received PIM Hello was counted as a receive failure, and every actual socket error was silently ignored in the counter. The new != 0 check is correct: it increments the stat only when the helper returns a non-zero error value.

Prompt To Fix With AI

This is a comment left during a code review.
Path: pimd/pim_pim.c
Line: 531-532

Comment:
**Inverted failure-counter condition (pre-existing bug fixed here)**

The original `== 0` check incremented `pim_ifstat_hello_recvfail` whenever `pim_sock_read_helper` returned *success* (0), so every successfully received PIM Hello was counted as a receive failure, and every actual socket error was silently ignored in the counter. The new `!= 0` check is correct: it increments the stat only when the helper returns a non-zero error value.

How can I resolve this? If you propose a fix, please make it concise.

[Jafaral]

@greptile, I fixed the bug, what else you want here?

[greptile-apps]

Nothing else needed — my comment was just explaining the fix, not requesting changes. The != 0 correction is right and the note was meant as confirmation. All good here!

[Jafaral]

@greptile, it is s bit confusing when you mark it as p1 and end it with "Prompt To Fix With AI", like you do with real bugs you flag.

[greptile-apps]

Fair point — sorry for the confusion. Since this was already fixed in the PR, I should have framed it as a confirmation note rather than a flagged issue with a severity label. Explanatory comments on already-corrected code shouldn't carry a P1 label or an AI fix prompt.

[Jafaral]

Nothing else to do on this PR. Nothing to add to greptile, code formatting is ignored to keep the changed lines consistent with surrounding text.

[Jafaral]

@Mergifyio backport stable/10.6 stable/10.5

[mergify]

backport stable/10.6 stable/10.5

✅ Backports have been created

Details

• #22180 pimd: BSR/C-RP fixes with expanded topotest coverage (backport #22117) has been created for branch stable/10.6
• #22181 pimd: BSR/C-RP fixes with expanded topotest coverage (backport #22117) has been created for branch stable/10.5