Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zebra rework factor #15473

Closed

Conversation

pguibert6WIND
Copy link
Member

This is a list of change in zebra that factorize the code.
There is no change in the behavior.

Reduce the code size by reusing the same code to display
nexthops and backup nexthops.

Signed-off-by: Philippe Guibert <[email protected]>
…nction

Move the rib_evpn_route_add() and rib_evpn_route_delete() functions
in a separate place.

Signed-off-by: Philippe Guibert <[email protected]>
Factorise the nexthop display handling in a separate function.

Signed-off-by: Philippe Guibert <[email protected]>
Create a separate function to handle separately the nexthop structure.
On the next commit, when a nexthop group is used, the nexthop of
each nexthop group dependency is used.

Signed-off-by: Philippe Guibert <[email protected]>
The show_route_nexthop_group_helper() and the
show_nexthop_group_json_helper() helper functions are introduced
to separate nexthop handling from the remaining 'show ip route'
command.

Signed-off-by: Philippe Guibert <[email protected]>
Create a second function zsend_redistribute_route_nhg() to
handle nexthop group.

Signed-off-by: Philippe Guibert <[email protected]>
Create a mpls_ftn_uninstall_nhg() function that handles the
nexthop parsing for each route entry. This parsing is common
to primary and backup nexthops.

Signed-off-by: Philippe Guibert <[email protected]>
The zebra_nexthop_vty_helper() and zebra_nexthop_json_helper()
functions could be very helpful to display nexthop information
from whatever daemon.

Move the core function in the nexthop_vty_helper() and the
nexthop_json_helper() function. The zebra API call remains
unchanged.

Signed-off-by: Philippe Guibert <[email protected]>
@frrbot frrbot bot added the libfrr label Mar 4, 2024
@github-actions github-actions bot added size/XXL and removed size/XL labels Mar 4, 2024
@@ -1158,3 +1158,331 @@ bool nexthop_is_ifindex_type(const struct nexthop *nh)
return true;
return false;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the reason I've never moved to having a vty function and a json function is that you end up with code that is in two functions and an addition to the data structure being dumped is easy to forget the json aspect of it ( or vice versa ). It always sucked but I felt the in your face aspect was warranted. What advantage are you looking for with this change? How do we ensure that a new developer is going to remember to hit both sides of the equation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to factor some changes for the BGP NHG changes. but it seems I no longer need to support it.
So I try to upstream it.

I admit most of the code change is not justified, today.

What advantage are you looking for with this change?

  • I could see that nexthop display is not up to date everywhere.
    A little more work could help in having a single function.

How do we ensure that a new developer is going to remember to hit both sides of the equation?

A single function could help, to handle both json and vty.

@pguibert6WIND pguibert6WIND marked this pull request as draft March 5, 2024 12:58
@pguibert6WIND
Copy link
Member Author

closing for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants