Yep, you are right. I'll do so. dponald
On Wed, Sep 16, 2015 at 12:29 PM, Paul Jakma <[email protected]> wrote: > Hi, > > Should this perhaps be split into 2 commit, for clarity and to ensure the > bug fix stands out? > > regards, > > Paul > > > On Wed, 22 Jul 2015, Donald Sharp wrote: > > peer_delete has been written to handle the peer->group pointer and to >> remove the peer from the peer group if it exists upon deletion being >> called. Shutdown/deletion Code was intentionally setting the peer->group >> to NULL prior to calling peer_delete. This leaked the memory associated >> with >> the peer->group because of refcnt accounting. >> >> Signed-off-by: Donald Sharp <[email protected]> >> --- >> bgpd/bgpd.c | 28 +++++++++++----------------- >> bgpd/bgpd.h | 13 +++++++++++-- >> 2 files changed, 22 insertions(+), 19 deletions(-) >> >> diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c >> index 4de854e..bb2ceed 100644 >> --- a/bgpd/bgpd.c >> +++ b/bgpd/bgpd.c >> @@ -745,9 +745,13 @@ peer_free (struct peer *peer) >> >> /* increase reference count on a struct peer */ >> struct peer * >> -peer_lock (struct peer *peer) >> +peer_lock_with_caller (const char *name, struct peer *peer) >> { >> assert (peer && (peer->lock >= 0)); >> + >> +#if 0 >> + zlog_debug("%s peer_lock %p %d", name, peer, peer->lock); >> +#endif >> >> peer->lock++; >> >> @@ -758,30 +762,22 @@ peer_lock (struct peer *peer) >> * struct peer is freed and NULL returned if last reference >> */ >> struct peer * >> -peer_unlock (struct peer *peer) >> +peer_unlock_with_caller (const char *name, struct peer *peer) >> { >> assert (peer && (peer->lock > 0)); >> + >> +#if 0 >> + zlog_debug("%s peer_unlock %p %d", name, peer, peer->lock); >> +#endif >> >> peer->lock--; >> >> if (peer->lock == 0) >> { >> -#if 0 >> - zlog_debug ("unlocked and freeing"); >> - zlog_backtrace (LOG_DEBUG); >> -#endif >> peer_free (peer); >> return NULL; >> } >> >> -#if 0 >> - if (peer->lock == 1) >> - { >> - zlog_debug ("unlocked to 1"); >> - zlog_backtrace (LOG_DEBUG); >> - } >> -#endif >> - >> return peer; >> } >> >> @@ -863,7 +859,7 @@ peer_create (union sockunion *su, struct bgp *bgp, >> as_t local_as, >> peer->v_routeadv = BGP_DEFAULT_IBGP_ROUTEADV; >> else >> peer->v_routeadv = BGP_DEFAULT_EBGP_ROUTEADV; >> - >> + >> peer = peer_lock (peer); /* bgp peer list reference */ >> listnode_add_sort (bgp->peer, peer); >> >> @@ -1700,7 +1696,6 @@ peer_group_delete (struct peer_group *group) >> >> for (ALL_LIST_ELEMENTS (group->peer, node, nnode, peer)) >> { >> - peer->group = NULL; >> peer_delete (peer); >> } >> list_delete (group->peer); >> @@ -1730,7 +1725,6 @@ peer_group_remote_as_delete (struct peer_group >> *group) >> >> for (ALL_LIST_ELEMENTS (group->peer, node, nnode, peer)) >> { >> - peer->group = NULL; >> peer_delete (peer); >> } >> list_delete_all_node (group->peer); >> diff --git a/bgpd/bgpd.h b/bgpd/bgpd.h >> index 58d1eca..7ae0acb 100644 >> --- a/bgpd/bgpd.h >> +++ b/bgpd/bgpd.h >> @@ -851,8 +851,17 @@ extern struct peer_group *peer_group_lookup (struct >> bgp *, const char *); >> extern struct peer_group *peer_group_get (struct bgp *, const char *); >> extern struct peer *peer_lookup_with_open (union sockunion *, as_t, >> struct in_addr *, >> int *); >> -extern struct peer *peer_lock (struct peer *); >> -extern struct peer *peer_unlock (struct peer *); >> + >> +/* >> + * Peers are incredibly easy to memory leak >> + * due to the various ways that they are actually used >> + * Provide some functionality to debug locks and unlocks >> + */ >> +extern struct peer *peer_lock_with_caller(const char *, struct peer *); >> +extern struct peer *peer_unlock_with_caller(const char *, struct peer *); >> +#define peer_unlock(A) peer_unlock_with_caller(__FUNCTION__, (A)) >> +#define peer_lock(B) peer_lock_with_caller(__FUNCTION__, (B)) >> + >> extern bgp_peer_sort_t peer_sort (struct peer *peer); >> extern int peer_active (struct peer *); >> extern int peer_active_nego (struct peer *); >> >> > -- > Paul Jakma [email protected] @pjakma Key ID: 64A2FF6A > Fortune: > Honesty is for the most part less profitable than dishonesty. > -- Plato >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
