pim-msdp: CLI and debug cleanup

No functional change.

Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com>

7 files changed, 163 insertions, 168 deletions

diff --git a/pimd/pim_cmd.c b/pimd/pim_cmd.c
index ab16943431..ced7497e24 100644
--- a/pimd/pim_cmd.c
+++ b/pimd/pim_cmd.c
@@ -5632,7 +5632,7 @@ ip_msdp_show_peers(struct vty *vty, u_char uj)
   if (uj) {
     json = json_object_new_object();
   } else {
-    vty_out(vty, "Peer                       Local        Mesh-group        State    Uptime%s", VTY_NEWLINE);
+    vty_out(vty, "Peer                       Local        State    Uptime  SaCnt%s", VTY_NEWLINE);
   }
 
   for (ALL_LIST_ELEMENTS_RO(msdp->peer_list, mpnode, mp)) {
@@ -5649,14 +5649,14 @@ ip_msdp_show_peers(struct vty *vty, u_char uj)
       json_row = json_object_new_object();
       json_object_string_add(json_row, "peer", peer_str);
       json_object_string_add(json_row, "local", local_str);
-      json_object_string_add(json_row, "meshGroupName", mp->mesh_group_name);
       json_object_string_add(json_row, "state", state_str);
       json_object_string_add(json_row, "upTime", timebuf);
+      json_object_int_add(json_row, "saCount", mp->sa_cnt);
       json_object_object_add(json, peer_str, json_row);
     } else {
-      vty_out(vty, "%-15s  %15s  %16s  %11s  %8s%s",
-          peer_str, local_str, mp->mesh_group_name, state_str,
-          timebuf, VTY_NEWLINE);
+      vty_out(vty, "%-15s  %15s  %11s  %8s  %5d%s",
+          peer_str, local_str, state_str,
+          timebuf, mp->sa_cnt, VTY_NEWLINE);
     }
   }
 
@@ -5711,12 +5711,13 @@ ip_msdp_show_peers_detail(struct vty *vty, const char *peer, u_char uj)
       json_object_string_add(json_row, "meshGroupName", mp->mesh_group_name);
       json_object_string_add(json_row, "state", state_str);
       json_object_string_add(json_row, "upTime", timebuf);
-      json_object_string_add(json_row, "keepAliveTime", katimer);
-      json_object_string_add(json_row, "connRetryTime", crtimer);
-      json_object_string_add(json_row, "holdTime", holdtimer);
+      json_object_string_add(json_row, "keepAliveTimer", katimer);
+      json_object_string_add(json_row, "connRetryTimer", crtimer);
+      json_object_string_add(json_row, "holdTimer", holdtimer);
       json_object_string_add(json_row, "lastReset", mp->last_reset);
       json_object_int_add(json_row, "connAttempts", mp->conn_attempts);
-      json_object_int_add(json_row, "establishedFlaps", mp->est_flaps);
+      json_object_int_add(json_row, "establishedChanges", mp->est_flaps);
+      json_object_int_add(json_row, "saCount", mp->sa_cnt);
       json_object_int_add(json_row, "kaSent", mp->ka_tx_cnt);
       json_object_int_add(json_row, "kaRcvd", mp->ka_rx_cnt);
       json_object_int_add(json_row, "saSent", mp->sa_tx_cnt);
@@ -5724,18 +5725,19 @@ ip_msdp_show_peers_detail(struct vty *vty, const char *peer, u_char uj)
       json_object_object_add(json, peer_str, json_row);
     } else {
       vty_out(vty, "Peer : %s%s", peer_str, VTY_NEWLINE);
-      vty_out(vty, "  Local            : %s%s", local_str, VTY_NEWLINE);
-      vty_out(vty, "  Mesh Group       : %s%s", mp->mesh_group_name, VTY_NEWLINE);
-      vty_out(vty, "  State            : %s%s", state_str, VTY_NEWLINE);
-      vty_out(vty, "  Uptime           : %s%s", timebuf, VTY_NEWLINE);
-
-      vty_out(vty, "  Keepalive Time   : %s%s", katimer, VTY_NEWLINE);
-      vty_out(vty, "  Conn Retry Time  : %s%s", crtimer, VTY_NEWLINE);
-      vty_out(vty, "  Hold Time        : %s%s", holdtimer, VTY_NEWLINE);
-      vty_out(vty, "  Last Reset       : %s%s", mp->last_reset, VTY_NEWLINE);
-      vty_out(vty, "  Conn Attempts    : %d%s", mp->conn_attempts, VTY_NEWLINE);
-      vty_out(vty, "  Established Flaps: %d%s", mp->est_flaps, VTY_NEWLINE);
-      vty_out(vty, "  Statistics       :%s", VTY_NEWLINE);
+      vty_out(vty, "  Local               : %s%s", local_str, VTY_NEWLINE);
+      vty_out(vty, "  Mesh Group          : %s%s", mp->mesh_group_name, VTY_NEWLINE);
+      vty_out(vty, "  State               : %s%s", state_str, VTY_NEWLINE);
+      vty_out(vty, "  Uptime              : %s%s", timebuf, VTY_NEWLINE);
+
+      vty_out(vty, "  Keepalive Timer     : %s%s", katimer, VTY_NEWLINE);
+      vty_out(vty, "  Conn Retry Timer    : %s%s", crtimer, VTY_NEWLINE);
+      vty_out(vty, "  Hold Timer          : %s%s", holdtimer, VTY_NEWLINE);
+      vty_out(vty, "  Last Reset          : %s%s", mp->last_reset, VTY_NEWLINE);
+      vty_out(vty, "  Conn Attempts       : %d%s", mp->conn_attempts, VTY_NEWLINE);
+      vty_out(vty, "  Established Changes : %d%s", mp->est_flaps, VTY_NEWLINE);
+      vty_out(vty, "  SA Count            : %d%s", mp->sa_cnt, VTY_NEWLINE);
+      vty_out(vty, "  Statistics          :%s", VTY_NEWLINE);
       vty_out(vty, "                       Sent       Rcvd%s", VTY_NEWLINE);
       vty_out(vty, "    Keepalives : %10d %10d%s",
           mp->ka_tx_cnt, mp->ka_rx_cnt, VTY_NEWLINE);
@@ -5894,16 +5896,16 @@ ip_msdp_show_sa_entry_detail(struct pim_msdp_sa *sa, const char *src_str,
     json_object_string_add(json_row, "local", local_str);
     json_object_string_add(json_row, "sptSetup", spt_str);
     json_object_string_add(json_row, "upTime", timebuf);
-    json_object_string_add(json_row, "stateTime", statetimer);
+    json_object_string_add(json_row, "stateTimer", statetimer);
     json_object_object_add(json_group, src_str, json_row);
   } else {
     vty_out(vty, "SA : %s%s", sa->sg_str, VTY_NEWLINE);
-    vty_out(vty, "  RP         : %s%s", rp_str, VTY_NEWLINE);
-    vty_out(vty, "  Peer       : %s%s", peer_str, VTY_NEWLINE);
-    vty_out(vty, "  Local      : %s%s", local_str, VTY_NEWLINE);
-    vty_out(vty, "  SPT Setup  : %s%s", spt_str, VTY_NEWLINE);
-    vty_out(vty, "  Uptime     : %s%s", timebuf, VTY_NEWLINE);
-    vty_out(vty, "  State Time : %s%s", statetimer, VTY_NEWLINE);
+    vty_out(vty, "  RP          : %s%s", rp_str, VTY_NEWLINE);
+    vty_out(vty, "  Peer        : %s%s", peer_str, VTY_NEWLINE);
+    vty_out(vty, "  Local       : %s%s", local_str, VTY_NEWLINE);
+    vty_out(vty, "  SPT Setup   : %s%s", spt_str, VTY_NEWLINE);
+    vty_out(vty, "  Uptime      : %s%s", timebuf, VTY_NEWLINE);
+    vty_out(vty, "  State Timer : %s%s", statetimer, VTY_NEWLINE);
     vty_out(vty, "%s", VTY_NEWLINE);
   }
 }
diff --git a/pimd/pim_msdp.c b/pimd/pim_msdp.c
index e10289e90c..bc40352809 100644
--- a/pimd/pim_msdp.c
+++ b/pimd/pim_msdp.c
@@ -58,29 +58,10 @@ static void pim_msdp_mg_mbr_free(struct pim_msdp_mg_mbr *mbr);
 static void pim_msdp_mg_mbr_do_del(struct pim_msdp_mg *mg, struct pim_msdp_mg_mbr *mbr);
 
 /************************ SA cache management ******************************/
-char *
-pim_msdp_sa_key_dump(struct pim_msdp_sa *sa, char *buf, int buf_size, bool long_format)
-{
-  char rp_str[INET_ADDRSTRLEN];
-
-  if (long_format && (sa->flags & PIM_MSDP_SAF_PEER)) {
-    pim_inet4_dump("<rp?>", sa->rp, rp_str, sizeof(rp_str));
-    snprintf(buf, buf_size, "MSDP SA %s rp %s",
-	     sa->sg_str, rp_str);
-  } else {
-    snprintf(buf, buf_size, "MSDP SA %s", sa->sg_str);
-  }
-
-  return buf;
-}
-
 static void
 pim_msdp_sa_timer_expiry_log(struct pim_msdp_sa *sa, const char *timer_str)
 {
-  char key_str[PIM_MSDP_SA_KEY_STRLEN];
-
-  pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), false);
-  zlog_debug("%s %s timer expired", key_str, timer_str);
+  zlog_debug("MSDP SA %s %s timer expired", sa->sg_str, timer_str);
 }
 
 /* RFC-3618:Sec-5.1 - global active source advertisement timer */
@@ -88,7 +69,7 @@ static int
 pim_msdp_sa_adv_timer_cb(struct thread *t)
 {
   msdp->sa_adv_timer = NULL;
-  if (PIM_DEBUG_MSDP_INTERNAL) {
+  if (PIM_DEBUG_MSDP_EVENTS) {
     zlog_debug("MSDP SA advertisment timer expired");
   }
 
@@ -147,9 +128,7 @@ pim_msdp_sa_upstream_del(struct pim_msdp_sa *sa)
   }
 
   if (PIM_DEBUG_MSDP_EVENTS) {
-    char key_str[PIM_MSDP_SA_KEY_STRLEN];
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true);
-    zlog_debug("%s de-referenced SPT", key_str);
+    zlog_debug("MSDP SA %s de-referenced SPT", sa->sg_str);
   }
 }
 
@@ -197,15 +176,6 @@ pim_msdp_sa_upstream_update(struct pim_msdp_sa *sa,
                             struct pim_upstream *xg_up, const char *ctx)
 {
   struct pim_upstream *up;
-  char key_str[PIM_MSDP_SA_KEY_STRLEN];
-
-  if (PIM_DEBUG_MSDP_EVENTS || PIM_DEBUG_MSDP_INTERNAL) {
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true);
-  }
-
-  if (PIM_DEBUG_MSDP_INTERNAL) {
-      zlog_debug("%s upstream update on %s", key_str, ctx);
-  }
 
   if (!pim_msdp_sa_upstream_add_ok(sa, xg_up)) {
     pim_msdp_sa_upstream_del(sa);
@@ -222,7 +192,7 @@ pim_msdp_sa_upstream_update(struct pim_msdp_sa *sa,
     /* somehow we lost track of the upstream ptr? best log it */
     sa->up = up;
     if (PIM_DEBUG_MSDP_EVENTS) {
-      zlog_debug("%s SPT reference missing", key_str);
+      zlog_debug("MSDP SA %s SPT reference missing", sa->sg_str);
     }
     return;
   }
@@ -240,11 +210,11 @@ pim_msdp_sa_upstream_update(struct pim_msdp_sa *sa,
     /* should we also start the kat in parallel? we will need it when the
      * SA ages out */
     if (PIM_DEBUG_MSDP_EVENTS) {
-      zlog_debug("%s referenced SPT", key_str);
+      zlog_debug("MSDP SA %s referenced SPT", sa->sg_str);
     }
   } else {
     if (PIM_DEBUG_MSDP_EVENTS) {
-      zlog_debug("%s SPT reference failed", key_str);
+      zlog_debug("MSDP SA %s SPT reference failed", sa->sg_str);
     }
   }
 }
@@ -269,7 +239,7 @@ pim_msdp_sa_new(struct prefix_sg *sg, struct in_addr rp)
   }
 
   sa->sg = *sg;
-  pim_str_sg_set (sg, sa->sg_str);
+  pim_str_sg_set(sg, sa->sg_str);
   sa->rp = rp;
   sa->uptime = pim_time_monotonic_sec();
 
@@ -283,10 +253,7 @@ pim_msdp_sa_new(struct prefix_sg *sg, struct in_addr rp)
   listnode_add_sort(msdp->sa_list, sa);
 
   if (PIM_DEBUG_MSDP_EVENTS) {
-    char key_str[PIM_MSDP_SA_KEY_STRLEN];
-
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true);
-    zlog_debug("%s created", key_str);
+    zlog_debug("MSDP SA %s created", sa->sg_str);
   }
 
   return sa;
@@ -329,16 +296,39 @@ pim_msdp_sa_del(struct pim_msdp_sa * sa)
   hash_release(msdp->sa_hash, sa);
 
   if (PIM_DEBUG_MSDP_EVENTS) {
-    char key_str[PIM_MSDP_SA_KEY_STRLEN];
-
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true /* long */);
-    zlog_debug("%s deleted", key_str);
+    zlog_debug("MSDP SA %s deleted", sa->sg_str);
   }
 
   /* free up any associated memory */
   pim_msdp_sa_free(sa);
 }
 
+static void
+pim_msdp_sa_peer_ip_set(struct pim_msdp_sa *sa, struct pim_msdp_peer *mp)
+{
+  struct pim_msdp_peer *old_mp;
+
+  /* optimize the "no change" case as it will happen
+   * frequently/periodically */
+  if (mp && (sa->peer.s_addr == mp->peer.s_addr)) {
+    return;
+  }
+
+  if (PIM_INADDR_ISNOT_ANY(sa->peer)) {
+    old_mp = pim_msdp_peer_find(sa->peer);
+    if (old_mp && old_mp->sa_cnt) {
+      --old_mp->sa_cnt;
+    }
+  }
+
+  if (mp) {
+    ++mp->sa_cnt;
+    sa->peer = mp->peer;
+  } else {
+    sa->peer.s_addr = PIM_NET_INADDR_ANY;
+  }
+}
+
 /* When a local active-source is removed there is no way to withdraw the
  * source from peers. We will simply remove it from the SA cache so it will
  * not be sent in supsequent SA updates. Peers will consequently timeout the
@@ -350,17 +340,12 @@ pim_msdp_sa_del(struct pim_msdp_sa * sa)
 static void
 pim_msdp_sa_deref(struct pim_msdp_sa *sa, enum pim_msdp_sa_flags flags)
 {
-  char key_str[PIM_MSDP_SA_KEY_STRLEN];
   bool update_up = false;
 
-  if (PIM_DEBUG_MSDP_EVENTS) {
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true);
-  }
-
   if ((sa->flags &PIM_MSDP_SAF_LOCAL)) {
     if (flags & PIM_MSDP_SAF_LOCAL) {
       if (PIM_DEBUG_MSDP_EVENTS) {
-        zlog_debug("%s local reference removed", key_str);
+        zlog_debug("MSDP SA %s local reference removed", sa->sg_str);
       }
       if (msdp->local_cnt)
         --msdp->local_cnt;
@@ -370,9 +355,10 @@ pim_msdp_sa_deref(struct pim_msdp_sa *sa, enum pim_msdp_sa_flags flags)
   if ((sa->flags &PIM_MSDP_SAF_PEER)) {
     if (flags & PIM_MSDP_SAF_PEER) {
       if (PIM_DEBUG_MSDP_EVENTS) {
-        zlog_debug("%s peer reference removed", key_str);
+        zlog_debug("MSDP SA %s peer reference removed", sa->sg_str);
       }
       pim_msdp_sa_state_timer_setup(sa, false /* start */);
+      pim_msdp_sa_peer_ip_set(sa, NULL /* mp */);
       /* if peer ref was removed we need to remove the msdp reference on the
        * msdp entry */
       update_up = true;
@@ -394,26 +380,21 @@ pim_msdp_sa_ref(struct pim_msdp_peer *mp, struct prefix_sg *sg,
                 struct in_addr rp)
 {
   struct pim_msdp_sa *sa;
-  char key_str[PIM_MSDP_SA_KEY_STRLEN];
 
   sa = pim_msdp_sa_add(sg, rp);
   if (!sa) {
     return;
   }
 
-  if (PIM_DEBUG_MSDP_EVENTS) {
-    pim_msdp_sa_key_dump(sa, key_str, sizeof(key_str), true);
-  }
-
   /* reference it */
   if (mp) {
     if (!(sa->flags & PIM_MSDP_SAF_PEER)) {
       sa->flags |= PIM_MSDP_SAF_PEER;
       if (PIM_DEBUG_MSDP_EVENTS) {
-        zlog_debug("%s added by peer", key_str);
+        zlog_debug("MSDP SA %s added by peer", sa->sg_str);
       }
     }
-    sa->peer = mp->peer;
+    pim_msdp_sa_peer_ip_set(sa, mp);
     /* start/re-start the state timer to prevent cache expiry */
     pim_msdp_sa_state_timer_setup(sa, true /* start */);
     /* We re-evaluate SA "SPT-trigger" everytime we hear abt it from a
@@ -425,7 +406,7 @@ pim_msdp_sa_ref(struct pim_msdp_peer *mp, struct prefix_sg *sg,
       sa->flags |= PIM_MSDP_SAF_LOCAL;
       ++msdp->local_cnt;
       if (PIM_DEBUG_MSDP_EVENTS) {
-        zlog_debug("%s added locally", key_str);
+        zlog_debug("MSDP SA %s added locally", sa->sg_str);
       }
       /* send an immediate SA update to peers */
       pim_msdp_pkt_sa_tx_one(sa);
@@ -719,11 +700,9 @@ static void
 pim_msdp_peer_state_chg_log(struct pim_msdp_peer *mp)
 {
   char state_str[PIM_MSDP_STATE_STRLEN];
-  char key_str[PIM_MSDP_PEER_KEY_STRLEN];
 
   pim_msdp_state_dump(mp->state, state_str, sizeof(state_str));
-  pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-  zlog_debug("%s state chg to %s", key_str, state_str);
+  zlog_debug("MSDP peer %s state chg to %s", mp->key_str, state_str);
 }
 
 /* MSDP Connection State Machine actions (defined in RFC-3618:Sec-11.2) */
@@ -800,10 +779,7 @@ pim_msdp_peer_stop_tcp_conn(struct pim_msdp_peer *mp, bool chg_state)
   }
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s pim_msdp_peer_stop_tcp_conn", key_str);
+    zlog_debug("MSDP peer %s pim_msdp_peer_stop_tcp_conn", mp->key_str);
   }
   /* stop read and write threads */
   PIM_MSDP_PEER_READ_OFF(mp);
@@ -832,10 +808,7 @@ void
 pim_msdp_peer_reset_tcp_conn(struct pim_msdp_peer *mp, const char *rc_str)
 {
   if (PIM_DEBUG_EVENTS) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s tcp reset %s", key_str, rc_str);
+    zlog_debug("MSDP peer %s tcp reset %s", mp->key_str, rc_str);
     snprintf(mp->last_reset, sizeof(mp->last_reset), "%s", rc_str);
   }
 
@@ -851,10 +824,7 @@ pim_msdp_peer_reset_tcp_conn(struct pim_msdp_peer *mp, const char *rc_str)
 static void
 pim_msdp_peer_timer_expiry_log(struct pim_msdp_peer *mp, const char *timer_str)
 {
-  char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-  pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-  zlog_debug("%s %s timer expired", key_str, timer_str);
+  zlog_debug("MSDP peer %s %s timer expired", mp->key_str, timer_str);
 }
 
 /* RFC-3618:Sec-5.4 - peer hold timer */
@@ -926,10 +896,7 @@ pim_msdp_peer_active_connect(struct pim_msdp_peer *mp)
   rc = pim_msdp_sock_connect(mp);
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s pim_msdp_peer_active_connect: %d", key_str, rc);
+    zlog_debug("MSDP peer %s pim_msdp_peer_active_connect: %d", mp->key_str, rc);
   }
 
   switch (rc) {
@@ -1032,6 +999,7 @@ pim_msdp_peer_new(struct in_addr peer_addr, struct in_addr local_addr,
   }
 
   mp->peer = peer_addr;
+  pim_inet4_dump("<peer?>", mp->peer, mp->key_str, sizeof(mp->key_str));
   pim_msdp_addr2su(&mp->su_peer, mp->peer);
   mp->local = local_addr;
   /* XXX: originator_id setting needs to move to the mesh group */
@@ -1055,10 +1023,7 @@ pim_msdp_peer_new(struct in_addr peer_addr, struct in_addr local_addr,
   listnode_add_sort(msdp->peer_list, mp);
 
   if (PIM_DEBUG_MSDP_EVENTS) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), true);
-    zlog_debug("%s created", key_str);
+    zlog_debug("MSDP peer %s created", mp->key_str);
 
     pim_msdp_peer_state_chg_log(mp);
   }
@@ -1147,10 +1112,7 @@ pim_msdp_peer_do_del(struct pim_msdp_peer *mp)
   hash_release(msdp->peer_hash, mp);
 
   if (PIM_DEBUG_MSDP_EVENTS) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), true);
-    zlog_debug("%s deleted", key_str);
+    zlog_debug("MSDP peer %s deleted", mp->key_str);
   }
 
   /* free up any associated memory */
diff --git a/pimd/pim_msdp.h b/pimd/pim_msdp.h
index 0f64021805..80d928a05d 100644
--- a/pimd/pim_msdp.h
+++ b/pimd/pim_msdp.h
@@ -53,8 +53,6 @@ enum pim_msdp_err {
 };
 
 #define PIM_MSDP_STATE_STRLEN 16
-#define PIM_MSDP_PEER_KEY_STRLEN 80
-#define PIM_MSDP_SA_KEY_STRLEN 80
 #define PIM_MSDP_UPTIME_STRLEN 80
 #define PIM_MSDP_TIMER_STRLEN 12
 #define PIM_MSDP_TCP_PORT 639
@@ -103,6 +101,7 @@ struct pim_msdp_peer {
   struct in_addr local;
   struct in_addr peer;
   char *mesh_group_name;
+  char key_str[INET_ADDRSTRLEN];
 
   /* state */
   enum pim_msdp_peer_state state;
@@ -131,6 +130,7 @@ struct pim_msdp_peer {
   /* stats */
   uint32_t conn_attempts;
   uint32_t est_flaps;
+  uint32_t sa_cnt; /* number of SAs attributed to this peer */
 #define PIM_MSDP_PEER_LAST_RESET_STR 20
   char last_reset[PIM_MSDP_PEER_LAST_RESET_STR];
 
@@ -217,7 +217,6 @@ int pim_msdp_write(struct thread *thread);
 char *pim_msdp_peer_key_dump(struct pim_msdp_peer *mp, char *buf, int buf_size, bool long_format);
 int pim_msdp_config_write(struct vty *vty);
 void pim_msdp_peer_pkt_txed(struct pim_msdp_peer *mp);
-char *pim_msdp_sa_key_dump(struct pim_msdp_sa *sa, char *buf, int buf_size, bool long_format);
 void pim_msdp_sa_ref(struct pim_msdp_peer *mp, struct prefix_sg *sg, struct in_addr rp);
 void pim_msdp_sa_local_update(struct pim_upstream *up);
 void pim_msdp_sa_local_del(struct prefix_sg *sg);
diff --git a/pimd/pim_msdp_packet.c b/pimd/pim_msdp_packet.c
index bc6156325f..71dd00beca 100644
--- a/pimd/pim_msdp_packet.c
+++ b/pimd/pim_msdp_packet.c
@@ -64,17 +64,65 @@ pim_msdp_pkt_type_dump(enum pim_msdp_tlv type, char *buf, int buf_size)
 }
 
 static void
-pim_msdp_pkt_dump(struct pim_msdp_peer *mp, int type, int len, bool rx)
+pim_msdp_pkt_sa_dump_one(struct stream *s)
+{
+  struct prefix_sg sg;
+
+  /* just throw away the three reserved bytes */
+  stream_get3(s);
+  /* throw away the prefix length also */
+  stream_getc(s);
+
+  memset(&sg, 0, sizeof (struct prefix_sg));
+  sg.grp.s_addr = stream_get_ipv4(s);
+  sg.src.s_addr = stream_get_ipv4(s);
+
+  zlog_debug("  sg %s", pim_str_sg_dump(&sg));
+}
+
+static void
+pim_msdp_pkt_sa_dump(struct stream *s)
+{
+  int entry_cnt;
+  int i;
+  struct in_addr rp; /* Last RP address associated with this SA */
+
+  entry_cnt = stream_getc(s);
+  rp.s_addr = stream_get_ipv4(s);
+
+  if (PIM_DEBUG_MSDP_PACKETS) {
+    char rp_str[INET_ADDRSTRLEN];
+    pim_inet4_dump("<rp?>", rp, rp_str, sizeof(rp_str));
+    zlog_debug("  entry_cnt %d rp %s", entry_cnt, rp_str);
+  }
+
+  /* dump SAs */
+  for (i = 0; i < entry_cnt; ++i) {
+    pim_msdp_pkt_sa_dump_one(s);
+  }
+}
+
+static void
+pim_msdp_pkt_dump(struct pim_msdp_peer *mp, int type, int len, bool rx,
+                  struct stream *s)
 {
-  char key_str[PIM_MSDP_PEER_KEY_STRLEN];
   char type_str[PIM_MSDP_PKT_TYPE_STRLEN];
 
-  pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
   pim_msdp_pkt_type_dump(type, type_str, sizeof(type_str));
 
-  zlog_debug("%s pkt %s type %s len %d",
-      key_str, rx?"rx":"tx", type_str, len);
-  /* XXX: dump actual data */
+  zlog_debug("MSDP peer %s pkt %s type %s len %d",
+      mp->key_str, rx?"rx":"tx", type_str, len);
+
+  if (!s) {
+    return;
+  }
+
+  switch(type) {
+      case PIM_MSDP_V4_SOURCE_ACTIVE:
+        pim_msdp_pkt_sa_dump(s);
+        break;
+      default:;
+  }
 }
 
 /* Check file descriptor whether connect is established. */
@@ -88,7 +136,6 @@ pim_msdp_connect_check(struct pim_msdp_peer *mp)
   if (mp->state != PIM_MSDP_CONNECTING) {
     /* if we are here it means we are not in a connecting or established state
      * for now treat this as a fatal error */
-    /* XXX:revisit; reset TCP connection */
     pim_msdp_peer_reset_tcp_conn(mp, "invalid-state");
     return;
   }
@@ -103,22 +150,17 @@ pim_msdp_connect_check(struct pim_msdp_peer *mp)
   /* If getsockopt is fail, this is fatal error. */
   if (ret < 0) {
     zlog_err("can't get sockopt for nonblocking connect");
-    /* XXX:revisit; reset TCP connection */
     pim_msdp_peer_reset_tcp_conn(mp, "connect-failed");
     return;
   }
 
   /* When status is 0 then TCP connection is established. */
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s pim_connect_check %s", key_str, status?"fail":"success");
+    zlog_debug("MSDP peer %s pim_connect_check %s", mp->key_str, status?"fail":"success");
   }
   if (status == 0) {
     pim_msdp_peer_established(mp);
   } else {
-    /* XXX:revisit; reset TCP connection */
     pim_msdp_peer_reset_tcp_conn(mp, "connect-failed");
   }
 }
@@ -150,18 +192,14 @@ pim_msdp_write(struct thread *thread)
   struct stream *s;
   int num;
   enum pim_msdp_tlv type;
+  int len;
   int work_cnt = 0;
-  char key_str[PIM_MSDP_PEER_KEY_STRLEN];
 
   mp = THREAD_ARG(thread);
   mp->t_write = NULL;
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-  }
-
-  if (PIM_DEBUG_MSDP_INTERNAL) {
-    zlog_debug("%s pim_msdp_write", key_str);
+    zlog_debug("MSDP peer %s pim_msdp_write", mp->key_str);
   }
   if (mp->fd < 0) {
     return -1;
@@ -195,12 +233,11 @@ pim_msdp_write(struct thread *thread)
       /* write failed either retry needed or error */
       if (ERRNO_IO_RETRY(errno)) {
         if (PIM_DEBUG_MSDP_INTERNAL) {
-          zlog_debug("%s pim_msdp_write io retry", key_str);
+          zlog_debug("MSDP peer %s pim_msdp_write io retry", mp->key_str);
         }
         break;
       }
 
-      /* XXX:revisit; reset TCP connection */
       pim_msdp_peer_reset_tcp_conn(mp, "pkt-tx-failed");
       return 0;
     }
@@ -209,7 +246,7 @@ pim_msdp_write(struct thread *thread)
       /* Partial write */
       stream_forward_getp(s, num);
       if (PIM_DEBUG_MSDP_INTERNAL) {
-        zlog_debug("%s pim_msdp_partial_write", key_str);
+        zlog_debug("MSDP peer %s pim_msdp_partial_write", mp->key_str);
       }
       break;
     }
@@ -217,6 +254,7 @@ pim_msdp_write(struct thread *thread)
     /* Retrieve msdp packet type. */
     stream_set_getp(s,0);
     type = stream_getc(s);
+    len = stream_getw(s);
     switch (type)
     {
       case PIM_MSDP_KEEPALIVE:
@@ -228,7 +266,7 @@ pim_msdp_write(struct thread *thread)
       default:;
     }
     if (PIM_DEBUG_MSDP_PACKETS) {
-      pim_msdp_pkt_dump(mp, type, writenum, false /*rx*/);
+      pim_msdp_pkt_dump(mp, type, len, false /*rx*/, s);
     }
 
     /* packet sent delete it. */
@@ -243,7 +281,7 @@ pim_msdp_write(struct thread *thread)
   sockopt_cork(mp->fd, 0);
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    zlog_debug("%s pim_msdp_write wrote %d packets", key_str, work_cnt);
+    zlog_debug("MSDP peer %s pim_msdp_write wrote %d packets", mp->key_str, work_cnt);
   }
 
   return 0;
@@ -301,10 +339,7 @@ pim_msdp_pkt_sa_push(struct pim_msdp_peer *mp)
   } else {
     for (ALL_LIST_ELEMENTS_RO(msdp->peer_list, mpnode, mp)) {
       if (PIM_DEBUG_MSDP_INTERNAL) {
-        char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-        pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-        zlog_debug("%s pim_msdp_pkt_sa_push", key_str);
+        zlog_debug("MSDP peer %s pim_msdp_pkt_sa_push", mp->key_str);
       }
       pim_msdp_pkt_sa_push_to_one_peer(mp);
     }
@@ -413,7 +448,6 @@ pim_msdp_pkt_sa_tx_to_one_peer(struct pim_msdp_peer *mp)
 static void
 pim_msdp_pkt_rxed_with_fatal_error(struct pim_msdp_peer *mp)
 {
-  /* XXX:revisit; reset TCP connection */
   pim_msdp_peer_reset_tcp_conn(mp, "invalid-pkt-rx");
 }
 
@@ -519,7 +553,7 @@ pim_msdp_pkt_rx(struct pim_msdp_peer *mp)
   }
 
   if (PIM_DEBUG_MSDP_PACKETS) {
-    pim_msdp_pkt_dump(mp, type, len, true /*rx*/);
+    pim_msdp_pkt_dump(mp, type, len, true /*rx*/, NULL /*s*/);
   }
 
   switch(type) {
@@ -581,10 +615,7 @@ pim_msdp_read(struct thread *thread)
   mp->t_read = NULL;
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s pim_msdp_read", key_str);
+    zlog_debug("MSDP peer %s pim_msdp_read", mp->key_str);
   }
 
   if (mp->fd < 0) {
diff --git a/pimd/pim_msdp_packet.h b/pimd/pim_msdp_packet.h
index 30221a399a..a362ff11d5 100644
--- a/pimd/pim_msdp_packet.h
+++ b/pimd/pim_msdp_packet.h
@@ -57,8 +57,7 @@
 #define PIM_MSDP_SA_TLV_MIN_SIZE PIM_MSDP_SA_ENTRY_CNT2SIZE(1)
 #define PIM_MSDP_SA_MAX_ENTRY_CNT ((PIM_MSDP_SA_TLV_MAX_SIZE - PIM_MSDP_SA_X_SIZE)/PIM_MSDP_SA_ONE_ENTRY_SIZE)
 
-/* XXX: this is just a guesstimate - need to revist */
-#define PIM_MSDP_MAX_PACKET_SIZE (PIM_MSDP_SA_TLV_MAX_SIZE + PIM_MSDP_KA_TLV_MAX_SIZE)
+#define PIM_MSDP_MAX_PACKET_SIZE max(PIM_MSDP_SA_TLV_MAX_SIZE, PIM_MSDP_KA_TLV_MAX_SIZE)
 
 #define PIM_MSDP_PKT_TYPE_STRLEN 16
 
diff --git a/pimd/pim_msdp_socket.c b/pimd/pim_msdp_socket.c
index f71d23e4a1..bc9720f1f3 100644
--- a/pimd/pim_msdp_socket.c
+++ b/pimd/pim_msdp_socket.c
@@ -86,25 +86,26 @@ pim_msdp_sock_accept(struct thread *thread)
   mp = pim_msdp_peer_find(su.sin.sin_addr);
   if (!mp || !PIM_MSDP_PEER_IS_LISTENER(mp)) {
     ++msdp->rejected_accepts;
-    //XXX: make debug event
-    zlog_err("msdp peer connection refused from %s",
-            sockunion2str(&su, buf, SU_ADDRSTRLEN));
+    if (PIM_DEBUG_MSDP_EVENTS) {
+      zlog_err("msdp peer connection refused from %s",
+          sockunion2str(&su, buf, SU_ADDRSTRLEN));
+    }
     close(msdp_sock);
     return -1;
   }
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s accept success%s", key_str, mp->fd>=0?"(dup)":"");
+    zlog_debug("MSDP peer %s accept success%s", mp->key_str, mp->fd>=0?"(dup)":"");
   }
 
   /* if we have an existing connection we need to kill that one
    * with this one */
   if (mp->fd >= 0) {
-    /* XXX: revisit */
-    pim_msdp_peer_stop_tcp_conn(mp, false /* chg_state */);
+    if (PIM_DEBUG_MSDP_EVENTS) {
+      zlog_err("msdp peer new connection from %s stop old connection",
+          sockunion2str(&su, buf, SU_ADDRSTRLEN));
+    }
+    pim_msdp_peer_stop_tcp_conn(mp, true /* chg_state */);
   }
   mp->fd = msdp_sock;
   set_nonblocking(mp->fd);
@@ -187,16 +188,15 @@ pim_msdp_sock_connect(struct pim_msdp_peer *mp)
   int rc;
 
   if (PIM_DEBUG_MSDP_INTERNAL) {
-    char key_str[PIM_MSDP_PEER_KEY_STRLEN];
-
-    pim_msdp_peer_key_dump(mp, key_str, sizeof(key_str), false);
-    zlog_debug("%s attempt connect%s", key_str, mp->fd<0?"":"(dup)");
+    zlog_debug("MSDP peer %s attempt connect%s", mp->key_str, mp->fd<0?"":"(dup)");
   }
 
   /* if we have an existing connection we need to kill that one
    * with this one */
   if (mp->fd >= 0) {
-    /* XXX: revisit */
+    if (PIM_DEBUG_MSDP_EVENTS) {
+      zlog_err("msdp duplicate connect to %s nuke old connection", mp->key_str);
+    }
     pim_msdp_peer_stop_tcp_conn(mp, false /* chg_state */);
   }
 
diff --git a/pimd/pimd.h b/pimd/pimd.h
index 54cecd19d2..cb0165c22c 100644
--- a/pimd/pimd.h
+++ b/pimd/pimd.h
@@ -52,6 +52,8 @@
 #define PIM_INADDR_IS_ANY(addr) (addr).s_addr == PIM_NET_INADDR_ANY
 #define PIM_INADDR_ISNOT_ANY(addr) ((addr).s_addr != PIM_NET_INADDR_ANY) /* struct in_addr addr */
 
+#define max(x,y) ((x) > (y) ? (x) : (y))
+
 #define PIM_MASK_PIM_EVENTS          (1 << 0)
 #define PIM_MASK_PIM_EVENTS_DETAIL   (1 << 1)
 #define PIM_MASK_PIM_PACKETS         (1 << 2)