🔴 Live TV: FRRouting/frr/pull/

donaldsharp · 2026-01-30T13:41:04Z

Allow the optional send of a table id for the installation/removal of routes.

greptile-apps · 2026-01-30T13:45:16Z

Greptile Overview

Greptile Summary

Added optional table ID parameter to sharpd route install/remove commands, allowing routes to be installed into specific routing tables. Added sharp install stop command to gracefully terminate repeating route operations.

Key changes:

Added table (0-4294967295) parameter to sharp install routes and sharp remove routes CLI commands
Added tableid and tableid_set fields to sharp_routes struct to track table ID state
Updated route_add() and route_delete() to set api.tableid and ZAPI_MESSAGE_TABLEID when table ID is specified
Implemented sharp_install_stop() to stop repeating install/remove loops and clean up installed routes
Added stop_loop flag to prevent handle_repeated() from continuing after stop is requested
Updated all route helper functions to propagate tableid and tableid_set parameters
Documented new features in user documentation

Issues from previous review threads:

Redundant validation checks at lines 365 and 833 in sharp_vty.c (CLI parser already validates range)
Missing handling for SHARP_DELETE_ROUTES_RESTART state in sharp_install_stop() at line 496

Confidence Score: 4/5

Safe to merge with minor style improvements recommended
The implementation correctly propagates table ID through all route operations and properly sets ZAPI message flags. The new sharp install stop command provides graceful shutdown. However, there are minor issues: redundant validation checks that can never fail, and a redundant flag set that the zclient library handles automatically. Previous review identified a missing state cleanup in sharp_install_stop() for the DELETE case.
sharpd/sharp_vty.c contains redundant validation checks; sharpd/sharp_zebra.c has incomplete state cleanup in sharp_install_stop()

Important Files Changed

Filename	Overview
sharpd/sharp_zebra.c	Adds `tableid` and `tableid_set` parameters throughout route add/remove functions, implements `sharp_install_stop()` to gracefully stop route operations
sharpd/sharp_vty.c	Adds table ID parameter to CLI commands and includes redundant validation checks that can never fail
sharpd/sharp_globals.h	Adds `tableid_set`, `tableid`, and `stop_loop` fields to `sharp_routes` struct

Sequence Diagram

sequenceDiagram
    participant User
    participant VTY as sharp_vty.c
    participant Helper as sharp_zebra.c
    participant Zebra as zebra daemon

    Note over User,Zebra: Install routes with table ID
    User->>VTY: sharp install routes ... table 100
    VTY->>VTY: Set sg.r.tableid=100, tableid_set=true
    VTY->>Helper: sharp_install_routes_helper(tableid, tableid_set)
    Helper->>Helper: sharp_install_routes_restart()
    loop For each route
        Helper->>Helper: route_add(tableid, tableid_set)
        Helper->>Helper: Set api.tableid, ZAPI_MESSAGE_TABLEID
        Helper->>Zebra: ZEBRA_ROUTE_ADD with table ID
        alt Buffer full
            Helper->>Helper: Store state in wb (including tableid)
            Zebra-->>Helper: Buffer ready callback
            Helper->>Helper: Resume from wb state
        end
    end
    Zebra-->>Helper: ZAPI_ROUTE_INSTALLED notifications
    
    Note over User,Zebra: Remove routes with table ID
    User->>VTY: sharp remove routes ... table 100
    VTY->>VTY: Set sg.r.tableid=100, tableid_set=true
    VTY->>Helper: sharp_remove_routes_helper(tableid, tableid_set)
    Helper->>Helper: sharp_remove_routes_restart()
    loop For each route
        Helper->>Helper: route_delete(tableid, tableid_set)
        Helper->>Helper: Set api.tableid, ZAPI_MESSAGE_TABLEID
        Helper->>Zebra: ZEBRA_ROUTE_DELETE with table ID
    end
    
    Note over User,Zebra: Stop operation
    User->>VTY: sharp install stop
    VTY->>Helper: sharp_install_stop()
    Helper->>Helper: Set stop_loop=true, clear INSTALL restart
    Helper->>Helper: sharp_remove_routes_helper() to cleanup

greptile-apps

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-30T13:45:19Z

+	if (tableid_set) {
+		api.tableid = tableid;
+		SET_FLAG(api.flags, ZEBRA_FLAG_TABLEID);


Inconsistent flag usage with route_add(). In route_add() at line 257, you use SET_FLAG(api.message, ZAPI_MESSAGE_TABLEID), but here you use SET_FLAG(api.flags, ZEBRA_FLAG_TABLEID). Both should use ZAPI_MESSAGE_TABLEID in api.message for consistency. The zclient library automatically sets the flag bit in api.flags when the message bit is set (see lib/zclient.c:1422-1423).

Suggested change

if (tableid_set) {

api.tableid = tableid;

SET_FLAG(api.flags, ZEBRA_FLAG_TABLEID);

if (tableid_set) {

api.tableid = tableid;

SET_FLAG(api.message, ZAPI_MESSAGE_TABLEID);

}

Prompt To Fix With AI

This is a comment left during a code review. Path: sharpd/sharp_zebra.c Line: 328:330 Comment: Inconsistent flag usage with `route_add()`. In `route_add()` at line 257, you use `SET_FLAG(api.message, ZAPI_MESSAGE_TABLEID)`, but here you use `SET_FLAG(api.flags, ZEBRA_FLAG_TABLEID)`. Both should use `ZAPI_MESSAGE_TABLEID` in `api.message` for consistency. The zclient library automatically sets the flag bit in `api.flags` when the message bit is set (see lib/zclient.c:1422-1423). ```suggestion if (tableid_set) { api.tableid = tableid; SET_FLAG(api.message, ZAPI_MESSAGE_TABLEID); } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-01-30T13:45:20Z

+	if (table_id_str) {
+		if (table_id > UINT32_MAX) {
+			vty_out(vty, "%% invalid input for table: %s\n", table_id_str);
+			return CMD_WARNING;
+		}
+		sg.r.tableid = (uint32_t)table_id;
+		sg.r.tableid_set = true;
+		SET_FLAG(sg.r.flags, ZEBRA_FLAG_TABLEID);
+	}


Redundant validation check. The CLI definition specifies [table (0-4294967295)$table_id] where 4294967295 equals UINT32_MAX, so the check at line 355 will never be true. The CLI parser already ensures table_id is within the valid range for uint32_t.

Suggested change

if (table_id_str) {

if (table_id > UINT32_MAX) {

vty_out(vty, "%% invalid input for table: %s\n", table_id_str);

return CMD_WARNING;

}

sg.r.tableid = (uint32_t)table_id;

sg.r.tableid_set = true;

SET_FLAG(sg.r.flags, ZEBRA_FLAG_TABLEID);

}

if (table_id_str) {

sg.r.tableid = (uint32_t)table_id;

sg.r.tableid_set = true;

SET_FLAG(sg.r.flags, ZEBRA_FLAG_TABLEID);

}

Prompt To Fix With AI

This is a comment left during a code review. Path: sharpd/sharp_vty.c Line: 354:362 Comment: Redundant validation check. The CLI definition specifies `[table (0-4294967295)$table_id]` where 4294967295 equals UINT32_MAX, so the check at line 355 will never be true. The CLI parser already ensures `table_id` is within the valid range for uint32_t. ```suggestion if (table_id_str) { sg.r.tableid = (uint32_t)table_id; sg.r.tableid_set = true; SET_FLAG(sg.r.flags, ZEBRA_FLAG_TABLEID); } ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-01-30T13:45:21Z

+	if (table_id_str) {
+		if (table_id > UINT32_MAX) {
+			vty_out(vty, "%% invalid input for table: %s\n", table_id_str);
+			return CMD_WARNING;
+		}
+		sg.r.tableid = (uint32_t)table_id;
+		sg.r.tableid_set = true;
+	}


Same redundant validation as in install_routes_cmd. The CLI definition limits the range to UINT32_MAX, making this check unnecessary.

Suggested change

if (table_id_str) {

if (table_id > UINT32_MAX) {

vty_out(vty, "%% invalid input for table: %s\n", table_id_str);

return CMD_WARNING;

}

sg.r.tableid = (uint32_t)table_id;

sg.r.tableid_set = true;

}

if (table_id_str) {

sg.r.tableid = (uint32_t)table_id;

sg.r.tableid_set = true;

}

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review. Path: sharpd/sharp_vty.c Line: 818:825 Comment: Same redundant validation as in `install_routes_cmd`. The CLI definition limits the range to UINT32_MAX, making this check unnecessary. ```suggestion if (table_id_str) { sg.r.tableid = (uint32_t)table_id; sg.r.tableid_set = true; } ``` <sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub> How can I resolve this? If you propose a fix, please make it concise.

donaldsharp · 2026-01-30T14:16:48Z

@greptile review

greptile-apps

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-30T14:20:41Z

+	if (wb.restart == SHARP_INSTALL_ROUTES_RESTART)
+		wb.restart = SHARP_RESTART_NONE;


Missing handling for SHARP_DELETE_ROUTES_RESTART. If a route removal operation is in progress when sharp install stop is called, the wb.restart state won't be cleared, causing the buffer ready callback to continue removing routes.

Suggested change

if (wb.restart == SHARP_INSTALL_ROUTES_RESTART)

wb.restart = SHARP_RESTART_NONE;

if (wb.restart == SHARP_INSTALL_ROUTES_RESTART || wb.restart == SHARP_DELETE_ROUTES_RESTART)

wb.restart = SHARP_RESTART_NONE;

Prompt To Fix With AI

This is a comment left during a code review. Path: sharpd/sharp_zebra.c Line: 496:497 Comment: Missing handling for `SHARP_DELETE_ROUTES_RESTART`. If a route removal operation is in progress when `sharp install stop` is called, the `wb.restart` state won't be cleared, causing the buffer ready callback to continue removing routes. ```suggestion if (wb.restart == SHARP_INSTALL_ROUTES_RESTART || wb.restart == SHARP_DELETE_ROUTES_RESTART) wb.restart = SHARP_RESTART_NONE; ``` How can I resolve this? If you propose a fix, please make it concise.

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-30T14:24:20Z

+		}
+		sg.r.tableid = (uint32_t)table_id;
+		sg.r.tableid_set = true;
+		SET_FLAG(sg.r.flags, ZEBRA_FLAG_TABLEID);


Redundant flag set. The zclient library automatically sets ZEBRA_FLAG_TABLEID in api.flags when ZAPI_MESSAGE_TABLEID is set in api.message (see lib/zclient.c:1422-1423). This line can be removed.

Prompt To Fix With AI

This is a comment left during a code review. Path: sharpd/sharp_vty.c Line: 372:372 Comment: Redundant flag set. The zclient library automatically sets `ZEBRA_FLAG_TABLEID` in `api.flags` when `ZAPI_MESSAGE_TABLEID` is set in `api.message` (see lib/zclient.c:1422-1423). This line can be removed. How can I resolve this? If you propose a fix, please make it concise.

Seems this is right what greptile is saying?

ton31337 · 2026-01-31T16:10:47Z

+	sg.r.tableid = 0;
+	sg.r.tableid_set = false;
+	if (table_id_str) {
+		if (table_id > UINT32_MAX) {


I think that's not possible, right?

yeah removed

Wanted the ability to push routes into a random table for some testing that I want to do. Just add the ability to allow this. Signed-off-by: Donald Sharp <sharpd@nvidia.com>

If you have a large number of routes being repeatedly installed/removed, add a `sharp install stop` command. This allows for the stopping of the loop. Signed-off-by: Donald Sharp <sharpd@nvidia.com>

Signed-off-by: Donald Sharp <sharpd@nvidia.com>

riw777

looks good

frrbot Bot added documentation sharp labels Jan 30, 2026

github-actions Bot added master size/L labels Jan 30, 2026

greptile-apps Bot reviewed Jan 30, 2026

View reviewed changes

donaldsharp force-pushed the sharp_send_tableid_for_route branch from f8e94d7 to c06f2ea Compare January 30, 2026 14:05

greptile-apps Bot reviewed Jan 30, 2026

View reviewed changes

ton31337 reviewed Jan 31, 2026

View reviewed changes

donaldsharp mentioned this pull request Feb 1, 2026

mgmt_notif.test_ds_notif failure on my machine #20648

Closed

2 tasks

donaldsharp added 3 commits February 10, 2026 09:13

sharpd: Add table_id to installation/removal of routes.

44d7262

Wanted the ability to push routes into a random table for some testing that I want to do. Just add the ability to allow this. Signed-off-by: Donald Sharp <sharpd@nvidia.com>

sharpd: Add a sharp install stop to stop repeated running

59454b4

If you have a large number of routes being repeatedly installed/removed, add a `sharp install stop` command. This allows for the stopping of the loop. Signed-off-by: Donald Sharp <sharpd@nvidia.com>

doc: Update sharp documentation for this change to installation/removal

43c4fee

Signed-off-by: Donald Sharp <sharpd@nvidia.com>

donaldsharp force-pushed the sharp_send_tableid_for_route branch from c06f2ea to 43c4fee Compare February 10, 2026 14:17

riw777 approved these changes Feb 10, 2026

View reviewed changes

ton31337 merged commit 2b7160c into FRRouting:master Feb 11, 2026
19 checks passed

donaldsharp deleted the sharp_send_tableid_for_route branch April 30, 2026 10:51

		if (wb.restart == SHARP_INSTALL_ROUTES_RESTART)
		wb.restart = SHARP_RESTART_NONE;

Uh oh!

Conversation

donaldsharp commented Jan 30, 2026

Uh oh!

greptile-apps Bot commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

donaldsharp commented Jan 30, 2026

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

donaldsharp Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

ton31337 Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

donaldsharp Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

riw777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Jan 30, 2026 •

edited

Loading