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
