[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-ha-dev
Subject:    Re: [Linux-ha-dev] [patch] Fix potential memory leaks in the HB
From:       Dejan Muhamedagic <dejanmm () fastmail ! fm>
Date:       2007-10-30 11:56:52
Message-ID: 20071030115651.GB16595 () rondo ! suse ! de
[Download RAW message or body]

Hi,

On Tue, Oct 30, 2007 at 08:53:54PM +0900, Keisuke MORI wrote:
> Hi,
> 
> I've been testing the heartbeat with valgrind enabled,
> and found that it reported a couple of leaks which are
> in the heartbeat API client library.
> 
> I'm submitting my proposed patch to fix them, so
> could somebody please review it and the correctness?
> 
> In my understanding, these leaks are not so serious 
> because the leaks only happens when the heartbeat exits,
> but it may be a problem if a HB client does signon()/signoff()/delete()
> repeatedly in a single process.

Thanks for the patch. I'll take a look and apply it.

Dejan

> 
> The valgrind report is below:
> (It's excerpt. I used the hbagent to generate this, but it has
> some other problems and I'm still looking into it.)
> 
> ----8<--------8<--------8<--------8<--------8<--------8<--------8<----
> (1) ==13900== 72 (28 direct, 44 indirect) bytes in 1 blocks are definitely lost in \
> loss record 8 of 47 (1) ==13900==    at 0x4004405: malloc (vg_replace_malloc.c:149)
> (1) ==13900==    by 0xAB1B72: g_malloc (in /usr/lib/libglib-2.0.so.0.400.7)
> (1) ==13900==    by 0xAA142E: g_hash_table_new_full (in \
> /usr/lib/libglib-2.0.so.0.400.7) (1) ==13900==    by 0xAA14C4: g_hash_table_new (in \
> /usr/lib/libglib-2.0.so.0.400.7) (1) ==13900==    by 0x404D5A0: hb_api_signon \
> (client_lib.c:359) (1) ==13900==    by 0x804A12B: init_heartbeat (hbagent.c:572)
> (1) ==13900==    by 0x804AF1B: main (hbagent.c:1382)
> (1) ==13900== 
> (1) ==13900== 
> (1) ==13900== 881 (12 direct, 869 indirect) bytes in 1 blocks are definitely lost \
> in loss record 21 of 47 (1) ==13900==    at 0x4004405: malloc \
> (vg_replace_malloc.c:149) (1) ==13900==    by 0x4049760: enqueue_msg \
> (client_lib.c:1535) (1) ==13900==    by 0x404B51E: read_api_msg (client_lib.c:1684)
> (1) ==13900==    by 0x404D680: hb_api_signon (client_lib.c:396)
> (1) ==13900==    by 0x804A12B: init_heartbeat (hbagent.c:572)
> (1) ==13900==    by 0x804AF1B: main (hbagent.c:1382)
> ----8<--------8<--------8<--------8<--------8<--------8<--------8<----
> 
> 
> -- 
> Keisuke MORI
> NTT DATA Intellilink Corporation
> 

> diff -r 8b07ee716560 lib/hbclient/client_lib.c
> --- a/lib/hbclient/client_lib.c	Thu Oct 25 10:05:45 2007 +0200
> +++ b/lib/hbclient/client_lib.c	Tue Oct 30 19:22:48 2007 +0900
> @@ -161,6 +161,7 @@ static void		zap_iflist(llc_private_t*);
> static void		zap_iflist(llc_private_t*);
> static void		zap_order_seq(llc_private_t* pi);
> static void		zap_order_queue(llc_private_t* pi);
> +static void		zap_msg_queue(llc_private_t* pi);
> static int		enqueue_msg(llc_private_t*,struct ha_msg*);
> static struct ha_msg*	dequeue_msg(llc_private_t*);
> static gen_callback_t*	search_gen_callback(const char * type, llc_private_t*);
> @@ -361,8 +362,9 @@ hb_api_signon(struct ll_cluster* cinfo, 
> 
> 	/* Connect to the heartbeat API server */
> 
> -	if ((pi->chan = ipc_channel_constructor(IPC_ANYTYPE, wchanattrs))
> -	==	NULL) {
> +	pi->chan = ipc_channel_constructor(IPC_ANYTYPE, wchanattrs);
> +	g_hash_table_destroy(wchanattrs);
> +	if (pi->chan == NULL) {
> 		ha_api_log(LOG_ERR, "hb_api_signon: Can't connect"
> 		" to heartbeat");
> 		ZAPMSG(request);
> @@ -504,7 +506,8 @@ hb_api_delete(struct ll_cluster* ci)
> 	zap_iflist(pi);
> 	zap_nodelist(pi);
> 
> -	/* What about our message queue? */
> +	/* Free up the message queue */
> +	zap_msg_queue(pi);
> 
> 	/* Free up the private information */
> 	memset(pi, 0, sizeof(*pi));
> @@ -1479,6 +1482,23 @@ zap_order_queue(llc_private_t* pi)
> 	}
> 	pi->order_queue_head = NULL;
> }
> +
> +static void
> +zap_msg_queue(llc_private_t* pi)
> +{
> +	struct MsgQueue* qelem = pi->firstQdmsg;
> +	struct MsgQueue* next;
> +
> +	while (qelem != NULL){
> +		next = qelem->next;
> +		ZAPMSG(qelem->value);
> +		cl_free(qelem);
> +		qelem = next;	 
> +	}
> +	pi->firstQdmsg = NULL;
> +	pi->lastQdmsg = NULL;
> +}
> +
> 
> /*
> * Create a new stringlist.

> _______________________________________________________
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
> Home Page: http://linux-ha.org/

_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/


[prev in list] [next in list] [prev in thread] [next in thread]