🔴 Live TV: FRRouting/frr/pull/

pguibert6WIND · 2026-04-17T10:25:59Z

See individual commits.

greptile-apps · 2026-04-17T10:40:28Z

Greptile Summary

This PR contains three targeted BFD daemon fixes: (1) aligns the VTY text output of _display_peer_counter with the JSON output by only showing the "Tx fail packet" counter for SBFD sessions; (2) adds the missing "Rx fail packet" line to the user documentation examples; and (3) fixes profile deletion logic so that removing a profile definition no longer also wipes the profile_name reference from sessions that use it — preserving the session–profile association so the profile can be re-applied when recreated.

Confidence Score: 5/5

Safe to merge; all three fixes are correct and well-scoped

The only finding is a P2 style suggestion (unnecessary heap allocation for a stack-suitable struct). All logic is correct: the profile fix properly distinguishes between suppressing the profile reference only vs. the full profile name; the VTY counter guard matches the existing JSON path; and the doc examples match the actual output.

bfdd/bfd.c — minor style note on the temporary XCALLOC pattern in bfd_profile_detach

Important Files Changed

Filename	Overview
bfdd/bfd.c	Adds `suppress_profile_from_session_config` parameter to `bfd_profile_free`/`bfd_profile_detach` and a new heap-allocated `bfd_profile_detach_info` struct; heap allocation is unnecessary since the struct is entirely stack-local to the function
bfdd/bfd.h	Declaration of `bfd_profile_free` updated to include the new `suppress_profile_from_session_config` bool parameter; change is consistent with the implementation
bfdd/bfdd_nb_config.c	Profile destroy callback correctly passes `false` to preserve `profile_name` on sessions when only the profile definition is removed
bfdd/bfdd_vty.c	Tx fail packet counter in VTY text output now correctly guarded to SBFD sessions only, matching the existing JSON path
doc/user/bfd.rst	Adds "Rx fail packet: 0" to six documentation examples for `show bfd peers counters` and `clear bfd peers counters`; matches the unconditional VTY output line

Sequence Diagram

sequenceDiagram
    participant CLI as NB CLI
    participant NB as bfdd_nb_config
    participant BFD as bfd.c
    participant Hash as bfd_key_hash

    Note over CLI,Hash: Profile removal (fix: preserve profile_name on sessions)
    CLI->>NB: bfdd_bfd_profile_destroy()
    NB->>BFD: bfd_profile_free(bp, suppress=false)
    BFD->>BFD: bfd_profile_detach(bp, suppress=false)
    BFD->>Hash: hash_iterate(_bfd_profile_detach, info)
    Hash->>BFD: _bfd_profile_detach(session)
    BFD->>BFD: bfd_profile_remove_reference(bs) bs->profile = NULL (keeps profile_name)
    BFD->>BFD: bfd_session_apply(bs) uses peer_profile fallback

    Note over CLI,Hash: Full config wipe (suppress=true removes profile_name too)
    CLI->>NB: bfdd_bfd_destroy()
    NB->>BFD: bfd_profiles_remove()
    BFD->>BFD: bfd_profile_free(bp, suppress=true)
    BFD->>BFD: bfd_profile_detach(bp, suppress=true)
    Hash->>BFD: _bfd_profile_detach(session)
    BFD->>BFD: bfd_profile_remove(bs) XFREE(profile_name) + clears profile

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bfdd/bfd.c
Line: 2491-2500

Comment:
**Unnecessary heap allocation for a function-local struct**

`bfd_profile_detach_info` is allocated with `XCALLOC`, passed to `hash_iterate` (which is synchronous), and immediately freed — it never escapes the function. Allocating it on the stack is simpler, avoids an extra mtype definition (`BFDD_PROFILE_DETACH_INFO`), and carries no risk.

```suggestion
static void bfd_profile_detach(struct bfd_profile *bp, bool suppress_profile_from_session_config)
{
	struct bfd_profile_detach_info prof_info = {
		.bp = bp,
		.suppress_config = suppress_profile_from_session_config,
	};

	hash_iterate(bfd_key_hash, _bfd_profile_detach, &prof_info);
}
```

You can then remove the `DEFINE_MTYPE_STATIC(BFDD, BFDD_PROFILE_DETACH_INFO, ...)` line as well.

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

_{Reviews (1): Last reviewed commit: "bfdd: fix do not unconfig bfd profile se..." | Re-trigger Greptile}

greptile-apps · 2026-04-17T10:40:32Z

+static void bfd_profile_detach(struct bfd_profile *bp, bool suppress_profile_from_session_config)
 {
-	hash_iterate(bfd_key_hash, _bfd_profile_detach, bp);
+	struct bfd_profile_detach_info *prof_info;
+
+	prof_info = XCALLOC(MTYPE_BFDD_PROFILE_DETACH_INFO, sizeof(struct bfd_profile_detach_info));
+	prof_info->bp = bp;
+	prof_info->suppress_config = suppress_profile_from_session_config;
+	hash_iterate(bfd_key_hash, _bfd_profile_detach, prof_info);
+	XFREE(MTYPE_BFDD_PROFILE_DETACH_INFO, prof_info);
 }


Unnecessary heap allocation for a function-local struct

bfd_profile_detach_info is allocated with XCALLOC, passed to hash_iterate (which is synchronous), and immediately freed — it never escapes the function. Allocating it on the stack is simpler, avoids an extra mtype definition (BFDD_PROFILE_DETACH_INFO), and carries no risk.

Suggested change

static void bfd_profile_detach(struct bfd_profile *bp, bool suppress_profile_from_session_config)

{

hash_iterate(bfd_key_hash, _bfd_profile_detach, bp);

struct bfd_profile_detach_info *prof_info;

prof_info = XCALLOC(MTYPE_BFDD_PROFILE_DETACH_INFO, sizeof(struct bfd_profile_detach_info));

prof_info->bp = bp;

prof_info->suppress_config = suppress_profile_from_session_config;

hash_iterate(bfd_key_hash, _bfd_profile_detach, prof_info);

XFREE(MTYPE_BFDD_PROFILE_DETACH_INFO, prof_info);

}

static void bfd_profile_detach(struct bfd_profile *bp, bool suppress_profile_from_session_config)

{

struct bfd_profile_detach_info prof_info = {

.bp = bp,

.suppress_config = suppress_profile_from_session_config,

};

hash_iterate(bfd_key_hash, _bfd_profile_detach, &prof_info);

}

You can then remove the DEFINE_MTYPE_STATIC(BFDD, BFDD_PROFILE_DETACH_INFO, ...) line as well.

Prompt To Fix With AI

This is a comment left during a code review. Path: bfdd/bfd.c Line: 2491-2500 Comment: **Unnecessary heap allocation for a function-local struct** `bfd_profile_detach_info` is allocated with `XCALLOC`, passed to `hash_iterate` (which is synchronous), and immediately freed — it never escapes the function. Allocating it on the stack is simpler, avoids an extra mtype definition (`BFDD_PROFILE_DETACH_INFO`), and carries no risk. ```suggestion static void bfd_profile_detach(struct bfd_profile *bp, bool suppress_profile_from_session_config) { struct bfd_profile_detach_info prof_info = { .bp = bp, .suppress_config = suppress_profile_from_session_config, }; hash_iterate(bfd_key_hash, _bfd_profile_detach, &prof_info); } ``` You can then remove the `DEFINE_MTYPE_STATIC(BFDD, BFDD_PROFILE_DETACH_INFO, ...)` line as well. How can I resolve this? If you propose a fix, please make it concise.

rzalamena

Looks good, just two observations.

Update: your second commit is missing the Signed-off-by line.

rzalamena · 2026-04-17T18:56:59Z

-	hash_iterate(bfd_key_hash, _bfd_profile_detach, bp);
+	struct bfd_profile_detach_info *prof_info;
+
+	prof_info = XCALLOC(MTYPE_BFDD_PROFILE_DETACH_INFO, sizeof(struct bfd_profile_detach_info));


I agreed with greptile this allocation seems unnecessary, the variable could just be put in the stack.

rzalamena · 2026-04-17T18:58:00Z

 }

-void bfd_profile_free(struct bfd_profile *bp)
+void bfd_profile_free(struct bfd_profile *bp, bool suppress_profile_from_session_config)


suppress_profile_from_session_config seems unnecessary long variable name, please can we make it shorter? suppress_profile maybe?

The json show does not display such counters. Do the same for the non json show. Fixes: 868c4d0 ("bfdd: adjust show commands for SBFD") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

BFD doc needs to be updated. Fixes: 9ae600c ("bfdd, yang: Add a bad packet counter for bfd peers") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

If a profile is used by a BFD session, then removing the profile from the configuration will also remove the configuration of that profile in that BFD session. This should not happen, as from config perspective, the profile config and the profile attached to the bfd session are two distinct elements. Fixes: f6dfa24 ("bfdd: remove profile pointers on removal") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

riw777

looks good

riw777 · 2026-04-21T15:40:19Z

Waiting on @rzalamena 's comments

frrbot Bot added bfd bugfix documentation labels Apr 17, 2026

github-actions Bot added size/M master labels Apr 17, 2026

greptile-apps Bot reviewed Apr 17, 2026

View reviewed changes

rzalamena approved these changes Apr 17, 2026

View reviewed changes

pguibert6WIND added 3 commits April 21, 2026 10:43

bfdd: fix do not display tx fail counter for non sbfd sessions

e5caa73

The json show does not display such counters. Do the same for the non json show. Fixes: 868c4d0 ("bfdd: adjust show commands for SBFD") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

doc: add missing bfd statistics for rx failed packet

e5936d6

BFD doc needs to be updated. Fixes: 9ae600c ("bfdd, yang: Add a bad packet counter for bfd peers") Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>

pguibert6WIND force-pushed the bfd_misc_fixes branch from 0556952 to c21eb14 Compare April 21, 2026 08:48

riw777 approved these changes Apr 21, 2026

View reviewed changes

rzalamena merged commit a6ff023 into FRRouting:master Apr 23, 2026
23 checks passed

Navigation Menu

Uh oh!

BFD miscellaneous fixes - #21613

BFD miscellaneous fixes#21613
rzalamena merged 3 commits into
FRRouting:masterfrom
pguibert6WIND:bfd_misc_fixes

pguibert6WIND commented Apr 17, 2026

Uh oh!

greptile-apps Bot commented Apr 17, 2026

Uh oh!

greptile-apps Bot Apr 17, 2026

Uh oh!

pguibert6WIND Apr 22, 2026

Uh oh!

rzalamena left a comment •

edited

Loading

Uh oh!

rzalamena Apr 17, 2026

Uh oh!

pguibert6WIND Apr 22, 2026

Uh oh!

rzalamena Apr 17, 2026

Uh oh!

pguibert6WIND Apr 22, 2026

Uh oh!

riw777 left a comment

Uh oh!

riw777 commented Apr 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

pguibert6WIND commented Apr 17, 2026

Uh oh!

greptile-apps Bot commented Apr 17, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

pguibert6WIND Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

rzalamena left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rzalamena Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

pguibert6WIND Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

rzalamena Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

pguibert6WIND Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

riw777 left a comment

Choose a reason for hiding this comment

Uh oh!

riw777 commented Apr 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rzalamena left a comment •

edited

Loading