Re: [cmaster-next] [PATCH] bgpd: Remove unguarded debugs

4 Dec 2016

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


    