Hi,
See below.
On Fri, 11 Dec 2015, Donald Sharp wrote:
From: Daniel Walton <[email protected]>
Summary of changes
- added an option to enable keepalive debugs for a specific peer
- added an option to enable inbound and/or outbound updates debugs for a
specific peer
- added an option to enable update debugs for a specific prefix
- added an option to enable zebra debugs for a specific prefix
- combined "deb bgp", "deb bgp events" and "deb bgp fsm" into "deb bgp neighbor-events".
"deb bgp neighbor-events" can be enabled for a specific peer.
- merged "deb bgp filters" into "deb bgp update"
- moved the per-peer logging to one central log file. We now have the ability
to filter all verbose debugs on a per-peer and per-prefix basis so we no longer
need to keep log files per-peer. This simplifies troubleshooting by keeping all
BGP logs in one location. The use
r can then grep for the peer IP they are interested in if they wish to see the
logs for a specific peer.
- Changed "show debugging" in isis to "show debugging isis" to be consistent with all
other protocols. This was very confusing for the user because they would type "show debug" and
expect to see a list of debugs enabled across all protocols.
- Removed "undebug" from the parser for BGP. Again this was to be consisten
with all other protocols.
- Removed the "all" keyword from the BGP debug parser. The user can now do "no debug
bgp" to disable all BGP debugs, before you had to type "no deb all bgp" which was confusing.
The new parse tree for BGP debugging is:
deb bgp as4
deb bgp as4 segment
deb bgp keepalives [A.B.C.D|WORD|X:X::X:X]
deb bgp neighbor-events [A.B.C.D|WORD|X:X::X:X]
deb bgp nht
deb bgp updates [in|out] [A.B.C.D|WORD|X:X::X:X]
deb bgp updates prefix [A.B.C.D/M|X:X::X:X/M]
deb bgp zebra
deb bgp zebra prefix [A.B.C.D/M|X:X::X:X/M]
Signed-off-by: Daniel Walton <[email protected]>
---
bgpd/bgp_advertise.c | 4 +-
bgpd/bgp_attr.c | 146 +++---
bgpd/bgp_debug.c | 1108 +++++++++++++++++++++++++++++++++----------
bgpd/bgp_debug.h | 36 +-
bgpd/bgp_fsm.c | 86 ++--
bgpd/bgp_main.c | 5 +-
bgpd/bgp_mpath.c | 2 +-
bgpd/bgp_network.c | 33 +-
bgpd/bgp_open.c | 24 +-
bgpd/bgp_packet.c | 307 ++++++------
bgpd/bgp_route.c | 288 +++++------
bgpd/bgp_routemap.c | 36 +-
bgpd/bgp_vty.c | 2 +-
bgpd/bgp_zebra.c | 42 +-
bgpd/bgpd.c | 12 +-
bgpd/bgpd.h | 9 +-
isisd/isisd.c | 5 +-
lib/log.c | 22 -
lib/log.h | 12 -
lib/memtypes.c | 2 +
tests/bgp_capability_test.c | 8 +-
tests/bgp_mp_attr_test.c | 8 +-
22 files changed, 1348 insertions(+), 849 deletions(-)
diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c
index ea1e283..0c15622 100644
--- a/bgpd/bgp_advertise.c
+++ b/bgpd/bgp_advertise.c
@@ -282,7 +282,7 @@ bgp_adj_out_set (struct bgp_node *rn, struct peer *peer,
struct prefix *p,
{
if (!peer->radv_adjusted)
{
- if (BGP_DEBUG (events, EVENTS))
+ if (bgp_debug_update(peer, NULL, 0))
+/* Return true if this prefix is on the per_prefix_list of prefixes to debug
+ * for BGP_DEBUG_TYPE
+ */
+static int
+bgp_debug_per_prefix (struct prefix *p, unsigned long term_bgp_debug_type,
+ unsigned int BGP_DEBUG_TYPE, struct list
*per_prefix_list)
+{
+ struct bgp_debug_filter *filter;
+ struct listnode *node, *nnode;
+
+ if (term_bgp_debug_type & BGP_DEBUG_TYPE)
+ {
+ /* We are debugging all prefixes so return true */
+ if (!per_prefix_list || list_isempty(per_prefix_list))
+ return 1;
+
+ else
+ {
+ if (!p)
+ return 0;
+
+ for (ALL_LIST_ELEMENTS (per_prefix_list, node, nnode, filter))
+ if (filter->p->prefixlen == p->prefixlen &&
prefix_match(filter->p, p))
+ return 1;
+
+ return 0;
+ }
+ }
+
+ return 0;
+}
+
+/* Return true if this peer is on the per_peer_list of peers to debug
+ * for BGP_DEBUG_TYPE
+ */
+static int
+bgp_debug_per_peer(struct peer *peer, unsigned long term_bgp_debug_type,
+ unsigned int BGP_DEBUG_TYPE, struct list *per_peer_list)
+{
+ struct bgp_debug_filter *filter;
+ struct listnode *node, *nnode;
+
+ if (term_bgp_debug_type & BGP_DEBUG_TYPE)
+ {
+ /* We are debugging all peers so return true */
+ if (!per_peer_list || list_isempty(per_peer_list))
+ return 1;
+
+ else
+ {
+ if (!peer)
+ return 0;
+
+ for (ALL_LIST_ELEMENTS (per_peer_list, node, nnode, filter))
+ if (filter->peer == peer)
+ return 1;
+
+ return 0;
+ }
+ }
+
+ return 0;
+}
+
+int
+bgp_debug_neighbor_events (struct peer *peer)
+{
+ return bgp_debug_per_peer (peer,
+ term_bgp_debug_neighbor_events,
+ BGP_DEBUG_NEIGHBOR_EVENTS,
+ bgp_debug_neighbor_events_peers);
+}
+
+int
+bgp_debug_keepalive (struct peer *peer)
+{
+ return bgp_debug_per_peer (peer,
+ term_bgp_debug_keepalive,
+ BGP_DEBUG_KEEPALIVE,
+ bgp_debug_keepalive_peers);
+}
+
+int
+bgp_debug_update (struct peer *peer, struct prefix *p, unsigned int inbound)
+{
+ if (inbound)
+ {
+ if (bgp_debug_per_peer (peer, term_bgp_debug_update, BGP_DEBUG_UPDATE_IN,
+ bgp_debug_update_in_peers))
+ return 1;
+ }
+
+ /* outbound */
+ else
+ {
+ if (bgp_debug_per_peer (peer, term_bgp_debug_update,
+ BGP_DEBUG_UPDATE_OUT,
+ bgp_debug_update_out_peers))
+ return 1;
+ }
+
+
+ if (BGP_DEBUG (update, UPDATE_PREFIX))
+ {
+ if (bgp_debug_per_prefix (p, term_bgp_debug_update,
+ BGP_DEBUG_UPDATE_PREFIX,
+ bgp_debug_update_prefixes))
+ return 1;
+ }
+
+ return 0;
+}
+
+int
+bgp_debug_zebra (struct prefix *p)
+{
+ if (BGP_DEBUG (zebra, ZEBRA))
+ {
+ if (bgp_debug_per_prefix (p, term_bgp_debug_zebra, BGP_DEBUG_ZEBRA,
+ bgp_debug_zebra_prefixes))
+ return 1;
+ }
+
+ return 0;
Some questions on this, as it's a very high "churn" patch, and so will
cause others to have to edit conflicts as they adjust. I.e., it's worth
making sure it's as good as possible first time.
Is this better:
- if (BGP_DEBUG (events, EVENTS))
+ if (bgp_debug_neighbor_events (NULL))
- if (BGP_DEBUG (events, EVENTS))
+ if (bgp_debug_update(peer, NULL, 0))
just from a typing POV? :)
Also, it goes from an in-place test of a flag variable (via macro) to a
function call, to check whether to call the logging function?:
if (bgp_debug_foo (arg))
zlog_debug ("....", ...);
If we take a function call hit anyway, then the logging call can go into
the conditional test, or not? Just:
bgp_debug_foo (arg, "...", ...)
?
Also, implementation wise, the debug test has O(#peers) or O(#prefixes)
scaling? For bgp_debug_per_peer, the peer is an arg, so why not just
store the 'debug enabled for this peer?' bit in the peer struct itself,
instead of having a separate list? The prefix one is tougher...
regards,
--
Paul Jakma | [email protected] | @pjakma | Key ID: 0xD86BF79464A2FF6A
Fortune:
Research is to see what everybody else has seen, and think what nobody
else has thought.
_______________________________________________
Quagga-dev mailing list
[email protected]
https://lists.quagga.net/mailman/listinfo/quagga-dev
