More nh groups #1863
More nh groups #1863
Conversation
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
682c2c2 to
6e3ad5b
Compare
lib/.gitignore
Outdated
| @@ -25,3 +25,4 @@ grammar_sandbox | |||
| clippy | |||
| defun_lex.c | |||
| plist_clippy.c | |||
| nexthop_group_clippy.c | |||
There was a problem hiding this comment.
You can remove this, it's covered in the root gitignore in master now.
lib/nexthop_group.c
Outdated
| "If the nexthop is in a different vrf tell us\n" | ||
| "The nexthop-vrf Name\n") | ||
| { | ||
| struct nexthop_group_cmd *nhgc = VTY_GET_CONTEXT(nexthop_group_cmd); |
There was a problem hiding this comment.
It's better to use VTY_DECLVAR_CONTEXT to provide protection against someone removing the nexthop-group in another session.
| nhop.ifindex = ifname2ifindex(intf, vrf->vrf_id); | ||
| if (nhop.ifindex == IFINDEX_INTERNAL) { | ||
| vty_out(vty, | ||
| "Specified Intf %s does not exist in vrf: %s\n", |
There was a problem hiding this comment.
Wouldn't it be better if we allow the users to configure nexthops pointing to non-existing interfaces/VRFs? #1819 comes to mind.
There was a problem hiding this comment.
I agree that this needs to be done. Can I open an issue to track this and allow this commit to go in?
There was a problem hiding this comment.
Sure, I don't think this should block this PR.
lib/nexthop_group.c
Outdated
| break; | ||
| } | ||
|
|
||
| if (nh->vrf_id) { |
There was a problem hiding this comment.
maybe if (nh->vrf_id != VRF_DEFAULT) {
There was a problem hiding this comment.
I fixed this in the 3rd commit since I didn't want to have to edit the 1st and remake it, if you are looking for where I did thist
lib/nexthop_group.c
Outdated
| nhgc = nhgc_find(name); | ||
| if (!nhgc) { | ||
| nhgc = XCALLOC(MTYPE_TMP, sizeof(*nhgc)); | ||
| strcpy(nhgc->name, name); |
There was a problem hiding this comment.
this is dangerous, please use strlcpy() instead.
Add a nexthop-group cli: nexthop-group NAME nexthop A nexthop B nexthop C ! This will allow interested parties to hook into the cli for nexthops. Users can add callback functions for add/delete of a nexthop group as well as add/delete of each individual nexthop. Future work( PBR and static routes ) will take advantage of this. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Expose to the world the nhgc_find command so that interested parties can find a stored nexthop group. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Add code to allow nexthops to be written by people who are interested in writing their own nexthop line. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
6e3ad5b to
1b7bce0
Compare
There was a problem hiding this comment.
LGTM.
however, I would like to put in perspective flowspec.
In the nexthop-group, I would like to add some more items:
- mark a packet ( DSCP)
- add traffic control on a defined rate)
the modification does not need to be done now.
but I want to ask:
- is the nexthop group name ok to host something else than nexthop IPs or interfaces ?
this will permit to reach
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-2880/ This is a comment from an EXPERIMENTAL automated CI system. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
|
@rwestphal @pguibert6WIND is there anything else I need to do here? For this review? |
Some more nexthop_group work