From: P.G. R. <p.g...@ph...> - 2006-05-29 12:44:58
Attachments:
ipod_init_133129052006.patch
|
Patch updated according to reviewed comments. Includes: ipod_init() itdb_create_directories() ipod_directories_number() - included to determine the number of f.. directories but I presume will be redundant when the number is included in itdb->device fix to stop the Artwork database being written in itdb_write_file on devices not containing said db. Please let me know if anything further is required. Regards Paul -- P.G. Richardson Email: p.g...@ph... |
From: Jorg S. <Jor...@gm...> - 2006-05-29 16:09:40
|
Hi Paul, great! I've done some minor adjustments (e.g., since yesterday the number of directories is available in the model description) and committed to CVS. I've also written a short test program (test-init-ipod.c) to play with. The test also demonstrates how to 'autodetect' the iPod model number. Cheers, JCS. On Mon, May 29, 2006 at 01:44:30PM +0100, P.G. Richardson wrote: > Patch updated according to reviewed comments. > Includes: > ipod_init() > itdb_create_directories() > ipod_directories_number() - included to determine the number of f.. > directories but I presume will be redundant when the number is included in > itdb->device > fix to stop the Artwork database being written in itdb_write_file on devices > not containing said db. > > Please let me know if anything further is required. > > Regards > > Paul > > -- > P.G. Richardson > Email: p.g...@ph... |
From: P.G. R. <p.g...@ph...> - 2006-05-29 21:08:39
Attachments:
init_ipod_glitch
|
Jorg, Thanks for that... Just updated from cvs, hosed the shuffle and re-inited.... ... and a small problem, which I have worked around but not yet resolved. In itdb_create_directories, I did g_free(podpath) as you did with errordir originally then removed it before submitting the patch. With it in place I get the attached backtrace. Now the backtrace is coming from muvon (java program) that through jni is calling jnigpod (will submit for review when finalised), which is dynamically linked to libgpod. Its this bit in the backtrace that I think is the important bit though: *** glibc detected *** /opt/java/j2sdk/bin/java: munmap_chunk(): invalid pointer: 0xb19f0e6b *** ======= Backtrace: ========= /lib/libc.so.6(__libc_free+0x179)[0x1d14f0] /usr/lib/libglib-2.0.so.0(g_free+0x31)[0x4b055a1] When I remove the g_free(podpath), everything works fine. I dont think its specifically a java/jni problem hence bringing it to your attention. I am no expert on glib so any ideas, most appreciated. Thanks PGR On Monday 29 May 2006 17:08, Jorg Schuler wrote: > Hi Paul, > > great! I've done some minor adjustments (e.g., since yesterday the number > of directories is available in the model description) and committed to CVS. > I've also written a short test program (test-init-ipod.c) to play with. The > test also demonstrates how to 'autodetect' the iPod model number. > > Cheers, > > > JCS. > > On Mon, May 29, 2006 at 01:44:30PM +0100, P.G. Richardson wrote: > > Patch updated according to reviewed comments. > > Includes: > > ipod_init() > > itdb_create_directories() > > ipod_directories_number() - included to determine the number of f.. > > directories but I presume will be redundant when the number is included > > in itdb->device > > fix to stop the Artwork database being written in itdb_write_file on > > devices not containing said db. > > > > Please let me know if anything further is required. > > > > Regards > > > > Paul > > > > -- > > P.G. Richardson > > Email: p.g...@ph... > > ------------------------------------------------------- > All the advantages of Linux Managed Hosting--Without the Cost and Risk! > Fully trained technicians. The highest number of Red Hat certifications in > the hosting industry. Fanatical Support. Click to learn more > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=107521&bid=248729&dat=121642 > _______________________________________________ > Gtkpod-devel mailing list > Gtk...@li... > https://lists.sourceforge.net/lists/listinfo/gtkpod-devel -- P.G. Richardson Email: p.g...@ph... |
From: Christophe F. <te...@gn...> - 2006-05-29 21:31:37
Attachments:
eh.diff
|
Le lundi 29 mai 2006 à 22:08 +0100, P.G. Richardson a écrit : > Jorg, > > Thanks for that... > > Just updated from cvs, hosed the shuffle and re-inited.... > > ... and a small problem, which I have worked around but not yet resolved. In > itdb_create_directories, I did g_free(podpath) as you did with errordir > originally then removed it before submitting the patch. With it in place I > get the attached backtrace. I glanced through the code, and I didn't find anything wrong with this g_free(podpath) (ie imo it's right and needed). It would be really nice if you had a C test case that you could run through valgrind, it would probably give really useful information. As a side note, the attached patch makes the error-handling in create_directories less invasive, thus the function is more readable imo (and it fixes a few leaks at the same time). The indentation with that patch is probably crap though :p I also removed the G_GNUC_INTERNAL since a simple static seems enough. I can make a patch fixing only the leaks and the G_GNUC_INTERNAL thing if you prefer. And I haven't even tried compiling that code... Christophe |
From: P.G. R. <p.g...@ph...> - 2006-05-29 21:59:09
|
Ah update to last... Turns out it seems I was mistaken on the exact point of the crash. Rather t= han=20 being the g_free(podpath) in itdb_create_directories, it is in fact the=20 freeing of the itdb database after the writes in itdb_init_ipod: <snip> writeok =3D itdb_shuffle_write(itdb, error); if(! writeok) { itdb_free (itdb); return FALSE; } =09 /*itdb_free (itdb)*/; return TRUE; <snip> Literally commenting it out addresses the problem as far as muvon is=20 concerned. I will look more into this tomorrow and take up Christophe's=20 suggestion for a c test case as it maybe that the java is garbage collectin= g=20 the itdb before the g_free (too clever for own good perhaps!). Cheers for having a look Christophe. Regards PGR On Monday 29 May 2006 22:31, Christophe Fergeau wrote: > Le lundi 29 mai 2006 =E0 22:08 +0100, P.G. Richardson a =E9crit : > > Jorg, > > > > Thanks for that... > > > > Just updated from cvs, hosed the shuffle and re-inited.... > > > > ... and a small problem, which I have worked around but not yet resolve= d. > > In itdb_create_directories, I did g_free(podpath) as you did with > > errordir originally then removed it before submitting the patch. With it > > in place I get the attached backtrace. > > I glanced through the code, and I didn't find anything wrong with this > g_free(podpath) (ie imo it's right and needed). It would be really nice > if you had a C test case that you could run through valgrind, it would > probably give really useful information. > > As a side note, the attached patch makes the error-handling in > create_directories less invasive, thus the function is more readable imo > (and it fixes a few leaks at the same time). The indentation with that > patch is probably crap though :p I also removed the G_GNUC_INTERNAL > since a simple static seems enough. I can make a patch fixing only the > leaks and the G_GNUC_INTERNAL thing if you prefer. And I haven't even > tried compiling that code... > > Christophe =2D-=20 P.G. Richardson Email: p.g...@ph... |
From: Jorg S. <Jor...@gm...> - 2006-05-30 13:02:37
|
> Turns out it seems I was mistaken on the exact point of the crash. Rather than > being the g_free(podpath) in itdb_create_directories, it is in fact the > freeing of the itdb database after the writes in itdb_init_ipod: Found it... g_hash_table_insert (itdb->device->sysinfo, "ModelNumStr", model_number); -> g_hash_table_insert (itdb->device->sysinfo, g_strdup ("ModelNumStr"), g_strdup (model_number)); Cheers, JCS. |
From: Jorg S. <Jor...@gm...> - 2006-05-30 13:02:58
|
Hmmm... that shouldn't be it either. That code is even unchanged for quite a while and handles far more complex iTunesDBs without problems... I'll submit Christophe's patch to make the code nicer -- and take the blame: that was me in gtkpod initially producing the mess... still can't see any leaks, though :-( Cheers, JCS. On Mon, May 29, 2006 at 10:59:01PM +0100, P.G. Richardson wrote: > Ah update to last... > > Turns out it seems I was mistaken on the exact point of the crash. Rather than > being the g_free(podpath) in itdb_create_directories, it is in fact the > freeing of the itdb database after the writes in itdb_init_ipod: > > <snip> > writeok = itdb_shuffle_write(itdb, error); > if(! writeok) > { > itdb_free (itdb); > return FALSE; > } > > /*itdb_free (itdb)*/; > return TRUE; > <snip> > > Literally commenting it out addresses the problem as far as muvon is > concerned. I will look more into this tomorrow and take up Christophe's > suggestion for a c test case as it maybe that the java is garbage collecting > the itdb before the g_free (too clever for own good perhaps!). > > Cheers for having a look Christophe. > > Regards > > PGR > > On Monday 29 May 2006 22:31, Christophe Fergeau wrote: > > Le lundi 29 mai 2006 à 22:08 +0100, P.G. Richardson a écrit : > > > Jorg, > > > > > > Thanks for that... > > > > > > Just updated from cvs, hosed the shuffle and re-inited.... > > > > > > ... and a small problem, which I have worked around but not yet resolved. > > > In itdb_create_directories, I did g_free(podpath) as you did with > > > errordir originally then removed it before submitting the patch. With it > > > in place I get the attached backtrace. > > > > I glanced through the code, and I didn't find anything wrong with this > > g_free(podpath) (ie imo it's right and needed). It would be really nice > > if you had a C test case that you could run through valgrind, it would > > probably give really useful information. > > > > As a side note, the attached patch makes the error-handling in > > create_directories less invasive, thus the function is more readable imo > > (and it fixes a few leaks at the same time). The indentation with that > > patch is probably crap though :p I also removed the G_GNUC_INTERNAL > > since a simple static seems enough. I can make a patch fixing only the > > leaks and the G_GNUC_INTERNAL thing if you prefer. And I haven't even > > tried compiling that code... > > > > Christophe |
From: Christophe F. <te...@gn...> - 2006-05-30 13:15:08
|
Le mardi 30 mai 2006 =C3=A0 21:44 +0900, Jorg Schuler a =C3=A9crit : > I'll submit Christophe's patch to make the code nicer -- and take the b= lame: > that was me in gtkpod initially producing the mess... still can't see a= ny > leaks, though :-( if (errordir) { g_set_error (error, 0, -1, _("Problem creating iPod directory or file: '% s'."), errordir); return FALSE; } g_free (errordir); g_free (podpath); =3D> errordir and podpath are leaked if errordir was set for(i =3D 0; i < dirnum; i++) { if (!errordir) { gchar *num =3D g_strdup_printf ("F%02d", i); pbuf =3D g_build_filename (mp, podpath, "Music", num, NULL); if (!g_file_test (pbuf, G_FILE_TEST_EXISTS)) { if((mkdir(pbuf, 0777) !=3D 0)) errordir =3D pbuf; g_free (num); } if (!errordir) g_free (pbuf); } } =3D> g_free (num) should be done unconditionnally Christophe PS: good catch for the hash table issue :) |
From: Jorg S. <Jor...@gm...> - 2006-05-30 13:27:52
|
On Tue, May 30, 2006 at 03:14:47PM +0200, Christophe Fergeau wrote: > Le mardi 30 mai 2006 à 21:44 +0900, Jorg Schuler a écrit : > > I'll submit Christophe's patch to make the code nicer -- and take the blame: > > that was me in gtkpod initially producing the mess... still can't see any > > leaks, though :-( .... > => errordir and podpath are leaked if errordir was set :-( ... > => g_free (num) should be done unconditionnally Some ghosts live longer than one would like: for(i = 0; i < dirnum; i++) { gchar *num = g_strdup_printf ("F%02d", i); pbuf = g_build_filename (mp, podpath, "Music", num, NULL); if (!g_file_test (pbuf, G_FILE_TEST_EXISTS)) { if((mkdir(pbuf, 0777) != 0)) { goto error_dir; } } g_free (num); g_free (pbuf); } ;-) Cheers, JCS. |
From: P.G. R. <p.g...@ph...> - 2006-05-30 15:37:06
|
Jorg, Was getting towards solving this, this morning. Would you, or someonelse, just mind educating me on why the code below was causing the free() invalid pointer segfault please. Much appreciated. PGR On Tuesday 30 May 2006 14:01, Jorg Schuler wrote: > > Turns out it seems I was mistaken on the exact point of the crash. Rather > > than being the g_free(podpath) in itdb_create_directories, it is in fact > > the freeing of the itdb database after the writes in itdb_init_ipod: > > Found it... > > g_hash_table_insert (itdb->device->sysinfo, > "ModelNumStr", model_number); > > -> > > g_hash_table_insert (itdb->device->sysinfo, > g_strdup ("ModelNumStr"), > g_strdup (model_number)); > > Cheers, > > > JCS. > > > ------------------------------------------------------- > All the advantages of Linux Managed Hosting--Without the Cost and Risk! > Fully trained technicians. The highest number of Red Hat certifications in > the hosting industry. Fanatical Support. Click to learn more > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=107521&bid=248729&dat=121642 > _______________________________________________ > Gtkpod-devel mailing list > Gtk...@li... > https://lists.sourceforge.net/lists/listinfo/gtkpod-devel -- P.G. Richardson Email: p.g...@ph... |
From: Jorg S. <Jor...@gm...> - 2006-05-30 15:46:32
|
It all depends on how the hash table is set up. Here it's set up to g_free all entries (keys and values) when it's destroyed: device->sysinfo = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); Cheers, JCS. > Was getting towards solving this, this morning. Would you, or someonelse, just > mind educating me on why the code below was causing the free() invalid > pointer segfault please. > > Much appreciated. > > PGR > > On Tuesday 30 May 2006 14:01, Jorg Schuler wrote: > > > Turns out it seems I was mistaken on the exact point of the crash. Rather > > > than being the g_free(podpath) in itdb_create_directories, it is in fact > > > the freeing of the itdb database after the writes in itdb_init_ipod: > > > > Found it... > > > > g_hash_table_insert (itdb->device->sysinfo, > > "ModelNumStr", model_number); > > > > -> > > > > g_hash_table_insert (itdb->device->sysinfo, > > g_strdup ("ModelNumStr"), > > g_strdup (model_number)); |
From: Christophe F. <te...@gn...> - 2006-05-30 15:49:09
|
Le mardi 30 mai 2006 =C3=A0 16:36 +0100, P.G. Richardson a =C3=A9crit : > Jorg, >=20 > Was getting towards solving this, this morning. Would you, or someonels= e, just=20 > mind educating me on why the code below was causing the free() invalid=20 > pointer segfault please. The itdb->device->sysinfo hash table is created with: device->sysinfo =3D g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); The 2 last arguments mean that when an element is removed from the hash table (or when the hash table is destroyed), then both the key and the element will be freed using g_free. When doing=20 g_hash_table_insert (itdb->device->sysinfo, "ModelNumStr", model_number); "ModelNumStr" is a static string (ie it's not allocated memory), so you can't use g_free on it, and model_number might be allocated memory, but the hash table probably can't assume it owns the allocated memory and can g_free it. Thus, you need to g_strdup the 2 strings before putting them in the hash table, the hash table then owns those 2 strings and will take care of freeing the g_strdup'ed memory when it's no longer needed. Hope that helps, Christophe |