From: Andrea <mar...@go...> - 2008-08-12 19:18:35
Attachments:
gtkpod.diff
|
Here is a patch to fix some more leaks. One of them is actually to remove a false positive when valgrind detects use of uninitialized data (basically it avoids calling fopen with a NULL filename). Andrea |
From: Tomas C. <to...@db...> - 2008-08-15 09:04:28
|
Andrea wrote: > Here is a patch to fix some more leaks. > > One of them is actually to remove a false positive when valgrind detects > use of uninitialized data (basically it avoids calling fopen with a NULL > filename). Index: src/display.c =================================================================== --- src/display.c (revision 2096) +++ src/display.c (working copy) @@ -565,6 +565,7 @@ gchar *resvalues = (gchar *) g_malloc (sizeof(gint) + (sizeof(gchar) * 3) + sizeof(gint)); g_sprintf (resvalues, "%d x %d", pixwidth, pixheight); text = g_markup_printf_escaped (_("<b>Image Dimensions: %s</b>"), resvalues); + g_free (resvalues); gtk_label_set_markup (GTK_LABEL (res_label), text); g_free (text); Why not simply: text = g_markup_printf_escaped (_("<b>Image Dimensions: %d x %d</b>"), pixwidth, pixheight); so you can get rid of resvalues? tom |
From: Andrea <mar...@go...> - 2008-08-15 17:58:05
|
Tomas Carnecky wrote: > Andrea wrote: >> Here is a patch to fix some more leaks. >> >> One of them is actually to remove a false positive when valgrind detects >> use of uninitialized data (basically it avoids calling fopen with a NULL >> filename). > > Index: src/display.c > =================================================================== > --- src/display.c (revision 2096) > +++ src/display.c (working copy) > @@ -565,6 +565,7 @@ > gchar *resvalues = (gchar *) g_malloc (sizeof(gint) + (sizeof(gchar) > * 3) + sizeof(gint)); > g_sprintf (resvalues, "%d x %d", pixwidth, pixheight); > text = g_markup_printf_escaped (_("<b>Image Dimensions: %s</b>"), > resvalues); > + g_free (resvalues); > gtk_label_set_markup (GTK_LABEL (res_label), text); > g_free (text); > > > Why not simply: > > text = g_markup_printf_escaped (_("<b>Image Dimensions: %d x %d</b>"), > pixwidth, pixheight); > > so you can get rid of resvalues? Looks fine to me. Andrea |
From: P.G. R. <p.g...@ph...> - 2008-08-16 14:30:03
|
> Tomas Carnecky wrote: >> Andrea wrote: >>> Here is a patch to fix some more leaks. >>> >>> One of them is actually to remove a false positive when valgrind >>> detects >>> use of uninitialized data (basically it avoids calling fopen with a >>> NULL >>> filename). >> >> Index: src/display.c >> =================================================================== >> --- src/display.c (revision 2096) >> +++ src/display.c (working copy) >> @@ -565,6 +565,7 @@ >> gchar *resvalues = (gchar *) g_malloc (sizeof(gint) + (sizeof(gchar) >> * 3) + sizeof(gint)); >> g_sprintf (resvalues, "%d x %d", pixwidth, pixheight); >> text = g_markup_printf_escaped (_("<b>Image Dimensions: %s</b>"), >> resvalues); >> + g_free (resvalues); >> gtk_label_set_markup (GTK_LABEL (res_label), text); >> g_free (text); >> >> >> Why not simply: >> >> text = g_markup_printf_escaped (_("<b>Image Dimensions: %d x %d</b>"), >> pixwidth, pixheight); >> >> so you can get rid of resvalues? > > Looks fine to me. > > Andrea > > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's > challenge > Build the coolest Linux based applications with Moblin SDK & win great > prizes > Grand prize is a trip for two to an Open Source event anywhere in the > world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > _______________________________________________ > Gtkpod-devel mailing list > Gtk...@li... > https://lists.sourceforge.net/lists/listinfo/gtkpod-devel > Commited. Thanks. PGR -- Laws are partly formed for the sake of good men, in order to instruct them how they may live on friendly terms with one another, and partly for the sake of those who refuse to be instructed, whose spirit cannot be subdued, or softened, or hindered from plunging into evil. [The Laws, Plato] You fiend! Never have I encountered such corrupt and foul-minded perversity Have you ever considered a career in the church? Bishop of Bath and Wells (Blackadder II) |
From: Andrea <mar...@go...> - 2008-08-16 16:36:00
Attachments:
gtkpod.diff
|
P.G. Richardson wrote: >> Tomas Carnecky wrote: > > Commited. Thanks. > > PGR > The rest of the patch is still valid though. There are 6 more leaks + 1 warning reported by valgrind. I attach the updated patch. Andrea |
From: Jorg S. <Jor...@gm...> - 2008-08-17 02:15:43
|
Hi Andrea, thanks for the patches -- I've committed the fixes and found some more leaks along the way. Keep them coming! Cheers, JCS. Andrea wrote: > P.G. Richardson wrote: >>> Tomas Carnecky wrote: >> Commited. Thanks. >> >> PGR >> > > The rest of the patch is still valid though. > There are 6 more leaks + 1 warning reported by valgrind. > > I attach the updated patch. > > Andrea |
From: Daniele F. <df...@gm...> - 2009-04-17 18:37:31
|
2008/8/12 Andrea wrote: > Here is a patch to fix some more leaks. > Index: src/repository.c > =================================================================== > --- src/repository.c (revision 2096) > +++ src/repository.c (working copy) > @@ -443,6 +443,8 @@ > SYNC_PLAYLIST_MODE_AUTOMATIC); > update_buttons (repwin); > } > + > + g_free (key); > } > > I think this hunk should be reverted because memory is already freed inside finish_int_storage (), in fact I get a segfault when clicking Steps to reproduce: 1. right click a repository or an iPod and choose "Edit iPod properties" 2. select "Playlists" tab 3. select "On startup automatically sync with playlist directories" option 4. segfault: #0 0xb8066430 in __kernel_vsyscall () #1 0xb74fb8a0 in raise () from /lib/tls/i686/cmov/libc.so.6 #2 0xb74fd268 in abort () from /lib/tls/i686/cmov/libc.so.6 #3 0xb753916d in ?? () from /lib/tls/i686/cmov/libc.so.6 #4 0xb753f454 in ?? () from /lib/tls/i686/cmov/libc.so.6 #5 0xb75414b6 in free () from /lib/tls/i686/cmov/libc.so.6 #6 0xb78d2c26 in g_free () from /usr/lib/libglib-2.0.so.0 [omitted the following lines from gtk and glib] -- Daniele Forsi |
From: Andrea <mar...@go...> - 2009-04-17 19:03:36
|
Daniele Forsi wrote: > 2008/8/12 Andrea wrote: > >> Here is a patch to fix some more leaks. > >> Index: src/repository.c >> =================================================================== >> --- src/repository.c (revision 2096) >> +++ src/repository.c (working copy) >> @@ -443,6 +443,8 @@ >> SYNC_PLAYLIST_MODE_AUTOMATIC); >> update_buttons (repwin); >> } >> + >> + g_free (key); >> } >> >> > > I think this hunk should be reverted because memory is already freed > inside finish_int_storage (), in fact I get a segfault when clicking > > Steps to reproduce: > 1. right click a repository or an iPod and choose "Edit iPod properties" > 2. select "Playlists" tab > 3. select "On startup automatically sync with playlist directories" option > 4. segfault: I think it is (a bit) more complicated. In the same file, in all (but one) functions where finish_int_storage() there is a potential memory leak according to which path of the if{} is taken. We need to add g_free() in else{} statements. I will post a patch to fix that during the weekend. Andrea |
From: Andrea <mar...@go...> - 2009-04-17 20:02:38
Attachments:
gtk.diff
|
Andrea wrote: > Daniele Forsi wrote: >> 2008/8/12 Andrea wrote: >> >>> >> I think this hunk should be reverted because memory is already freed >> inside finish_int_storage (), in fact I get a segfault when clicking >> >> Steps to reproduce: >> 1. right click a repository or an iPod and choose "Edit iPod properties" >> 2. select "Playlists" tab >> 3. select "On startup automatically sync with playlist directories" option >> 4. segfault: > > I think it is (a bit) more complicated. > In the same file, in all (but one) functions where finish_int_storage() there is a potential memory > leak according to which path of the if{} is taken. > We need to add g_free() in else{} statements. > > I will post a patch to fix that during the weekend. What about that? I think it is easier if finish_int_storage() does not free the key, but it is explicitly done by the caller. As a side point, (but I don't want to sound negative): I think I've found, fixed and (sometimes caused) a countless number of leaks: has anybody ever thought of moving gtkpod to a framework with "destructor" or "garbage collector"? Good night. |