Look, I agree debugs are needed but most of what I removed were unguarded debugs. These will simply just fill up the debug log with useless cruft that makes it harder to actually debug issues because of unnecessary data. If you need these debugs then submit a patch so that they can be turned on appropriately. I submit though that debugs that say 'entering function', 'exiting function' don't do allot to help you figure issues out. donald On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote:

Donald,

Huh? Debug logs are there for when debugging is needed - and that's what these are all about. Simply removing them is wrong and make others lives much more difficult (ours) when debugging.

What logs are filling? (I.e Who normally runs with debug logging?)

What type of "guards" would you like to see? We/you certain can add a debug_bgp_vnc flag if that's what you're thinking.

I really object to the summary removal of these debug log statements.

Lou

On December 3, 2016 10:40:12 PM Donald Sharp <sharpd@cumulusnetworks.com> wrote:

...

Remove allot of unguarded debugs in bgp that are causing the log file to be filed up.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/bgp_routemap.c | 2 -- bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- bgpd/rfapi/rfapi_import.c | 24 ----------------------- bgpd/rfapi/rfapi_monitor.c | 4 ---- bgpd/rfapi/rfapi_rib.c | 10 ---------- bgpd/rfapi/rfapi_vty.c | 46 --------------------------------------------- bgpd/rfapi/vnc_export_bgp.c | 14 -------------- bgpd/rfapi/vnc_import_bgp.c | 2 -- bgpd/rfapi/vnc_zebra.c | 11 ----------- 9 files changed, 120 deletions(-)

diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index ea42cb5..88a169a 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) bgp_route_map_process_update(bgp, rmap_name, 1);

#if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif return 0; @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) bgp_route_map_process_update(bgp, rmap_name, 0); #if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif } diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c index d064c50..a0d80d6 100644 --- a/bgpd/rfapi/bgp_rfapi_cfg.c +++ b/bgpd/rfapi/bgp_rfapi_cfg.c @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); memset (redist_was_enabled, 0, sizeof (redist_was_enabled));

/* @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

static void @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); /* * If we turned off redistribution above, turn it back on. Doing so * will tell zebra to resend the routes to us @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

DEFUN (vnc_redistribute_rh_roo_localadmin, @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) struct rfapi_cfg *hc; int i;

- zlog_debug ("%s(arg=%s)", __func__, unused); - if (!bgp) { zlog_debug ("%s: No BGP process is configured", __func__); @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) vnc_redistribute_prechange (bgp); vnc_redistribute_postchange (bgp);

- zlog_debug ("%s done", __func__); }

static void diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 77da4f9..54643ee 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( struct rfapi_next_hop_entry *new; int have_vnc_tunnel_un = 0;

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); -#endif - new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct rfapi_next_hop_entry)); assert (new);

@@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry (

new->un_options = rfapi_encap_tlv_to_un_option (bi->attr);

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", - __func__, __LINE__, have_vnc_tunnel_un); -#endif - if (!have_vnc_tunnel_un && bi && bi->extra) { /* @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( break; }

- zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, - action_str, - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), p->prefixlen); - memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); memset (&p_firstbi_new, 0, sizeof (p_firstbi_new));

@@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( if (import_table == bgp->rfapi->it_ce) is_it_ce = 1;

- zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, - (is_it_ce ? "CE-IT " : ""), - action_str, - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), - p->prefixlen, import_table, afi2str (afi)); - VNC_ITRCCK;

/* @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( buf_pfx[0] = '*'; buf_pfx[1] = 0; } - - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", - __func__, buf_pfx, delete_active, delete_holddown); } #endif

diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c index 216b45e..8e33530 100644 --- a/bgpd/rfapi/rfapi_monitor.c +++ b/bgpd/rfapi/rfapi_monitor.c @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) struct bgp *bgp; int count = 0;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - bgp = bgp_get_default ();

if (rfd->mon) @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( struct rfapi_monitor_eth mon_buf; int rc;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - assert (rfd->mon_eth);

memset ((void *) &mon_buf, 0, sizeof (mon_buf)); diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c index 896b5f5..40d00ca 100644 --- a/bgpd/rfapi/rfapi_rib.c +++ b/bgpd/rfapi/rfapi_rib.c @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( int count = 0; char buf[BUFSIZ];

- zlog_debug ("%s: entry", __func__); - if (CHECK_FLAG (bgp->rfapi_cfg->flags, BGP_VNC_CONFIG_CALLBACK_DISABLE)) return;

@@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( pn = route_node_get (rfd->rib_pending[afi], prefix); assert (pn);

- zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, - pn->aggregate); - if (pn->aggregate) { /* @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( char buf[BUFSIZ];

prefix2str (&it_node->p, buf, BUFSIZ); - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", - __func__, it, afi, it_node, buf);

if (AFI_ETHER == afi) { @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( */ if ((sl = RFAPI_MONITOR_ETH (it_node))) { - - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); - for (cursor = NULL, rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); !rc; rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor)) diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c index c198564..1f25c72 100644 --- a/bgpd/rfapi/rfapi_vty.c +++ b/bgpd/rfapi/rfapi_vty.c @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct rfapi_next_hop_entry *tail = NULL; struct rfapi_cfg *rfapi_cfg;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: entry", __func__); -#endif - if (!bgp_default) return ENXIO;

@@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) rfapiQprefix2Rprefix (pPrefix, &rprefix); }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: starting descriptor loop", __func__); -#endif - for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) { struct rfapi_adb *adb; @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct nve_addr ha; struct nve_addr *hap;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: rfd=%p", __func__, rfd); -#endif - /* * match un, vn addresses of NVEs */ @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) continue;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: un, vn match", __func__); -#endif - /* * match prefix */ @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (!prefix_same (pPrefix, &adb->prefix_ip)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, prefix doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) (cda->l2o.o.macaddr.octet, adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, macaddr doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, LNI doesn't match, skipping", - __func__, adb); -#endif continue; } }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, - adb); -#endif - listnode_add (adb_delete_list, adb); }

@@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) pVn = NULL; }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, pVn, RFAPI_REGISTER_KILL); if (!rc) { @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) continue; } } -#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 adding adb %p to delete list", - __func__, adb); -#endif listnode_add (adb_delete_list, adb); }

@@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; vn.v.l2addr = adb->l2o;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 killing reg from adb %p ", - __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, &vn, RFAPI_REGISTER_KILL); if (!rc) diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c index 6434c37..f1a9ecb 100644 --- a/bgpd/rfapi/vnc_export_bgp.c +++ b/bgpd/rfapi/vnc_export_bgp.c @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t afi) struct route_node *rn; struct bgp_info *ri;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( struct attr attr = { 0 }; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( struct route_node *rn; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t afi) struct rfapi_import_table *it; uint8_t family = afi2family (afi);

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t afi) struct bgp_node *prn; struct rfapi_cfg *hc;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index dc2640a..0ba927d 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; - zlog_debug ("%s: return", __func__); }

@@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; - zlog_debug ("%s: return", __func__); } diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c index e357ef6..bc77ca4 100644 --- a/bgpd/rfapi/vnc_zebra.c +++ b/bgpd/rfapi/vnc_zebra.c @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) struct bgp_node *prn; struct bgp_node *rn;

- zlog_debug ("%s: entry", __func__); - if (!bgp) return; if (!bgp->rfapi_cfg) @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) } } } - zlog_debug ("%s: return", __func__); }

/* @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( // struct prefix *nhpp; void *pAddr;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry", __func__); import_table = rfg->rfapi_import_table; if (!import_table) { @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) void vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) { - zlog_debug ("%s: entry", __func__); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); } @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, int type) /* Withdraw redistributed routes from current BGP's routing table. */ vnc_redistribute_withdraw (bgp, afi, type);

- zlog_debug ("%s: return", __func__); - return CMD_SUCCESS; }

-- 2.7.4

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

For tracing function entry and exit GCC has -finstrument-functions. On Sun, Dec 4, 2016 at 11:02 AM, Lou Berger <lberger@labn.net> wrote:

Donald,

On 12/4/2016 9:16 AM, Donald Sharp wrote:

...

Look, I agree debugs are needed but most of what I removed were unguarded debugs. Please elaborate on what a "guarded debug" looks like to you -- do you mean controlled by a debug ... statement, is if something that is conditionally compiled? Please point to an example of what you'd like to see. The former is certainly a reasonable change. These will simply just fill up the debug log with useless cruft that makes it harder to actually debug issues because of unnecessary data. Unnecessary to whom? If I'm debugging something related, you've made it impossible and also made it a lot of work to readd these statements.

...

If you need these debugs then submit a patch so that they can be turned on appropriately. We though the code was appropriate, please define what you think is appropriate. Perhaps there's something to be added to HACKING.md to capture this view.

...

I submit though that debugs that say 'entering function', 'exiting function' don't do allot to help you figure issues out.

While that is certainly one perspective, it isn't one shared by the developer of that code.

Lou

...

donald

On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

Huh? Debug logs are there for when debugging is needed - and that's

what

...

...

these are all about. Simply removing them is wrong and make others lives much more difficult (ours) when debugging.

What logs are filling? (I.e Who normally runs with debug logging?)

What type of "guards" would you like to see? We/you certain can add a debug_bgp_vnc flag if that's what you're thinking.

I really object to the summary removal of these debug log statements.

Lou

On December 3, 2016 10:40:12 PM Donald Sharp < sharpd@cumulusnetworks.com> wrote:

...

Remove allot of unguarded debugs in bgp that are causing the log file to be filed up.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/bgp_routemap.c | 2 -- bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- bgpd/rfapi/rfapi_import.c | 24 ----------------------- bgpd/rfapi/rfapi_monitor.c | 4 ---- bgpd/rfapi/rfapi_rib.c | 10 ---------- bgpd/rfapi/rfapi_vty.c | 46 --------------------------------------------- bgpd/rfapi/vnc_export_bgp.c | 14 -------------- bgpd/rfapi/vnc_import_bgp.c | 2 -- bgpd/rfapi/vnc_zebra.c | 11 ----------- 9 files changed, 120 deletions(-)

diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index ea42cb5..88a169a 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) bgp_route_map_process_update(bgp, rmap_name, 1);

#if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif return 0; @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) bgp_route_map_process_update(bgp, rmap_name, 0); #if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif } diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c index d064c50..a0d80d6 100644 --- a/bgpd/rfapi/bgp_rfapi_cfg.c +++ b/bgpd/rfapi/bgp_rfapi_cfg.c @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); memset (redist_was_enabled, 0, sizeof (redist_was_enabled));

/* @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

static void @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); /* * If we turned off redistribution above, turn it back on. Doing so * will tell zebra to resend the routes to us @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

DEFUN (vnc_redistribute_rh_roo_localadmin, @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) struct rfapi_cfg *hc; int i;

- zlog_debug ("%s(arg=%s)", __func__, unused); - if (!bgp) { zlog_debug ("%s: No BGP process is configured", __func__); @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) vnc_redistribute_prechange (bgp); vnc_redistribute_postchange (bgp);

- zlog_debug ("%s done", __func__); }

static void diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 77da4f9..54643ee 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( struct rfapi_next_hop_entry *new; int have_vnc_tunnel_un = 0;

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); -#endif - new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct rfapi_next_hop_entry)); assert (new);

@@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry (

new->un_options = rfapi_encap_tlv_to_un_option (bi->attr);

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", - __func__, __LINE__, have_vnc_tunnel_un); -#endif - if (!have_vnc_tunnel_un && bi && bi->extra) { /* @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( break; }

- zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, - action_str, - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), p->prefixlen); - memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); memset (&p_firstbi_new, 0, sizeof (p_firstbi_new));

@@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( if (import_table == bgp->rfapi->it_ce) is_it_ce = 1;

- zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, - (is_it_ce ? "CE-IT " : ""), - action_str, - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), - p->prefixlen, import_table, afi2str (afi)); - VNC_ITRCCK;

/* @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( buf_pfx[0] = '*'; buf_pfx[1] = 0; } - - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", - __func__, buf_pfx, delete_active, delete_holddown); } #endif

diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c index 216b45e..8e33530 100644 --- a/bgpd/rfapi/rfapi_monitor.c +++ b/bgpd/rfapi/rfapi_monitor.c @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) struct bgp *bgp; int count = 0;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - bgp = bgp_get_default ();

if (rfd->mon) @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( struct rfapi_monitor_eth mon_buf; int rc;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - assert (rfd->mon_eth);

memset ((void *) &mon_buf, 0, sizeof (mon_buf)); diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c index 896b5f5..40d00ca 100644 --- a/bgpd/rfapi/rfapi_rib.c +++ b/bgpd/rfapi/rfapi_rib.c @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( int count = 0; char buf[BUFSIZ];

- zlog_debug ("%s: entry", __func__); - if (CHECK_FLAG (bgp->rfapi_cfg->flags, BGP_VNC_CONFIG_CALLBACK_DISABLE)) return;

@@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( pn = route_node_get (rfd->rib_pending[afi], prefix); assert (pn);

- zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, - pn->aggregate); - if (pn->aggregate) { /* @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( char buf[BUFSIZ];

prefix2str (&it_node->p, buf, BUFSIZ); - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", - __func__, it, afi, it_node, buf);

if (AFI_ETHER == afi) { @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( */ if ((sl = RFAPI_MONITOR_ETH (it_node))) { - - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); - for (cursor = NULL, rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); !rc; rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor)) diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c index c198564..1f25c72 100644 --- a/bgpd/rfapi/rfapi_vty.c +++ b/bgpd/rfapi/rfapi_vty.c @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct rfapi_next_hop_entry *tail = NULL; struct rfapi_cfg *rfapi_cfg;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: entry", __func__); -#endif - if (!bgp_default) return ENXIO;

@@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) rfapiQprefix2Rprefix (pPrefix, &rprefix); }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: starting descriptor loop", __func__); -#endif - for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) { struct rfapi_adb *adb; @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct nve_addr ha; struct nve_addr *hap;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: rfd=%p", __func__, rfd); -#endif - /* * match un, vn addresses of NVEs */ @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) continue;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: un, vn match", __func__); -#endif - /* * match prefix */ @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (!prefix_same (pPrefix, &adb->prefix_ip)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, prefix doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) (cda->l2o.o.macaddr.octet, adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, macaddr doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, LNI doesn't match, skipping", - __func__, adb); -#endif continue; } }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, - adb); -#endif - listnode_add (adb_delete_list, adb); }

@@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) pVn = NULL; }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, pVn, RFAPI_REGISTER_KILL); if (!rc) { @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) continue; } } -#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 adding adb %p to delete list", - __func__, adb); -#endif listnode_add (adb_delete_list, adb); }

@@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; vn.v.l2addr = adb->l2o;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 killing reg from adb %p ", - __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, &vn, RFAPI_REGISTER_KILL); if (!rc) diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c index 6434c37..f1a9ecb 100644 --- a/bgpd/rfapi/vnc_export_bgp.c +++ b/bgpd/rfapi/vnc_export_bgp.c @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t afi) struct route_node *rn; struct bgp_info *ri;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( struct attr attr = { 0 }; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( struct route_node *rn; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t afi) struct rfapi_import_table *it; uint8_t family = afi2family (afi);

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t afi) struct bgp_node *prn; struct rfapi_cfg *hc;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index dc2640a..0ba927d 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; - zlog_debug ("%s: return", __func__); }

@@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; - zlog_debug ("%s: return", __func__); } diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c index e357ef6..bc77ca4 100644 --- a/bgpd/rfapi/vnc_zebra.c +++ b/bgpd/rfapi/vnc_zebra.c @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) struct bgp_node *prn; struct bgp_node *rn;

- zlog_debug ("%s: entry", __func__); - if (!bgp) return; if (!bgp->rfapi_cfg) @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) } } } - zlog_debug ("%s: return", __func__); }

/* @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( // struct prefix *nhpp; void *pAddr;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry", __func__); import_table = rfg->rfapi_import_table; if (!import_table) { @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) void vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) { - zlog_debug ("%s: entry", __func__); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); } @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, int type) /* Withdraw redistributed routes from current BGP's routing table. */ vnc_redistribute_withdraw (bgp, afi, type);

- zlog_debug ("%s: return", __func__); - return CMD_SUCCESS; }

-- 2.7.4

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

Donald, I'm not opposed to adding 'guarded debug' or submitting such a patch, but I do think this is something to be agreed upon and something to not catch other developers by surprise. Are "Guarded debug" defined/covered in any existing developer docs? If so, where? -- and I apologize for missing it. If not, please submit a patch showing the definition and conventions you'd like developers (i.e. us) to follow -- and assuming no one objects, we'll follow this. Without such a definition - we/others are just guessing at what one person finds acceptable. Lou On 12/5/2016 7:54 AM, Donald Sharp wrote:

Guarded debug - A debug that must be turned on by a cli command, ex 'debug bgp underpants' would turn on underpants bgp debugging . I am constantly hearing from internal test as well as actual customer cases when we make the mistake of not guarding a debug.. I believe that they are right and we must think carefully about what is displayed in the log when it is under normal operations( no debugs ).

I stand by this patch, it removes unguarded debugs. If you feel that it is in error, I welcome a patch that appropriately addresses this issue to your satisfaction.

donald

On Sun, Dec 4, 2016 at 11:02 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

On 12/4/2016 9:16 AM, Donald Sharp wrote:

...

Look, I agree debugs are needed but most of what I removed were unguarded debugs. Please elaborate on what a "guarded debug" looks like to you -- do you mean controlled by a debug ... statement, is if something that is conditionally compiled? Please point to an example of what you'd like to see. The former is certainly a reasonable change. These will simply just fill up the debug log with useless cruft that makes it harder to actually debug issues because of unnecessary data. Unnecessary to whom? If I'm debugging something related, you've made it impossible and also made it a lot of work to readd these statements.

...

If you need these debugs then submit a patch so that they can be turned on appropriately. We though the code was appropriate, please define what you think is appropriate. Perhaps there's something to be added to HACKING.md to capture this view.

...

I submit though that debugs that say 'entering function', 'exiting function' don't do allot to help you figure issues out. While that is certainly one perspective, it isn't one shared by the developer of that code.

Lou

...

donald

On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

Huh? Debug logs are there for when debugging is needed - and that's what these are all about. Simply removing them is wrong and make others lives much more difficult (ours) when debugging.

What logs are filling? (I.e Who normally runs with debug logging?)

What type of "guards" would you like to see? We/you certain can add a debug_bgp_vnc flag if that's what you're thinking.

I really object to the summary removal of these debug log statements.

Lou

On December 3, 2016 10:40:12 PM Donald Sharp <sharpd@cumulusnetworks.com> wrote:

...

Remove allot of unguarded debugs in bgp that are causing the log file to be filed up.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> --- bgpd/bgp_routemap.c | 2 -- bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- bgpd/rfapi/rfapi_import.c | 24 ----------------------- bgpd/rfapi/rfapi_monitor.c | 4 ---- bgpd/rfapi/rfapi_rib.c | 10 ---------- bgpd/rfapi/rfapi_vty.c | 46 --------------------------------------------- bgpd/rfapi/vnc_export_bgp.c | 14 -------------- bgpd/rfapi/vnc_import_bgp.c | 2 -- bgpd/rfapi/vnc_zebra.c | 11 ----------- 9 files changed, 120 deletions(-)

diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c index ea42cb5..88a169a 100644 --- a/bgpd/bgp_routemap.c +++ b/bgpd/bgp_routemap.c @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) bgp_route_map_process_update(bgp, rmap_name, 1);

#if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif return 0; @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) bgp_route_map_process_update(bgp, rmap_name, 0); #if ENABLE_BGP_VNC - zlog_debug("%s: calling vnc_routemap_update", __func__); vnc_routemap_update(bgp, __func__); #endif } diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c index d064c50..a0d80d6 100644 --- a/bgpd/rfapi/bgp_rfapi_cfg.c +++ b/bgpd/rfapi/bgp_rfapi_cfg.c @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); memset (redist_was_enabled, 0, sizeof (redist_was_enabled));

/* @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

static void @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) afi_t afi; int type;

- zlog_debug ("%s: entry", __func__); /* * If we turned off redistribution above, turn it back on. Doing so * will tell zebra to resend the routes to us @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) } } } - zlog_debug ("%s: return", __func__); }

DEFUN (vnc_redistribute_rh_roo_localadmin, @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) struct rfapi_cfg *hc; int i;

- zlog_debug ("%s(arg=%s)", __func__, unused); - if (!bgp) { zlog_debug ("%s: No BGP process is configured", __func__); @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char *unused) vnc_redistribute_prechange (bgp); vnc_redistribute_postchange (bgp);

- zlog_debug ("%s done", __func__); }

static void diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 77da4f9..54643ee 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( struct rfapi_next_hop_entry *new; int have_vnc_tunnel_un = 0;

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); -#endif - new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct rfapi_next_hop_entry)); assert (new);

@@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry (

new->un_options = rfapi_encap_tlv_to_un_option (bi->attr);

-#if DEBUG_ENCAP_MONITOR - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", - __func__, __LINE__, have_vnc_tunnel_un); -#endif - if (!have_vnc_tunnel_un && bi && bi->extra) { /* @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( } }

- zlog_debug ("%s: returning %d routes", __func__, count); return biglist; }

@@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( break; }

- zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, - action_str, - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), p->prefixlen); - memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); memset (&p_firstbi_new, 0, sizeof (p_firstbi_new));

@@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( if (import_table == bgp->rfapi->it_ce) is_it_ce = 1;

- zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, - (is_it_ce ? "CE-IT " : ""), - action_str, - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), - p->prefixlen, import_table, afi2str (afi)); - VNC_ITRCCK;

/* @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( buf_pfx[0] = '*'; buf_pfx[1] = 0; } - - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", - __func__, buf_pfx, delete_active, delete_holddown); } #endif

diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c index 216b45e..8e33530 100644 --- a/bgpd/rfapi/rfapi_monitor.c +++ b/bgpd/rfapi/rfapi_monitor.c @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) struct bgp *bgp; int count = 0;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - bgp = bgp_get_default ();

if (rfd->mon) @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( struct rfapi_monitor_eth mon_buf; int rc;

- zlog_debug ("%s: entry rfd=%p", __func__, rfd); - assert (rfd->mon_eth);

memset ((void *) &mon_buf, 0, sizeof (mon_buf)); diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c index 896b5f5..40d00ca 100644 --- a/bgpd/rfapi/rfapi_rib.c +++ b/bgpd/rfapi/rfapi_rib.c @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( int count = 0; char buf[BUFSIZ];

- zlog_debug ("%s: entry", __func__); - if (CHECK_FLAG (bgp->rfapi_cfg->flags, BGP_VNC_CONFIG_CALLBACK_DISABLE)) return;

@@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( pn = route_node_get (rfd->rib_pending[afi], prefix); assert (pn);

- zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, - pn->aggregate); - if (pn->aggregate) { /* @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( char buf[BUFSIZ];

prefix2str (&it_node->p, buf, BUFSIZ); - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", - __func__, it, afi, it_node, buf);

if (AFI_ETHER == afi) { @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( */ if ((sl = RFAPI_MONITOR_ETH (it_node))) { - - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); - for (cursor = NULL, rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); !rc; rc = skiplist_next (sl, NULL, (void **) &m, (void **) &cursor)) diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c index c198564..1f25c72 100644 --- a/bgpd/rfapi/rfapi_vty.c +++ b/bgpd/rfapi/rfapi_vty.c @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct rfapi_next_hop_entry *tail = NULL; struct rfapi_cfg *rfapi_cfg;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: entry", __func__); -#endif - if (!bgp_default) return ENXIO;

@@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) rfapiQprefix2Rprefix (pPrefix, &rprefix); }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: starting descriptor loop", __func__); -#endif - for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) { struct rfapi_adb *adb; @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) struct nve_addr ha; struct nve_addr *hap;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: rfd=%p", __func__, rfd); -#endif - /* * match un, vn addresses of NVEs */ @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) continue;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: un, vn match", __func__); -#endif - /* * match prefix */ @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (!prefix_same (pPrefix, &adb->prefix_ip)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, prefix doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) (cda->l2o.o.macaddr.octet, adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, macaddr doesn't match, skipping", - __func__, adb); -#endif continue; } } @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) { if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) { -#if DEBUG_L2_EXTRA - zlog_debug ("%s: adb=%p, LNI doesn't match, skipping", - __func__, adb); -#endif continue; } }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, - adb); -#endif - listnode_add (adb_delete_list, adb); }

@@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) pVn = NULL; }

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, pVn, RFAPI_REGISTER_KILL); if (!rc) { @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) continue; } } -#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 adding adb %p to delete list", - __func__, adb); -#endif listnode_add (adb_delete_list, adb); }

@@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct rfapi_local_reg_delete_arg *cda) vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; vn.v.l2addr = adb->l2o;

-#if DEBUG_L2_EXTRA - zlog_debug ("%s: ip0 killing reg from adb %p ", - __func__, adb); -#endif - rc = rfapi_register (rfd, &rp, 0, NULL, &vn, RFAPI_REGISTER_KILL); if (!rc) diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c index 6434c37..f1a9ecb 100644 --- a/bgpd/rfapi/vnc_export_bgp.c +++ b/bgpd/rfapi/vnc_export_bgp.c @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t afi) struct route_node *rn; struct bgp_info *ri;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( struct attr attr = { 0 }; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( struct route_node *rn; struct rfapi_import_table *import_table;

- zlog_debug ("%s: entry", __func__); - import_table = rfg->rfapi_import_table; if (!import_table) { @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t afi) struct rfapi_import_table *it; uint8_t family = afi2family (afi);

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t afi) struct bgp_node *prn; struct rfapi_cfg *hc;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

@@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, afi_t afi) { struct bgp_node *rn;

- zlog_debug ("%s: entry, afi=%d", __func__, afi); - if (!bgp) return;

diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c index dc2640a..0ba927d 100644 --- a/bgpd/rfapi/vnc_import_bgp.c +++ b/bgpd/rfapi/vnc_import_bgp.c @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; - zlog_debug ("%s: return", __func__); }

@@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp *bgp, afi_t afi) }

bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; - zlog_debug ("%s: return", __func__); } diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c index e357ef6..bc77ca4 100644 --- a/bgpd/rfapi/vnc_zebra.c +++ b/bgpd/rfapi/vnc_zebra.c @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) struct bgp_node *prn; struct bgp_node *rn;

- zlog_debug ("%s: entry", __func__); - if (!bgp) return; if (!bgp->rfapi_cfg) @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, uint8_t type) } } } - zlog_debug ("%s: return", __func__); }

/* @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( // struct prefix *nhpp; void *pAddr;

- zlog_debug ("%s: entry, add=%d", __func__, add); - if (zclient_vnc->sock < 0) return;

@@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( void *nh_ary = NULL; void *nhp_ary = NULL;

- zlog_debug ("%s: entry", __func__); import_table = rfg->rfapi_import_table; if (!import_table) { @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) void vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) { - zlog_debug ("%s: entry", __func__); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); } @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, int type) /* Withdraw redistributed routes from current BGP's routing table. */ vnc_redistribute_withdraw (bgp, afi, type);

- zlog_debug ("%s: return", __func__); - return CMD_SUCCESS; }

-- 2.7.4

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

I'm also attempting to argue( quite poorly apparently) that let's just remove the debugs until such time that the appropriate 'debug bgp vnc' code has been added in. They are filling up my logs and making it hard for me to find real issues that I am looking at. My proposal: Take this patch In the future when the appropriate debug guards have been coded, revert the patch and apply debug guard patch on top of it. donald On Mon, Dec 5, 2016 at 8:42 AM, David Lamparter <david@opensourcerouting.org> wrote:

Donald & Lou,

I think there's a communication failure here... here's what I understand:

Donald is arguing that there should be a "debug vnc" CLI command (or possibly more than one such command) which gates all of these debug messages. (I agree with that.)

Lou is arguing that the debug statements are useful and should remain in the code. (I agree with that, too - though some of them are a bit excessive IMHO and create logspam.)

So the result is that we need to add a "debug vnc" command and put these zlog_debug calls under an "if (debug ...)".

Lou is also right that we: - need to document this somewhere, i.e. that there's "debug XYZ" which should control which parts of the code you get debug logs from - maybe can improve the way this is handled (because adding "debug XYZ" is manual and open-coded everywhere, this could be better.)

Cheers,

-David

P.S.: Mechanically adding the "if ()" everywhere can probably be done with coccinelle [http://coccinelle.lip6.fr/docs/index.html]

On Mon, Dec 05, 2016 at 08:29:14AM -0500, Lou Berger wrote:

...

Donald,

I'm not opposed to adding 'guarded debug' or submitting such a patch, but I do think this is something to be agreed upon and something to not catch other developers by surprise.

Are "Guarded debug" defined/covered in any existing developer docs? If so, where? -- and I apologize for missing it. If not, please submit a patch showing the definition and conventions you'd like developers (i.e. us) to follow -- and assuming no one objects, we'll follow this.

Without such a definition - we/others are just guessing at what one person finds acceptable.

Lou

On 12/5/2016 7:54 AM, Donald Sharp wrote:

...

Guarded debug - A debug that must be turned on by a cli command, ex 'debug bgp underpants' would turn on underpants bgp debugging . I am constantly hearing from internal test as well as actual customer cases when we make the mistake of not guarding a debug.. I believe that they are right and we must think carefully about what is displayed in the log when it is under normal operations( no debugs ).

I stand by this patch, it removes unguarded debugs. If you feel that it is in error, I welcome a patch that appropriately addresses this issue to your satisfaction.

donald

On Sun, Dec 4, 2016 at 11:02 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

On 12/4/2016 9:16 AM, Donald Sharp wrote:

...

Look, I agree debugs are needed but most of what I removed were unguarded debugs. Please elaborate on what a "guarded debug" looks like to you -- do you mean controlled by a debug ... statement, is if something that is conditionally compiled? Please point to an example of what you'd like to see. The former is certainly a reasonable change. These will simply just fill up the debug log with useless cruft that makes it harder to actually debug issues because of unnecessary data. Unnecessary to whom? If I'm debugging something related, you've made it impossible and also made it a lot of work to readd these statements.

...

If you need these debugs then submit a patch so that they can be turned on appropriately. We though the code was appropriate, please define what you think is appropriate. Perhaps there's something to be added to HACKING.md to capture this view.

...

I submit though that debugs that say 'entering function', 'exiting function' don't do allot to help you figure issues out. While that is certainly one perspective, it isn't one shared by the developer of that code.

Lou

...

donald

On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

Huh? Debug logs are there for when debugging is needed - and that's what these are all about. Simply removing them is wrong and make others lives much more difficult (ours) when debugging.

What logs are filling? (I.e Who normally runs with debug logging?)

What type of "guards" would you like to see? We/you certain can add a debug_bgp_vnc flag if that's what you're thinking.

I really object to the summary removal of these debug log statements.

Lou

On December 3, 2016 10:40:12 PM Donald Sharp <sharpd@cumulusnetworks.com> wrote:

> Remove allot of unguarded debugs in bgp > that are causing the log file to be filed > up. > > Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> > --- > bgpd/bgp_routemap.c | 2 -- > bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- > bgpd/rfapi/rfapi_import.c | 24 ----------------------- > bgpd/rfapi/rfapi_monitor.c | 4 ---- > bgpd/rfapi/rfapi_rib.c | 10 ---------- > bgpd/rfapi/rfapi_vty.c | 46 > --------------------------------------------- > bgpd/rfapi/vnc_export_bgp.c | 14 -------------- > bgpd/rfapi/vnc_import_bgp.c | 2 -- > bgpd/rfapi/vnc_zebra.c | 11 ----------- > 9 files changed, 120 deletions(-) > > diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c > index ea42cb5..88a169a 100644 > --- a/bgpd/bgp_routemap.c > +++ b/bgpd/bgp_routemap.c > @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) > bgp_route_map_process_update(bgp, rmap_name, 1); > > #if ENABLE_BGP_VNC > - zlog_debug("%s: calling vnc_routemap_update", __func__); > vnc_routemap_update(bgp, __func__); > #endif > return 0; > @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) > for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) > bgp_route_map_process_update(bgp, rmap_name, 0); > #if ENABLE_BGP_VNC > - zlog_debug("%s: calling vnc_routemap_update", __func__); > vnc_routemap_update(bgp, __func__); > #endif > } > diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c > index d064c50..a0d80d6 100644 > --- a/bgpd/rfapi/bgp_rfapi_cfg.c > +++ b/bgpd/rfapi/bgp_rfapi_cfg.c > @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) > afi_t afi; > int type; > > - zlog_debug ("%s: entry", __func__); > memset (redist_was_enabled, 0, sizeof (redist_was_enabled)); > > /* > @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > static void > @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) > afi_t afi; > int type; > > - zlog_debug ("%s: entry", __func__); > /* > * If we turned off redistribution above, turn it back on. Doing so > * will tell zebra to resend the routes to us > @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > DEFUN (vnc_redistribute_rh_roo_localadmin, > @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char > *unused) > struct rfapi_cfg *hc; > int i; > > - zlog_debug ("%s(arg=%s)", __func__, unused); > - > if (!bgp) > { > zlog_debug ("%s: No BGP process is configured", __func__); > @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char > *unused) > vnc_redistribute_prechange (bgp); > vnc_redistribute_postchange (bgp); > > - zlog_debug ("%s done", __func__); > } > > static void > diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c > index 77da4f9..54643ee 100644 > --- a/bgpd/rfapi/rfapi_import.c > +++ b/bgpd/rfapi/rfapi_import.c > @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( > struct rfapi_next_hop_entry *new; > int have_vnc_tunnel_un = 0; > > -#if DEBUG_ENCAP_MONITOR > - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); > -#endif > - > new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct > rfapi_next_hop_entry)); > assert (new); > > @@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry ( > > new->un_options = rfapi_encap_tlv_to_un_option (bi->attr); > > -#if DEBUG_ENCAP_MONITOR > - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", > - __func__, __LINE__, have_vnc_tunnel_un); > -#endif > - > if (!have_vnc_tunnel_un && bi && bi->extra) > { > /* > @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( > } > } > > - zlog_debug ("%s: returning %d routes", __func__, count); > return biglist; > } > > @@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( > } > } > > - zlog_debug ("%s: returning %d routes", __func__, count); > return biglist; > } > > @@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( > break; > } > > - zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, > - action_str, > - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), > p->prefixlen); > - > memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); > memset (&p_firstbi_new, 0, sizeof (p_firstbi_new)); > > @@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( > if (import_table == bgp->rfapi->it_ce) > is_it_ce = 1; > > - zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, > - (is_it_ce ? "CE-IT " : ""), > - action_str, > - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), > - p->prefixlen, import_table, afi2str (afi)); > - > VNC_ITRCCK; > > /* > @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( > buf_pfx[0] = '*'; > buf_pfx[1] = 0; > } > - > - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", > - __func__, buf_pfx, delete_active, delete_holddown); > } > #endif > > diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c > index 216b45e..8e33530 100644 > --- a/bgpd/rfapi/rfapi_monitor.c > +++ b/bgpd/rfapi/rfapi_monitor.c > @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) > struct bgp *bgp; > int count = 0; > > - zlog_debug ("%s: entry rfd=%p", __func__, rfd); > - > bgp = bgp_get_default (); > > if (rfd->mon) > @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( > struct rfapi_monitor_eth mon_buf; > int rc; > > - zlog_debug ("%s: entry rfd=%p", __func__, rfd); > - > assert (rfd->mon_eth); > > memset ((void *) &mon_buf, 0, sizeof (mon_buf)); > diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c > index 896b5f5..40d00ca 100644 > --- a/bgpd/rfapi/rfapi_rib.c > +++ b/bgpd/rfapi/rfapi_rib.c > @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( > int count = 0; > char buf[BUFSIZ]; > > - zlog_debug ("%s: entry", __func__); > - > if (CHECK_FLAG (bgp->rfapi_cfg->flags, > BGP_VNC_CONFIG_CALLBACK_DISABLE)) > return; > > @@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( > pn = route_node_get (rfd->rib_pending[afi], prefix); > assert (pn); > > - zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, > - pn->aggregate); > - > if (pn->aggregate) > { > /* > @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( > char buf[BUFSIZ]; > > prefix2str (&it_node->p, buf, BUFSIZ); > - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", > - __func__, it, afi, it_node, buf); > > if (AFI_ETHER == afi) > { > @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( > */ > if ((sl = RFAPI_MONITOR_ETH (it_node))) > { > - > - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); > - > for (cursor = NULL, rc = > skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); > !rc; > rc = skiplist_next (sl, NULL, (void **) &m, (void **) > &cursor)) > diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c > index c198564..1f25c72 100644 > --- a/bgpd/rfapi/rfapi_vty.c > +++ b/bgpd/rfapi/rfapi_vty.c > @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > struct rfapi_next_hop_entry *tail = NULL; > struct rfapi_cfg *rfapi_cfg; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: entry", __func__); > -#endif > - > if (!bgp_default) > return ENXIO; > > @@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > rfapiQprefix2Rprefix (pPrefix, &rprefix); > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: starting descriptor loop", __func__); > -#endif > - > for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) > { > struct rfapi_adb *adb; > @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > struct nve_addr ha; > struct nve_addr *hap; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: rfd=%p", __func__, rfd); > -#endif > - > /* > * match un, vn addresses of NVEs > */ > @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) > continue; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: un, vn match", __func__); > -#endif > - > /* > * match prefix > */ > @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > { > if (!prefix_same (pPrefix, &adb->prefix_ip)) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, prefix doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > (cda->l2o.o.macaddr.octet, > adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, macaddr doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > { > if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, LNI doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, > - adb); > -#endif > - > listnode_add (adb_delete_list, adb); > } > > @@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > pVn = NULL; > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, > adb); > -#endif > - > rc = rfapi_register (rfd, &rp, 0, NULL, pVn, > RFAPI_REGISTER_KILL); > if (!rc) > { > @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > continue; > } > } > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ip0 adding adb %p to delete list", > - __func__, adb); > -#endif > listnode_add (adb_delete_list, adb); > } > > @@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; > vn.v.l2addr = adb->l2o; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ip0 killing reg from adb %p ", > - __func__, adb); > -#endif > - > rc = rfapi_register (rfd, &rp, 0, NULL, &vn, > RFAPI_REGISTER_KILL); > if (!rc) > diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c > index 6434c37..f1a9ecb 100644 > --- a/bgpd/rfapi/vnc_export_bgp.c > +++ b/bgpd/rfapi/vnc_export_bgp.c > @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t > afi) > struct route_node *rn; > struct bgp_info *ri; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t > afi) > { > struct bgp_node *rn; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( > struct attr attr = { 0 }; > struct rfapi_import_table *import_table; > > - zlog_debug ("%s: entry", __func__); > - > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( > struct route_node *rn; > struct rfapi_import_table *import_table; > > - zlog_debug ("%s: entry", __func__); > - > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t > afi) > struct rfapi_import_table *it; > uint8_t family = afi2family (afi); > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t > afi) > struct bgp_node *prn; > struct rfapi_cfg *hc; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, > afi_t afi) > { > struct bgp_node *rn; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c > index dc2640a..0ba927d 100644 > --- a/bgpd/rfapi/vnc_import_bgp.c > +++ b/bgpd/rfapi/vnc_import_bgp.c > @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, > afi_t afi) > } > > bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; > - zlog_debug ("%s: return", __func__); > } > > > @@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp > *bgp, afi_t afi) > } > > bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; > - zlog_debug ("%s: return", __func__); > } > diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c > index e357ef6..bc77ca4 100644 > --- a/bgpd/rfapi/vnc_zebra.c > +++ b/bgpd/rfapi/vnc_zebra.c > @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, > uint8_t type) > struct bgp_node *prn; > struct bgp_node *rn; > > - zlog_debug ("%s: entry", __func__); > - > if (!bgp) > return; > if (!bgp->rfapi_cfg) > @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, > uint8_t type) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > /* > @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( > void *nh_ary = NULL; > void *nhp_ary = NULL; > > - zlog_debug ("%s: entry, add=%d", __func__, add); > - > if (zclient_vnc->sock < 0) > return; > > @@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( > // struct prefix *nhpp; > void *pAddr; > > - zlog_debug ("%s: entry, add=%d", __func__, add); > - > if (zclient_vnc->sock < 0) > return; > > @@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( > void *nh_ary = NULL; > void *nhp_ary = NULL; > > - zlog_debug ("%s: entry", __func__); > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct > rfapi_nve_group_cfg *rfg) > void > vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) > { > - zlog_debug ("%s: entry", __func__); > vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); > vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); > } > @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, > int type) > /* Withdraw redistributed routes from current BGP's routing table. */ > vnc_redistribute_withdraw (bgp, afi, type); > > - zlog_debug ("%s: return", __func__); > - > return CMD_SUCCESS; > } > > -- > 2.7.4 > > > _______________________________________________ > cmaster-next mailing list > cmaster-next@lists.nox.tf > https://lists.nox.tf/listinfo/cmaster-next

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

I am really not sure what there is to discuss, in reality. I setup 2 bgp vm's. In one vm I have 25k network statements with a 'no bgp network import-check'. In the receiving VM( with vnc turned on during compile ) I have nothing but a connection. I then flapped the link between the two vm's: With no debugs turned on in the vnc vm: sharpd@robot /v/l/quagga> wc bgpd.log 275011 2350135 26505153 bgpd.log sharpd@robot /v/l/quagga> grep vnc_ bgpd.log | wc 275000 2350000 26504104 With no debugs turned on in the non-vnc vm: root@s1:/var/log/quagga# grep BGP bgpd.log | wc 36 394 3417 root@s1:/var/log/quagga# donald On Mon, Dec 5, 2016 at 8:55 AM, Lou Berger <lberger@labn.net> wrote:

Donald,

On 12/5/2016 8:46 AM, Donald Sharp wrote:

...

I'm also attempting to argue( quite poorly apparently) that let's just remove the debugs until such time that the appropriate 'debug bgp vnc' code has been added in. They are filling up my logs and making it hard for me to find real issues that I am looking at.

My proposal: Take this patch In the future when the appropriate debug guards have been coded, revert the patch and apply debug guard patch on top of it. so why not just run this locally for now while we discuss and agree on a general solution?

I think just taking a patch that changes conventions / removes someone else's code because one person objects is a bad precedent and not in the spirit of this project.

Lou

...

donald

On Mon, Dec 5, 2016 at 8:42 AM, David Lamparter <david@opensourcerouting.org> wrote:

...

Donald & Lou,

I think there's a communication failure here... here's what I understand:

Donald is arguing that there should be a "debug vnc" CLI command (or possibly more than one such command) which gates all of these debug messages. (I agree with that.)

Lou is arguing that the debug statements are useful and should remain in the code. (I agree with that, too - though some of them are a bit excessive IMHO and create logspam.)

So the result is that we need to add a "debug vnc" command and put these zlog_debug calls under an "if (debug ...)".

Lou is also right that we: - need to document this somewhere, i.e. that there's "debug XYZ" which should control which parts of the code you get debug logs from - maybe can improve the way this is handled (because adding "debug XYZ" is manual and open-coded everywhere, this could be better.)

Cheers,

-David

P.S.: Mechanically adding the "if ()" everywhere can probably be done with coccinelle [http://coccinelle.lip6.fr/docs/index.html]

On Mon, Dec 05, 2016 at 08:29:14AM -0500, Lou Berger wrote:

...

Donald,

I'm not opposed to adding 'guarded debug' or submitting such a patch, but I do think this is something to be agreed upon and something to not catch other developers by surprise.

Are "Guarded debug" defined/covered in any existing developer docs? If so, where? -- and I apologize for missing it. If not, please submit a patch showing the definition and conventions you'd like developers (i.e. us) to follow -- and assuming no one objects, we'll follow this.

Without such a definition - we/others are just guessing at what one person finds acceptable.

Lou

On 12/5/2016 7:54 AM, Donald Sharp wrote:

...

Guarded debug - A debug that must be turned on by a cli command, ex 'debug bgp underpants' would turn on underpants bgp debugging . I am constantly hearing from internal test as well as actual customer cases when we make the mistake of not guarding a debug.. I believe that they are right and we must think carefully about what is displayed in the log when it is under normal operations( no debugs ).

I stand by this patch, it removes unguarded debugs. If you feel that it is in error, I welcome a patch that appropriately addresses this issue to your satisfaction.

donald

On Sun, Dec 4, 2016 at 11:02 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

On 12/4/2016 9:16 AM, Donald Sharp wrote: > Look, I agree debugs are needed but most of what I removed were > unguarded debugs. Please elaborate on what a "guarded debug" looks like to you -- do you mean controlled by a debug ... statement, is if something that is conditionally compiled? Please point to an example of what you'd like to see. The former is certainly a reasonable change. > These will simply just fill up the debug log with > useless cruft that makes it harder to actually debug issues because of > unnecessary data. Unnecessary to whom? If I'm debugging something related, you've made it impossible and also made it a lot of work to readd these statements.

> If you need these debugs then submit a patch so > that they can be turned on appropriately. We though the code was appropriate, please define what you think is appropriate. Perhaps there's something to be added to HACKING.md to capture this view.

> I submit though that debugs > that say 'entering function', 'exiting function' don't do allot to > help you figure issues out. While that is certainly one perspective, it isn't one shared by the developer of that code.

Lou

> donald > > On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote: >> Donald, >> >> Huh? Debug logs are there for when debugging is needed - and that's what >> these are all about. Simply removing them is wrong and make others lives >> much more difficult (ours) when debugging. >> >> What logs are filling? (I.e Who normally runs with debug logging?) >> >> What type of "guards" would you like to see? We/you certain can add a >> debug_bgp_vnc flag if that's what you're thinking. >> >> I really object to the summary removal of these debug log statements. >> >> Lou >> >> >> >> On December 3, 2016 10:40:12 PM Donald Sharp <sharpd@cumulusnetworks.com> >> wrote: >> >>> Remove allot of unguarded debugs in bgp >>> that are causing the log file to be filed >>> up. >>> >>> Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> >>> --- >>> bgpd/bgp_routemap.c | 2 -- >>> bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- >>> bgpd/rfapi/rfapi_import.c | 24 ----------------------- >>> bgpd/rfapi/rfapi_monitor.c | 4 ---- >>> bgpd/rfapi/rfapi_rib.c | 10 ---------- >>> bgpd/rfapi/rfapi_vty.c | 46 >>> --------------------------------------------- >>> bgpd/rfapi/vnc_export_bgp.c | 14 -------------- >>> bgpd/rfapi/vnc_import_bgp.c | 2 -- >>> bgpd/rfapi/vnc_zebra.c | 11 ----------- >>> 9 files changed, 120 deletions(-) >>> >>> diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c >>> index ea42cb5..88a169a 100644 >>> --- a/bgpd/bgp_routemap.c >>> +++ b/bgpd/bgp_routemap.c >>> @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) >>> bgp_route_map_process_update(bgp, rmap_name, 1); >>> >>> #if ENABLE_BGP_VNC >>> - zlog_debug("%s: calling vnc_routemap_update", __func__); >>> vnc_routemap_update(bgp, __func__); >>> #endif >>> return 0; >>> @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) >>> for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) >>> bgp_route_map_process_update(bgp, rmap_name, 0); >>> #if ENABLE_BGP_VNC >>> - zlog_debug("%s: calling vnc_routemap_update", __func__); >>> vnc_routemap_update(bgp, __func__); >>> #endif >>> } >>> diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c >>> index d064c50..a0d80d6 100644 >>> --- a/bgpd/rfapi/bgp_rfapi_cfg.c >>> +++ b/bgpd/rfapi/bgp_rfapi_cfg.c >>> @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) >>> afi_t afi; >>> int type; >>> >>> - zlog_debug ("%s: entry", __func__); >>> memset (redist_was_enabled, 0, sizeof (redist_was_enabled)); >>> >>> /* >>> @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) >>> } >>> } >>> } >>> - zlog_debug ("%s: return", __func__); >>> } >>> >>> static void >>> @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) >>> afi_t afi; >>> int type; >>> >>> - zlog_debug ("%s: entry", __func__); >>> /* >>> * If we turned off redistribution above, turn it back on. Doing so >>> * will tell zebra to resend the routes to us >>> @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) >>> } >>> } >>> } >>> - zlog_debug ("%s: return", __func__); >>> } >>> >>> DEFUN (vnc_redistribute_rh_roo_localadmin, >>> @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char >>> *unused) >>> struct rfapi_cfg *hc; >>> int i; >>> >>> - zlog_debug ("%s(arg=%s)", __func__, unused); >>> - >>> if (!bgp) >>> { >>> zlog_debug ("%s: No BGP process is configured", __func__); >>> @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char >>> *unused) >>> vnc_redistribute_prechange (bgp); >>> vnc_redistribute_postchange (bgp); >>> >>> - zlog_debug ("%s done", __func__); >>> } >>> >>> static void >>> diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c >>> index 77da4f9..54643ee 100644 >>> --- a/bgpd/rfapi/rfapi_import.c >>> +++ b/bgpd/rfapi/rfapi_import.c >>> @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( >>> struct rfapi_next_hop_entry *new; >>> int have_vnc_tunnel_un = 0; >>> >>> -#if DEBUG_ENCAP_MONITOR >>> - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); >>> -#endif >>> - >>> new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct >>> rfapi_next_hop_entry)); >>> assert (new); >>> >>> @@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry ( >>> >>> new->un_options = rfapi_encap_tlv_to_un_option (bi->attr); >>> >>> -#if DEBUG_ENCAP_MONITOR >>> - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", >>> - __func__, __LINE__, have_vnc_tunnel_un); >>> -#endif >>> - >>> if (!have_vnc_tunnel_un && bi && bi->extra) >>> { >>> /* >>> @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( >>> } >>> } >>> >>> - zlog_debug ("%s: returning %d routes", __func__, count); >>> return biglist; >>> } >>> >>> @@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( >>> } >>> } >>> >>> - zlog_debug ("%s: returning %d routes", __func__, count); >>> return biglist; >>> } >>> >>> @@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( >>> break; >>> } >>> >>> - zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, >>> - action_str, >>> - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), >>> p->prefixlen); >>> - >>> memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); >>> memset (&p_firstbi_new, 0, sizeof (p_firstbi_new)); >>> >>> @@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( >>> if (import_table == bgp->rfapi->it_ce) >>> is_it_ce = 1; >>> >>> - zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, >>> - (is_it_ce ? "CE-IT " : ""), >>> - action_str, >>> - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), >>> - p->prefixlen, import_table, afi2str (afi)); >>> - >>> VNC_ITRCCK; >>> >>> /* >>> @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( >>> buf_pfx[0] = '*'; >>> buf_pfx[1] = 0; >>> } >>> - >>> - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", >>> - __func__, buf_pfx, delete_active, delete_holddown); >>> } >>> #endif >>> >>> diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c >>> index 216b45e..8e33530 100644 >>> --- a/bgpd/rfapi/rfapi_monitor.c >>> +++ b/bgpd/rfapi/rfapi_monitor.c >>> @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) >>> struct bgp *bgp; >>> int count = 0; >>> >>> - zlog_debug ("%s: entry rfd=%p", __func__, rfd); >>> - >>> bgp = bgp_get_default (); >>> >>> if (rfd->mon) >>> @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( >>> struct rfapi_monitor_eth mon_buf; >>> int rc; >>> >>> - zlog_debug ("%s: entry rfd=%p", __func__, rfd); >>> - >>> assert (rfd->mon_eth); >>> >>> memset ((void *) &mon_buf, 0, sizeof (mon_buf)); >>> diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c >>> index 896b5f5..40d00ca 100644 >>> --- a/bgpd/rfapi/rfapi_rib.c >>> +++ b/bgpd/rfapi/rfapi_rib.c >>> @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( >>> int count = 0; >>> char buf[BUFSIZ]; >>> >>> - zlog_debug ("%s: entry", __func__); >>> - >>> if (CHECK_FLAG (bgp->rfapi_cfg->flags, >>> BGP_VNC_CONFIG_CALLBACK_DISABLE)) >>> return; >>> >>> @@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( >>> pn = route_node_get (rfd->rib_pending[afi], prefix); >>> assert (pn); >>> >>> - zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, >>> - pn->aggregate); >>> - >>> if (pn->aggregate) >>> { >>> /* >>> @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( >>> char buf[BUFSIZ]; >>> >>> prefix2str (&it_node->p, buf, BUFSIZ); >>> - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", >>> - __func__, it, afi, it_node, buf); >>> >>> if (AFI_ETHER == afi) >>> { >>> @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( >>> */ >>> if ((sl = RFAPI_MONITOR_ETH (it_node))) >>> { >>> - >>> - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); >>> - >>> for (cursor = NULL, rc = >>> skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); >>> !rc; >>> rc = skiplist_next (sl, NULL, (void **) &m, (void **) >>> &cursor)) >>> diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c >>> index c198564..1f25c72 100644 >>> --- a/bgpd/rfapi/rfapi_vty.c >>> +++ b/bgpd/rfapi/rfapi_vty.c >>> @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> struct rfapi_next_hop_entry *tail = NULL; >>> struct rfapi_cfg *rfapi_cfg; >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: entry", __func__); >>> -#endif >>> - >>> if (!bgp_default) >>> return ENXIO; >>> >>> @@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> rfapiQprefix2Rprefix (pPrefix, &rprefix); >>> } >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: starting descriptor loop", __func__); >>> -#endif >>> - >>> for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) >>> { >>> struct rfapi_adb *adb; >>> @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> struct nve_addr ha; >>> struct nve_addr *hap; >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: rfd=%p", __func__, rfd); >>> -#endif >>> - >>> /* >>> * match un, vn addresses of NVEs >>> */ >>> @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) >>> continue; >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: un, vn match", __func__); >>> -#endif >>> - >>> /* >>> * match prefix >>> */ >>> @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> { >>> if (!prefix_same (pPrefix, &adb->prefix_ip)) >>> { >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: adb=%p, prefix doesn't match, >>> skipping", >>> - __func__, adb); >>> -#endif >>> continue; >>> } >>> } >>> @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> (cda->l2o.o.macaddr.octet, >>> adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) >>> { >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: adb=%p, macaddr doesn't match, >>> skipping", >>> - __func__, adb); >>> -#endif >>> continue; >>> } >>> } >>> @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> { >>> if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) >>> { >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: adb=%p, LNI doesn't match, >>> skipping", >>> - __func__, adb); >>> -#endif >>> continue; >>> } >>> } >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, >>> - adb); >>> -#endif >>> - >>> listnode_add (adb_delete_list, adb); >>> } >>> >>> @@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> pVn = NULL; >>> } >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, >>> adb); >>> -#endif >>> - >>> rc = rfapi_register (rfd, &rp, 0, NULL, pVn, >>> RFAPI_REGISTER_KILL); >>> if (!rc) >>> { >>> @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> continue; >>> } >>> } >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: ip0 adding adb %p to delete list", >>> - __func__, adb); >>> -#endif >>> listnode_add (adb_delete_list, adb); >>> } >>> >>> @@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct >>> rfapi_local_reg_delete_arg *cda) >>> vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; >>> vn.v.l2addr = adb->l2o; >>> >>> -#if DEBUG_L2_EXTRA >>> - zlog_debug ("%s: ip0 killing reg from adb %p ", >>> - __func__, adb); >>> -#endif >>> - >>> rc = rfapi_register (rfd, &rp, 0, NULL, &vn, >>> RFAPI_REGISTER_KILL); >>> if (!rc) >>> diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c >>> index 6434c37..f1a9ecb 100644 >>> --- a/bgpd/rfapi/vnc_export_bgp.c >>> +++ b/bgpd/rfapi/vnc_export_bgp.c >>> @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t >>> afi) >>> struct route_node *rn; >>> struct bgp_info *ri; >>> >>> - zlog_debug ("%s: entry, afi=%d", __func__, afi); >>> - >>> if (!bgp) >>> return; >>> >>> @@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t >>> afi) >>> { >>> struct bgp_node *rn; >>> >>> - zlog_debug ("%s: entry, afi=%d", __func__, afi); >>> - >>> if (!bgp) >>> return; >>> >>> @@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( >>> struct attr attr = { 0 }; >>> struct rfapi_import_table *import_table; >>> >>> - zlog_debug ("%s: entry", __func__); >>> - >>> import_table = rfg->rfapi_import_table; >>> if (!import_table) >>> { >>> @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( >>> struct route_node *rn; >>> struct rfapi_import_table *import_table; >>> >>> - zlog_debug ("%s: entry", __func__); >>> - >>> import_table = rfg->rfapi_import_table; >>> if (!import_table) >>> { >>> @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t >>> afi) >>> struct rfapi_import_table *it; >>> uint8_t family = afi2family (afi); >>> >>> - zlog_debug ("%s: entry, afi=%d", __func__, afi); >>> - >>> if (!bgp) >>> return; >>> >>> @@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t >>> afi) >>> struct bgp_node *prn; >>> struct rfapi_cfg *hc; >>> >>> - zlog_debug ("%s: entry, afi=%d", __func__, afi); >>> - >>> if (!bgp) >>> return; >>> >>> @@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, >>> afi_t afi) >>> { >>> struct bgp_node *rn; >>> >>> - zlog_debug ("%s: entry, afi=%d", __func__, afi); >>> - >>> if (!bgp) >>> return; >>> >>> diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c >>> index dc2640a..0ba927d 100644 >>> --- a/bgpd/rfapi/vnc_import_bgp.c >>> +++ b/bgpd/rfapi/vnc_import_bgp.c >>> @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, >>> afi_t afi) >>> } >>> >>> bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; >>> - zlog_debug ("%s: return", __func__); >>> } >>> >>> >>> @@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp >>> *bgp, afi_t afi) >>> } >>> >>> bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; >>> - zlog_debug ("%s: return", __func__); >>> } >>> diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c >>> index e357ef6..bc77ca4 100644 >>> --- a/bgpd/rfapi/vnc_zebra.c >>> +++ b/bgpd/rfapi/vnc_zebra.c >>> @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, >>> uint8_t type) >>> struct bgp_node *prn; >>> struct bgp_node *rn; >>> >>> - zlog_debug ("%s: entry", __func__); >>> - >>> if (!bgp) >>> return; >>> if (!bgp->rfapi_cfg) >>> @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, >>> uint8_t type) >>> } >>> } >>> } >>> - zlog_debug ("%s: return", __func__); >>> } >>> >>> /* >>> @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( >>> void *nh_ary = NULL; >>> void *nhp_ary = NULL; >>> >>> - zlog_debug ("%s: entry, add=%d", __func__, add); >>> - >>> if (zclient_vnc->sock < 0) >>> return; >>> >>> @@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( >>> // struct prefix *nhpp; >>> void *pAddr; >>> >>> - zlog_debug ("%s: entry, add=%d", __func__, add); >>> - >>> if (zclient_vnc->sock < 0) >>> return; >>> >>> @@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( >>> void *nh_ary = NULL; >>> void *nhp_ary = NULL; >>> >>> - zlog_debug ("%s: entry", __func__); >>> import_table = rfg->rfapi_import_table; >>> if (!import_table) >>> { >>> @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct >>> rfapi_nve_group_cfg *rfg) >>> void >>> vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) >>> { >>> - zlog_debug ("%s: entry", __func__); >>> vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); >>> vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); >>> } >>> @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, >>> int type) >>> /* Withdraw redistributed routes from current BGP's routing table. */ >>> vnc_redistribute_withdraw (bgp, afi, type); >>> >>> - zlog_debug ("%s: return", __func__); >>> - >>> return CMD_SUCCESS; >>> } >>> >>> -- >>> 2.7.4 >>> >>> >>> _______________________________________________ >>> cmaster-next mailing list >>> cmaster-next@lists.nox.tf >>> https://lists.nox.tf/listinfo/cmaster-next

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next

David, Thanks for this. On 12/5/2016 8:42 AM, David Lamparter wrote:

Donald & Lou,

I think there's a communication failure here... here's what I understand:

Donald is arguing that there should be a "debug vnc" CLI command (or possibly more than one such command) which gates all of these debug messages. (I agree with that.) Actually donald just submitted a patch that removed a slew of zlog_debug statements.

I too would agree with the above point -- had it been made more generally.

Lou is arguing that the debug statements are useful and should remain in the code. (I agree with that, too - though some of them are a bit excessive IMHO and create logspam.)

So the result is that we need to add a "debug vnc" command and put these zlog_debug calls under an "if (debug ...)".

Lou is also right that we: - need to document this somewhere, i.e. that there's "debug XYZ" which should control which parts of the code you get debug logs from

- maybe can improve the way this is handled (because adding "debug XYZ" is manual and open-coded everywhere, this could be better.) I was thinking there would be a macro based way to facilitate this, e.g., zlog_debug(<debug_term>,...) or zlog_debug_<debug_term>(...)

The first forces all log statements to change, the second allows for "unguarded debugs" but is easy transition to... Lou

Cheers,

-David

P.S.: Mechanically adding the "if ()" everywhere can probably be done with coccinelle [http://coccinelle.lip6.fr/docs/index.html]

On Mon, Dec 05, 2016 at 08:29:14AM -0500, Lou Berger wrote:

...

Donald,

I'm not opposed to adding 'guarded debug' or submitting such a patch, but I do think this is something to be agreed upon and something to not catch other developers by surprise.

Are "Guarded debug" defined/covered in any existing developer docs? If so, where? -- and I apologize for missing it. If not, please submit a patch showing the definition and conventions you'd like developers (i.e. us) to follow -- and assuming no one objects, we'll follow this.

Without such a definition - we/others are just guessing at what one person finds acceptable.

Lou

On 12/5/2016 7:54 AM, Donald Sharp wrote:

...

Guarded debug - A debug that must be turned on by a cli command, ex 'debug bgp underpants' would turn on underpants bgp debugging . I am constantly hearing from internal test as well as actual customer cases when we make the mistake of not guarding a debug.. I believe that they are right and we must think carefully about what is displayed in the log when it is under normal operations( no debugs ).

I stand by this patch, it removes unguarded debugs. If you feel that it is in error, I welcome a patch that appropriately addresses this issue to your satisfaction.

donald

On Sun, Dec 4, 2016 at 11:02 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

On 12/4/2016 9:16 AM, Donald Sharp wrote:

...

Look, I agree debugs are needed but most of what I removed were unguarded debugs. Please elaborate on what a "guarded debug" looks like to you -- do you mean controlled by a debug ... statement, is if something that is conditionally compiled? Please point to an example of what you'd like to see. The former is certainly a reasonable change. These will simply just fill up the debug log with useless cruft that makes it harder to actually debug issues because of unnecessary data. Unnecessary to whom? If I'm debugging something related, you've made it impossible and also made it a lot of work to readd these statements.

...

If you need these debugs then submit a patch so that they can be turned on appropriately. We though the code was appropriate, please define what you think is appropriate. Perhaps there's something to be added to HACKING.md to capture this view.

...

I submit though that debugs that say 'entering function', 'exiting function' don't do allot to help you figure issues out. While that is certainly one perspective, it isn't one shared by the developer of that code.

Lou

...

donald

On Sun, Dec 4, 2016 at 7:43 AM, Lou Berger <lberger@labn.net> wrote:

...

Donald,

Huh? Debug logs are there for when debugging is needed - and that's what these are all about. Simply removing them is wrong and make others lives much more difficult (ours) when debugging.

What logs are filling? (I.e Who normally runs with debug logging?)

What type of "guards" would you like to see? We/you certain can add a debug_bgp_vnc flag if that's what you're thinking.

I really object to the summary removal of these debug log statements.

Lou

On December 3, 2016 10:40:12 PM Donald Sharp <sharpd@cumulusnetworks.com> wrote:

> Remove allot of unguarded debugs in bgp > that are causing the log file to be filed > up. > > Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com> > --- > bgpd/bgp_routemap.c | 2 -- > bgpd/rfapi/bgp_rfapi_cfg.c | 7 ------- > bgpd/rfapi/rfapi_import.c | 24 ----------------------- > bgpd/rfapi/rfapi_monitor.c | 4 ---- > bgpd/rfapi/rfapi_rib.c | 10 ---------- > bgpd/rfapi/rfapi_vty.c | 46 > --------------------------------------------- > bgpd/rfapi/vnc_export_bgp.c | 14 -------------- > bgpd/rfapi/vnc_import_bgp.c | 2 -- > bgpd/rfapi/vnc_zebra.c | 11 ----------- > 9 files changed, 120 deletions(-) > > diff --git a/bgpd/bgp_routemap.c b/bgpd/bgp_routemap.c > index ea42cb5..88a169a 100644 > --- a/bgpd/bgp_routemap.c > +++ b/bgpd/bgp_routemap.c > @@ -2853,7 +2853,6 @@ bgp_route_map_process_update_cb (char *rmap_name) > bgp_route_map_process_update(bgp, rmap_name, 1); > > #if ENABLE_BGP_VNC > - zlog_debug("%s: calling vnc_routemap_update", __func__); > vnc_routemap_update(bgp, __func__); > #endif > return 0; > @@ -2893,7 +2892,6 @@ bgp_route_map_mark_update (const char *rmap_name) > for (ALL_LIST_ELEMENTS (bm->bgp, node, nnode, bgp)) > bgp_route_map_process_update(bgp, rmap_name, 0); > #if ENABLE_BGP_VNC > - zlog_debug("%s: calling vnc_routemap_update", __func__); > vnc_routemap_update(bgp, __func__); > #endif > } > diff --git a/bgpd/rfapi/bgp_rfapi_cfg.c b/bgpd/rfapi/bgp_rfapi_cfg.c > index d064c50..a0d80d6 100644 > --- a/bgpd/rfapi/bgp_rfapi_cfg.c > +++ b/bgpd/rfapi/bgp_rfapi_cfg.c > @@ -774,7 +774,6 @@ vnc_redistribute_prechange (struct bgp *bgp) > afi_t afi; > int type; > > - zlog_debug ("%s: entry", __func__); > memset (redist_was_enabled, 0, sizeof (redist_was_enabled)); > > /* > @@ -794,7 +793,6 @@ vnc_redistribute_prechange (struct bgp *bgp) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > static void > @@ -803,7 +801,6 @@ vnc_redistribute_postchange (struct bgp *bgp) > afi_t afi; > int type; > > - zlog_debug ("%s: entry", __func__); > /* > * If we turned off redistribution above, turn it back on. Doing so > * will tell zebra to resend the routes to us > @@ -818,7 +815,6 @@ vnc_redistribute_postchange (struct bgp *bgp) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > DEFUN (vnc_redistribute_rh_roo_localadmin, > @@ -2498,8 +2494,6 @@ vnc_routemap_update (struct bgp *bgp, const char > *unused) > struct rfapi_cfg *hc; > int i; > > - zlog_debug ("%s(arg=%s)", __func__, unused); > - > if (!bgp) > { > zlog_debug ("%s: No BGP process is configured", __func__); > @@ -2573,7 +2567,6 @@ vnc_routemap_update (struct bgp *bgp, const char > *unused) > vnc_redistribute_prechange (bgp); > vnc_redistribute_postchange (bgp); > > - zlog_debug ("%s done", __func__); > } > > static void > diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c > index 77da4f9..54643ee 100644 > --- a/bgpd/rfapi/rfapi_import.c > +++ b/bgpd/rfapi/rfapi_import.c > @@ -1349,10 +1349,6 @@ rfapiRouteInfo2NextHopEntry ( > struct rfapi_next_hop_entry *new; > int have_vnc_tunnel_un = 0; > > -#if DEBUG_ENCAP_MONITOR > - zlog_debug ("%s: entry, bi %p, rn %p", __func__, bi, rn); > -#endif > - > new = XCALLOC (MTYPE_RFAPI_NEXTHOP, sizeof (struct > rfapi_next_hop_entry)); > assert (new); > > @@ -1500,11 +1496,6 @@ rfapiRouteInfo2NextHopEntry ( > > new->un_options = rfapi_encap_tlv_to_un_option (bi->attr); > > -#if DEBUG_ENCAP_MONITOR > - zlog_debug ("%s: line %d: have_vnc_tunnel_un=%d", > - __func__, __LINE__, have_vnc_tunnel_un); > -#endif > - > if (!have_vnc_tunnel_un && bi && bi->extra) > { > /* > @@ -1960,7 +1951,6 @@ rfapiRouteTable2NextHopList ( > } > } > > - zlog_debug ("%s: returning %d routes", __func__, count); > return biglist; > } > > @@ -2055,7 +2045,6 @@ rfapiEthRouteTable2NextHopList ( > } > } > > - zlog_debug ("%s: returning %d routes", __func__, count); > return biglist; > } > > @@ -3117,10 +3106,6 @@ rfapiBgpInfoFilteredImportEncap ( > break; > } > > - zlog_debug ("%s: entry: %s: prefix %s/%d", __func__, > - action_str, > - inet_ntop (p->family, &p->u.prefix, buf, BUFSIZ), > p->prefixlen); > - > memset (&p_firstbi_old, 0, sizeof (p_firstbi_old)); > memset (&p_firstbi_new, 0, sizeof (p_firstbi_new)); > > @@ -3598,12 +3583,6 @@ rfapiBgpInfoFilteredImportVPN ( > if (import_table == bgp->rfapi->it_ce) > is_it_ce = 1; > > - zlog_debug ("%s: entry: %s%s: prefix %s/%d: it %p, afi %s", __func__, > - (is_it_ce ? "CE-IT " : ""), > - action_str, > - rfapi_ntop (p->family, &p->u.prefix, buf, BUFSIZ), > - p->prefixlen, import_table, afi2str (afi)); > - > VNC_ITRCCK; > > /* > @@ -4695,9 +4674,6 @@ rfapiDeleteRemotePrefixesIt ( > buf_pfx[0] = '*'; > buf_pfx[1] = 0; > } > - > - zlog_debug ("%s: entry, p=%s, delete_active=%d, delete_holddown=%d", > - __func__, buf_pfx, delete_active, delete_holddown); > } > #endif > > diff --git a/bgpd/rfapi/rfapi_monitor.c b/bgpd/rfapi/rfapi_monitor.c > index 216b45e..8e33530 100644 > --- a/bgpd/rfapi/rfapi_monitor.c > +++ b/bgpd/rfapi/rfapi_monitor.c > @@ -707,8 +707,6 @@ rfapiMonitorDelHd (struct rfapi_descriptor *rfd) > struct bgp *bgp; > int count = 0; > > - zlog_debug ("%s: entry rfd=%p", __func__, rfd); > - > bgp = bgp_get_default (); > > if (rfd->mon) > @@ -1511,8 +1509,6 @@ rfapiMonitorEthDel ( > struct rfapi_monitor_eth mon_buf; > int rc; > > - zlog_debug ("%s: entry rfd=%p", __func__, rfd); > - > assert (rfd->mon_eth); > > memset ((void *) &mon_buf, 0, sizeof (mon_buf)); > diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c > index 896b5f5..40d00ca 100644 > --- a/bgpd/rfapi/rfapi_rib.c > +++ b/bgpd/rfapi/rfapi_rib.c > @@ -1602,8 +1602,6 @@ rfapiRibUpdatePendingNode ( > int count = 0; > char buf[BUFSIZ]; > > - zlog_debug ("%s: entry", __func__); > - > if (CHECK_FLAG (bgp->rfapi_cfg->flags, > BGP_VNC_CONFIG_CALLBACK_DISABLE)) > return; > > @@ -1619,9 +1617,6 @@ rfapiRibUpdatePendingNode ( > pn = route_node_get (rfd->rib_pending[afi], prefix); > assert (pn); > > - zlog_debug ("%s: pn->info=%p, pn->aggregate=%p", __func__, pn->info, > - pn->aggregate); > - > if (pn->aggregate) > { > /* > @@ -2155,8 +2150,6 @@ rfapiRibPendingDeleteRoute ( > char buf[BUFSIZ]; > > prefix2str (&it_node->p, buf, BUFSIZ); > - zlog_debug ("%s: entry, it=%p, afi=%d, it_node=%p, pfx=%s", > - __func__, it, afi, it_node, buf); > > if (AFI_ETHER == afi) > { > @@ -2175,9 +2168,6 @@ rfapiRibPendingDeleteRoute ( > */ > if ((sl = RFAPI_MONITOR_ETH (it_node))) > { > - > - zlog_debug ("%s: route-specific skiplist: %p", __func__, sl); > - > for (cursor = NULL, rc = > skiplist_next (sl, NULL, (void **) &m, (void **) &cursor); > !rc; > rc = skiplist_next (sl, NULL, (void **) &m, (void **) > &cursor)) > diff --git a/bgpd/rfapi/rfapi_vty.c b/bgpd/rfapi/rfapi_vty.c > index c198564..1f25c72 100644 > --- a/bgpd/rfapi/rfapi_vty.c > +++ b/bgpd/rfapi/rfapi_vty.c > @@ -3283,10 +3283,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > struct rfapi_next_hop_entry *tail = NULL; > struct rfapi_cfg *rfapi_cfg; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: entry", __func__); > -#endif > - > if (!bgp_default) > return ENXIO; > > @@ -3305,10 +3301,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > rfapiQprefix2Rprefix (pPrefix, &rprefix); > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: starting descriptor loop", __func__); > -#endif > - > for (ALL_LIST_ELEMENTS_RO (&h->descriptors, node, rfd)) > { > struct rfapi_adb *adb; > @@ -3317,10 +3309,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > struct nve_addr ha; > struct nve_addr *hap; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: rfd=%p", __func__, rfd); > -#endif > - > /* > * match un, vn addresses of NVEs > */ > @@ -3329,10 +3317,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > if (pVn && (rfapi_ip_addr_cmp (pVn, &rfd->vn_addr))) > continue; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: un, vn match", __func__); > -#endif > - > /* > * match prefix > */ > @@ -3372,10 +3356,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > { > if (!prefix_same (pPrefix, &adb->prefix_ip)) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, prefix doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > @@ -3385,10 +3365,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > (cda->l2o.o.macaddr.octet, > adb->prefix_eth.u.prefix_eth.octet, ETHER_ADDR_LEN)) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, macaddr doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > @@ -3397,19 +3373,10 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > { > if (cda->l2o.o.logical_net_id != adb->l2o.logical_net_id) > { > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: adb=%p, LNI doesn't match, > skipping", > - __func__, adb); > -#endif > continue; > } > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ipN adding adb %p to delete list", __func__, > - adb); > -#endif > - > listnode_add (adb_delete_list, adb); > } > > @@ -3454,10 +3421,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > pVn = NULL; > } > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ipN killing reg from adb %p ", __func__, > adb); > -#endif > - > rc = rfapi_register (rfd, &rp, 0, NULL, pVn, > RFAPI_REGISTER_KILL); > if (!rc) > { > @@ -3508,10 +3471,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > continue; > } > } > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ip0 adding adb %p to delete list", > - __func__, adb); > -#endif > listnode_add (adb_delete_list, adb); > } > > @@ -3527,11 +3486,6 @@ rfapiDeleteLocalPrefixes (struct > rfapi_local_reg_delete_arg *cda) > vn.type = RFAPI_VN_OPTION_TYPE_L2ADDR; > vn.v.l2addr = adb->l2o; > > -#if DEBUG_L2_EXTRA > - zlog_debug ("%s: ip0 killing reg from adb %p ", > - __func__, adb); > -#endif > - > rc = rfapi_register (rfd, &rp, 0, NULL, &vn, > RFAPI_REGISTER_KILL); > if (!rc) > diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c > index 6434c37..f1a9ecb 100644 > --- a/bgpd/rfapi/vnc_export_bgp.c > +++ b/bgpd/rfapi/vnc_export_bgp.c > @@ -437,8 +437,6 @@ vnc_direct_bgp_vpn_enable_ce (struct bgp *bgp, afi_t > afi) > struct route_node *rn; > struct bgp_info *ri; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -499,8 +497,6 @@ vnc_direct_bgp_vpn_disable_ce (struct bgp *bgp, afi_t > afi) > { > struct bgp_node *rn; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -1276,8 +1272,6 @@ vnc_direct_bgp_add_group_afi ( > struct attr attr = { 0 }; > struct rfapi_import_table *import_table; > > - zlog_debug ("%s: entry", __func__); > - > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -1417,8 +1411,6 @@ vnc_direct_bgp_del_group_afi ( > struct route_node *rn; > struct rfapi_import_table *import_table; > > - zlog_debug ("%s: entry", __func__); > - > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -1626,8 +1618,6 @@ vnc_direct_bgp_vpn_disable (struct bgp *bgp, afi_t > afi) > struct rfapi_import_table *it; > uint8_t family = afi2family (afi); > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -1873,8 +1863,6 @@ vnc_direct_bgp_rh_vpn_enable (struct bgp *bgp, afi_t > afi) > struct bgp_node *prn; > struct rfapi_cfg *hc; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > @@ -2034,8 +2022,6 @@ vnc_direct_bgp_rh_vpn_disable (struct bgp *bgp, > afi_t afi) > { > struct bgp_node *rn; > > - zlog_debug ("%s: entry, afi=%d", __func__, afi); > - > if (!bgp) > return; > > diff --git a/bgpd/rfapi/vnc_import_bgp.c b/bgpd/rfapi/vnc_import_bgp.c > index dc2640a..0ba927d 100644 > --- a/bgpd/rfapi/vnc_import_bgp.c > +++ b/bgpd/rfapi/vnc_import_bgp.c > @@ -3097,7 +3097,6 @@ vnc_import_bgp_redist_disable (struct bgp *bgp, > afi_t afi) > } > > bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT] = 0; > - zlog_debug ("%s: return", __func__); > } > > > @@ -3145,5 +3144,4 @@ vnc_import_bgp_exterior_redist_disable (struct bgp > *bgp, afi_t afi) > } > > bgp->rfapi_cfg->redist[afi][ZEBRA_ROUTE_BGP_DIRECT_EXT] = 0; > - zlog_debug ("%s: return", __func__); > } > diff --git a/bgpd/rfapi/vnc_zebra.c b/bgpd/rfapi/vnc_zebra.c > index e357ef6..bc77ca4 100644 > --- a/bgpd/rfapi/vnc_zebra.c > +++ b/bgpd/rfapi/vnc_zebra.c > @@ -284,8 +284,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, > uint8_t type) > struct bgp_node *prn; > struct bgp_node *rn; > > - zlog_debug ("%s: entry", __func__); > - > if (!bgp) > return; > if (!bgp->rfapi_cfg) > @@ -331,7 +329,6 @@ vnc_redistribute_withdraw (struct bgp *bgp, afi_t afi, > uint8_t type) > } > } > } > - zlog_debug ("%s: return", __func__); > } > > /* > @@ -729,8 +726,6 @@ vnc_zebra_add_del_prefix ( > void *nh_ary = NULL; > void *nhp_ary = NULL; > > - zlog_debug ("%s: entry, add=%d", __func__, add); > - > if (zclient_vnc->sock < 0) > return; > > @@ -808,8 +803,6 @@ vnc_zebra_add_del_nve ( > // struct prefix *nhpp; > void *pAddr; > > - zlog_debug ("%s: entry, add=%d", __func__, add); > - > if (zclient_vnc->sock < 0) > return; > > @@ -909,7 +902,6 @@ vnc_zebra_add_del_group_afi ( > void *nh_ary = NULL; > void *nhp_ary = NULL; > > - zlog_debug ("%s: entry", __func__); > import_table = rfg->rfapi_import_table; > if (!import_table) > { > @@ -982,7 +974,6 @@ vnc_zebra_add_group (struct bgp *bgp, struct > rfapi_nve_group_cfg *rfg) > void > vnc_zebra_del_group (struct bgp *bgp, struct rfapi_nve_group_cfg *rfg) > { > - zlog_debug ("%s: entry", __func__); > vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP, 0); > vnc_zebra_add_del_group_afi (bgp, rfg, AFI_IP6, 0); > } > @@ -1083,8 +1074,6 @@ vnc_redistribute_unset (struct bgp *bgp, afi_t afi, > int type) > /* Withdraw redistributed routes from current BGP's routing table. */ > vnc_redistribute_withdraw (bgp, afi, type); > > - zlog_debug ("%s: return", __func__); > - > return CMD_SUCCESS; > } > > -- > 2.7.4 > > > _______________________________________________ > cmaster-next mailing list > cmaster-next@lists.nox.tf > https://lists.nox.tf/listinfo/cmaster-next

_______________________________________________ cmaster-next mailing list cmaster-next@lists.nox.tf https://lists.nox.tf/listinfo/cmaster-next