Re: [Evolution-hackers] Possible memory leak

2008-01-15 Thread Jose Dapena Paz

El lun, 14-01-2008 a las 14:33 +0100, Philip Van Hoof escribió:

 I think this needs to be something like this:
 
 GSList *item = cc-status_stack;
 cc-status_stack = g_slist_remove_link(cc-status_stack, item);
 g_slist_free (item);

Or just use g_slist_delete_link:

cc-status_stack = g_slist_delete_link (cc-status_stack, cc-status_stack);

AFAIK it frees the list node.

-- 
Jose Dapena Paz [EMAIL PROTECTED]
Igalia
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Possible memory leak

2008-01-14 Thread Philip Van Hoof

I think I've found the leak ...

g_slist_remove_link:

Removes an element from a GSList, without freeing the element. The
removed element's next link is set to NULL, so that it becomes a
self-contained list with one element.

Notice the without freeing the element

Yet 

void
camel_operation_end (CamelOperation *cc)
{
...

g_free(s);
cc-status_stack = g_slist_remove_link(cc-status_stack,
 cc-status_stack);
UNLOCK();

if (msg) {
cc-status(cc, msg, sofar, oftotal, cc-status_data);
g_free(msg);
}
}

I think this needs to be something like this:

GSList *item = cc-status_stack;
cc-status_stack = g_slist_remove_link(cc-status_stack, item);
g_slist_free (item);


Can somebody with GSList know-how acknowledge this?



On Mon, 2008-01-14 at 02:28 +0100, Philip Van Hoof wrote:
 On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote:
 
  I have this memory analysis tool that I'm willing to believe that tells
  me this line in camel-folder.c causes 381 times a leaked allocation of
  in total 5.95 kb. (I opened one folder of 1000 items). It seems to be
  the g_slice_alloc (which does a malloc since I disabled gslice) of the
  g_hash_node_new.
 
 I found it, this was my own mistake in camel-lite's adaptations.
 
 I have found another leak in camel-operation.c:
 
 The cc-status_stack = g_slist_prepend(cc-status_stack, s); in
 camel_operation_start seems to be leaked each time it's called.
 
 I have 23 calls leaking 184 bytes. Various callers of
 camel_operation_start (all of them, it seems) seem to create this leak.
 
 My version of camel-operation.c adds two fields (a total and a current
 position in stead of a percentage, because my requirements where to give
 accurate to-the-byte progress info, not rounded percentages). Those
 changes are closely reviewed once more and don't seem to be causing this
 leak.
 
 ps. I attached the differences that I have. I basically replaced pc
 with a sofar and a oftotal.
 
 
 ___
 Evolution-hackers mailing list
 Evolution-hackers@gnome.org
 http://mail.gnome.org/mailman/listinfo/evolution-hackers
-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be




___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Possible memory leak

2008-01-14 Thread Matthew Barnes
On Mon, 2008-01-14 at 14:33 +0100, Philip Van Hoof wrote:
 I think I've found the leak ...
 
 g_slist_remove_link:
 
 Removes an element from a GSList, without freeing the element. The
 removed element's next link is set to NULL, so that it becomes a
 self-contained list with one element.
 
 Notice the without freeing the element
 
 Yet 
 
 void
 camel_operation_end (CamelOperation *cc)
 {
   ...
 
   g_free(s);
   cc-status_stack = g_slist_remove_link(cc-status_stack,
cc-status_stack);
   UNLOCK();
 
   if (msg) {
   cc-status(cc, msg, sofar, oftotal, cc-status_data);
   g_free(msg);
   }
 }


g_slist_delete_link() frees the GSList node, whereas
g_slist_remove_link() just disconnects the node from the list.

If I'm reading this right, the preceding logic frees the list node
contents, so I think we just need to use g_slist_delete_link() in place
of g_slist_remove_link().  One line change.

void
camel_operation_end (CamelOperation *cc)
{
...
s = cc-status_stack-data;
...
g_free (s);
cc-status_stack = g_slist_delete_link (
cc-status_stack, cc-status_stack);
...
}

___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers


Re: [Evolution-hackers] Possible memory leak

2008-01-13 Thread Philip Van Hoof

On Mon, 2008-01-14 at 01:47 +0100, Philip Van Hoof wrote:

 I have this memory analysis tool that I'm willing to believe that tells
 me this line in camel-folder.c causes 381 times a leaked allocation of
 in total 5.95 kb. (I opened one folder of 1000 items). It seems to be
 the g_slice_alloc (which does a malloc since I disabled gslice) of the
 g_hash_node_new.

I found it, this was my own mistake in camel-lite's adaptations.

I have found another leak in camel-operation.c:

The cc-status_stack = g_slist_prepend(cc-status_stack, s); in
camel_operation_start seems to be leaked each time it's called.

I have 23 calls leaking 184 bytes. Various callers of
camel_operation_start (all of them, it seems) seem to create this leak.

My version of camel-operation.c adds two fields (a total and a current
position in stead of a percentage, because my requirements where to give
accurate to-the-byte progress info, not rounded percentages). Those
changes are closely reviewed once more and don't seem to be causing this
leak.

ps. I attached the differences that I have. I basically replaced pc
with a sofar and a oftotal.


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be
--- camel-operation.c	2007-12-23 18:12:33.0 +0100
+++ /home/pvanhoof/repos/tinymail/trunk/libtinymail-camel/camel-lite/camel/camel-operation.c	2007-11-15 10:42:00.0 +0100
@@ -46,7 +46,7 @@
 struct _status_stack {
 	guint32 flags;
 	char *msg;
-	int pc;/* last pc reported */
+	int sofar, oftotal;
 	unsigned int stamp;		/* last stamp reported */
 };
 
@@ -514,12 +514,14 @@
 	s = g_malloc0(sizeof(*s));
 	s-msg = msg;
 	s-flags = 0;
+	s-sofar = 0;
+	s-oftotal = 100;
 	cc-lastreport = s;
 	cc-status_stack = g_slist_prepend(cc-status_stack, s);
 
 	UNLOCK();
 
-	cc-status(cc, msg, CAMEL_OPERATION_START, cc-status_data);
+	cc-status(cc, msg, CAMEL_OPERATION_START, 100, cc-status_data);
 
 	d(printf(start '%s'\n, msg, pc));
 }
@@ -578,17 +580,17 @@
 /**
  * camel_operation_progress:
  * @cc: Operation to report to.
- * @pc: Percent complete, 0 to 100.
+ * @sofar: Amount complete
+ * @oftotal: Max amount (or, amount when completed)
  *
  * Report progress on the current operation.  If @cc is NULL, then the
- * currently registered operation is used.  @pc reports the current
- * percentage of completion, which should be in the range of 0 to 100.
+ * currently registered operation is used.  @
  *
  * If the total percentage is not know, then use
  * camel_operation_progress_count().
  **/
 void
-camel_operation_progress (CamelOperation *cc, int pc)
+camel_operation_progress (CamelOperation *cc, int sofar, int oftotal)
 {
 	unsigned int now;
 	struct _status_stack *s;
@@ -608,7 +610,8 @@
 	}
 
 	s = cc-status_stack-data;
-	s-pc = pc;
+	s-sofar = sofar;
+	s-oftotal = oftotal;
 
 	/* Transient messages dont start updating till 4 seconds after
 	   they started, then they update every second */
@@ -632,7 +635,7 @@
 	UNLOCK();
 
 	if (cc) {
-		cc-status(cc, msg, pc, cc-status_data);
+		cc-status(cc, msg, sofar, oftotal, cc-status_data);
 		g_free(msg);
 	}
 }
@@ -646,7 +649,7 @@
 void
 camel_operation_progress_count (CamelOperation *cc, int sofar)
 {
-	camel_operation_progress(cc, sofar);
+	camel_operation_progress(cc, sofar, 100);
 }
 
 /**
@@ -664,7 +667,8 @@
 	struct _status_stack *s, *p;
 	unsigned int now;
 	char *msg = NULL;
-	int pc = 0;
+	int sofar = 0;
+	int oftotal = 100;
 
 	if (cc == NULL)
 		cc = co_getcc();
@@ -693,13 +697,15 @@
 if (p-flags  CAMEL_OPERATION_TRANSIENT) {
 	if (p-stamp + CAMEL_OPERATION_TRANSIENT_DELAY  now) {
 		msg = g_strdup(p-msg);
-		pc = p-pc;
+		sofar = p-sofar;
+		oftotal = p-oftotal;
 		cc-lastreport = p;
 		break;
 	}
 } else {
 	msg = g_strdup(p-msg);
-	pc = p-pc;
+	sofar = p-sofar;
+	oftotal = p-oftotal;
 	cc-lastreport = p;
 	break;
 }
@@ -709,7 +715,8 @@
 		g_free(s-msg);
 	} else {
 		msg = s-msg;
-		pc = CAMEL_OPERATION_END;
+		sofar = CAMEL_OPERATION_END;
+		oftotal = 100;
 		cc-lastreport = s;
 	}
 	g_free(s);
@@ -718,7 +725,7 @@
 	UNLOCK();
 
 	if (msg) {
-		cc-status(cc, msg, pc, cc-status_data);
+		cc-status(cc, msg, sofar, oftotal, cc-status_data);
 		g_free(msg);
 	}
 }
___
Evolution-hackers mailing list
Evolution-hackers@gnome.org
http://mail.gnome.org/mailman/listinfo/evolution-hackers