-
Notifications
You must be signed in to change notification settings - Fork 27
[Dynamic Neighbor]: add patch to support dynamic neighbor configuration. #14
Conversation
@sihuihan88, |
peer_delete (peer); | ||
return -1; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same code 4 times. Probably it's better to introduce a static function here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is used to provide log info if debug_event is enabled. They are not exactly the same. So adding more detailed log info now for better troubleshooting.
bgpd/bgp_network.c
Outdated
@@ -220,6 +220,7 @@ bgp_accept (struct thread *thread) | |||
|
|||
peer1 = peer_lookup (NULL, &su); | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably it's better to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Have updated accordingly.
could you provide the patch info you reference? Add it to the commit message and also state what you have changed due to the patch conflicts. |
can you check this commit? FRRouting/frr@20eb886 |
and this one. FRRouting/frr@57e9ee0 |
and this one FRRouting/frr@2aab8d2 |
{ | ||
if (BGP_DEBUG (events, EVENTS)) | ||
{ | ||
zlog_debug("BGP stop: %s (dynamic neighbor) deleted", peer->host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> zlog_info
check line 467: neighbor down event is logged as info level, this is also neighbor down event, should also logged as info level.
next time, you need to do a fork and https://help.github.com/articles/fork-a-repo/ |
peer_delete (peer); | ||
return -1; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic can be merged into bgp_stop() as bgp_stop already has similar logic. We should enable code re-use.
|
||
peer_delete (peer); | ||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should move into bgp_stop()
|
||
peer_delete (peer); | ||
return -1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move into bgp_stop().
{ | ||
zlog_debug ("%s Dynamic Neighbor added, group %s count %d", | ||
peer->host, group->name, dncount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is neighbor add event, I think it should be info level, we should print it out when we enabled log neighbor change.
{ | ||
zlog_debug ("%s dropped from group %s, count %d", | ||
peer->host, peer->group->name, dncount); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should do something simliar here.
/* bgp log-neighbor-changes of neighbor Down */
if (bgp_flag_check (peer->bgp, BGP_FLAG_LOG_NEIGHBOR_CHANGES))
zlog_info ("%%ADJCHANGE: neighbor %s Down %s", peer->host,
peer_down_str [(int) peer->last_reset]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly logging information related
This patch bases on FRRouting/frr@f14e6fd, FRRouting/frr@20eb886, FRRouting/frr@57e9ee0, FRRouting/frr@2aab8d2
To make it work in our code base, the major additional changes are: