pimd,tests: refactor PIM join prune packet generation

[rzalamena]

The previous version of the code had three problems:

1. There was no upper limit on the amount of sources a group could have
   and when that number was too high it would cause a buffer overflow
2. When the first group had too many sources an empty group would be
   generated
3. When a group have too many sources there was no code to split them
   into multiple packets

The refactor addresses the mentioned problems by adding safe guards and
implementing a group filling function that supports restarting
iteration.

NOTES

• The removed function pim_msg_get_jp_group_size which did two
  things:

  • Counted the amount of bytes to be used by the whole group
  • Set the RPT prune to edge cases

  Is now replaced by pim_jp_groups_source_set_prune which only
  sets the RPT prune.

• The removed function pim_msg_build_jp_groups is now replaced
  by pim_jp_groups_fill which supports resume iteration over
  sources.

[greptile-apps]

Greptile Summary

This PR refactors PIM join/prune packet generation in pimd/pim_join.c to fix three bugs: unbounded source counts per group (buffer overflow), empty group emission on the first group overflow, and missing logic to split oversized groups across multiple packets. The old pim_msg_get_jp_group_size and pim_msg_build_jp_groups helpers are replaced by pim_jp_groups_fill (which supports resumable iteration over sources) and a new pim_jp_flush_packet helper. A topotest exercising 2000 IGMP static joins is added to validate the splitting behaviour.

• pim_jp_send replaces the duplicated bodies of pim_joinprune_send / pim_graft_send, using packet_max_size derived from the interface MTU and tracking last_source/last_child listnode cursors to resume group filling across packet boundaries.
• pim_jp_groups_fill accumulates sources one-by-one with explicit byte-budget accounting and returns the cursor state on overflow, preventing buffer overruns and empty groups.
• tgroups (the source-slot counter) is now size_t, eliminating the prior uint8_t overflow when a group held >255 sources under jumbo frames.

Confidence Score: 4/5

The refactor is logically sound and correctly addresses all three stated bugs; one guard in pim_jp_send conflates two size quantities in a way that would falsely reject very small MTUs, but this has no real-world impact.

The core packet-splitting logic — byte-budget tracking in pim_jp_groups_fill, resume via last_source/last_child cursors, and the goto-based retry loop — is correct. The MTU sanity check compares packet_max_size (already stripped of IP overhead) against a threshold that includes PIM_IP_HEADER_SIZE a second time, and sizeof(struct pim_jp) includes one full pim_jp_groups slot rather than just the JP message header, making the guard overly restrictive for unusual small MTUs. No data path affected by realistic deployments, but the mismatch warrants cleanup.

pimd/pim_join.c — specifically the packet_max_size sanity-check threshold on the newly added MTU guard.

Important Files Changed

Filename	Overview
pimd/pim_join.c	Core refactor: adds pim_jp_groups_fill/pim_jp_flush_packet, unifies joinprune/graft paths; MTU sanity check double-counts PIM_IP_HEADER_SIZE
pimd/pim_msg.c	Removes pim_msg_get_jp_group_size and pim_msg_build_jp_groups; remaining encoding helpers unchanged
pimd/pim_msg.h	Removes declarations for deleted helpers; no other changes
tests/topotests/pim_join_prune_packet_split/test_pim_join_prune_packet_split.py	New topotest with 2000 static IGMP joins to stress-test packet splitting; topology and assertions look correct
tests/topotests/pim_join_prune_packet_split/r1/frr.conf	r1 config: two interfaces with PIM/OSPF, r2 designated as RP
tests/topotests/pim_join_prune_packet_split/r2/frr.conf	r2 (RP) config: static route 10.0.0.0/16 via lo redistributed into OSPF to make test sources reachable

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pim_joinprune_send / pim_graft_send] --> B[pim_jp_send]
    B --> C{ifp MTU valid?}
    C -- No --> D[warn & return]
    C -- Yes --> E[compute packet_max_size]
    E --> F[for each group]
    F --> G[pim_jp_groups_source_set_prune\nset RPT prune flags]
    G --> H[pim_start_message]
    H --> I{packet_left le sizeof pim_jp?}
    I -- Yes --> J[init new packet header\npacket_size = PIM_JP_HEADER_SIZE]
    I -- No --> K[point grp to current offset]
    J --> L[pim_jp_groups_fill]
    K --> L
    L --> M{group_written == 0?}
    M -- Yes --> N[flush current packet\npacket_left=0, packet_size=0\ngoto pim_start_message]
    N --> H
    M -- No --> O[num_groups++\npacket_size += group_written\nhtons joins/prunes]
    O --> P{last_source or last_child?}
    P -- Yes --> Q[flush packet\npacket_left=0\ngoto pim_start_message]
    Q --> H
    P -- No --> R{packet too full or\nnum_groups == 255?}
    R -- Yes --> S[flush packet\npacket_left=0, packet_size=0\ncontinue]
    S --> F
    R -- No --> F
    F -- done --> T{packet_size > 0?}
    T -- Yes --> U[flush final packet]
    T -- No --> V[return 0]
    U --> V

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_join.c:955-958
`packet_max_size` already has `PIM_IP_HEADER_SIZE` deducted (line above), so adding it back in the RHS of this guard double-counts the IP overhead. Additionally `sizeof(struct pim_jp)` includes one complete `pim_jp_groups` slot (group header + one source) because `groups[1]` is not a C99 flexible array, so the threshold is more restrictive than intended. The net effect for IPv4 is that the guard rejects interfaces with MTU in the range [58, 95] even though a packet could actually be built; the `pim_jp_groups_fill` byte-budget accounting would catch any real overflow. The bound should reflect only what needs to fit in the available payload: the JP header plus the minimum group (header + one source).

```suggestion
	if (packet_max_size < (PIM_JP_HEADER_SIZE + sizeof(struct pim_jp_groups))) {
		zlog_warn("%s: interface %s MTU is too small: %d", __func__, ifp->name, ifp->mtu);
		return 0;
	}
```

_{Reviews (5): Last reviewed commit: "tests: PIM join prune test topology" | Re-trigger Greptile}

[rzalamena]

@greptileai

[rzalamena]

@greptileai

[Jafaral]

consider simplifying the config file by scripting the join?

[Jafaral]

@greptile review

[rzalamena]

@greptileai