search

FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.13k stars 1.21k forks source link

Router pim config #16269

Open nabahr opened 3 weeks ago

nabahr commented 3 weeks ago

Added new 'router pim[6] [vrf NAME]' config node.

Moved all existing global/vrf PIM configs to the new subnode and the existing configuration was set to be hidden and deprecated. Both versions of the configuration still work together for the time being until they can be safely removed.

PIM now always writes out configuration in the "router pim" syntax so ,in order to support frr-reload, the JSON based configuration in the PIM topotests also needs to generate configuration in the same syntax.

Config file style tests are left in the global pim configuration syntax for now as a test of backwards compatibility.

Fixes #16196

eqvinox commented 3 weeks ago

oh god that diff size T_T …

nabahr commented 3 weeks ago

oh god that diff size T_T …

Yeah. I tried different ways to ALIAS the commands but couldn't get it to work so I essentially had to duplicate all of the global PIM commands. Also all of the pim global xpath's have been touched, since now the current XPath is set and commands use relative paths. I am happy to hear any other ideas.

Jafaral commented 3 weeks ago

Documentation needs to be updated to reflect the new syntax.

nabahr commented 2 weeks ago

Verify source still fails for these 2 issues that won't be fixed

> < WARNING: Comparisons should place the constant on the right side of the test < #5618: FILE: /tmp/f1-589488/pim_cmd_common.c:5618:

This is a simple comparison of two defines, either order produces this warning.

> < WARNING: Macros with flow control statements should be avoided < #207: FILE: /tmp/f1-589488/pim_cmd_common.h:207:

This macro is meant to set up a few function variables and set the correct XPath for the deprecated CLI handlers. I can remove the macro if required but that'll require copying this duplicate code to all of the deprecated CLI handlers. As it is now I can't fix this warning.

Fixed these warnings now.

nabahr commented 1 day ago

Added a fix to frr-reload to detect and convert legacy pim configuration so that the tool works as expected. I reverted the changes to topotests so they are all back to using the legacy pim configuration.