Please take a look at
http://trac.osgeo.org/gdal/ticket/2537
Markus
Markus Neteler wrote:
Please take a look at
http://trac.osgeo.org/gdal/ticket/2537
Someone else needs to be reminded that the solution to memory leaks in
GRASS libraries is not to use those libraries from persistent
applications. The GRASS libraries exist for GRASS, and are designed
for GRASS; whether or not they are useful for anything else isn't
really an issue.
OTOH, I've committed a fix for the descriptor leak in
lib/proj/datum.c; that actually matters.
--
Glynn Clements <glynn@gclements.plus.com>
On Sun, Aug 24, 2008 at 10:06 PM, Glynn Clements
<glynn@gclements.plus.com> wrote:
Markus Neteler wrote:
Please take a look at
http://trac.osgeo.org/gdal/ticket/2537Someone else needs to be reminded that the solution to memory leaks in
GRASS libraries is not to use those libraries from persistent
applications. The GRASS libraries exist for GRASS, and are designed
for GRASS; whether or not they are useful for anything else isn't
really an issue.OTOH, I've committed a fix for the descriptor leak in
lib/proj/datum.c; that actually matters.
Here the followup:
Comment (by rouault):
Markus, thanks, but as far as I can see, only the leak of FILE* in
lib/proj/datum.c have been applied. There's still a leak of FILE* in
lib/proj/ellipse.c that was addressed by my patch.
--
Ticket URL: <http://trac.osgeo.org/gdal/ticket/2537#comment:9>
Markus
Markus Neteler wrote:
>> Please take a look at
>> http://trac.osgeo.org/gdal/ticket/2537
>
> Someone else needs to be reminded that the solution to memory leaks in
> GRASS libraries is not to use those libraries from persistent
> applications. The GRASS libraries exist for GRASS, and are designed
> for GRASS; whether or not they are useful for anything else isn't
> really an issue.
>
> OTOH, I've committed a fix for the descriptor leak in
> lib/proj/datum.c; that actually matters.Here the followup:
Comment (by rouault):
Markus, thanks, but as far as I can see, only the leak of FILE* in
lib/proj/datum.c have been applied. There's still a leak of FILE* in
lib/proj/ellipse.c that was addressed by my patch.
Okay, I've fixed that one also.
As for the memory leaks, some of them can be fixed by just using
sprintf() instead of G_asprintf(). Some of them involve data which is
known never to leave the function in question, so freeing the data
would be safe.
Others, e.g. the G_reset_mapsets() change, are unsafe, as other code
could be holding onto the pointers which are being freed. Changing
this would require adding G_store() calls and relying upon the caller
to free the data (or just allow it to leak). I think that the env.c
change falls into this category also.
A more problematic case is freeing the opt_in[i] in
lib/proj/get_proj.c. That array is passed to pj_init(). Does PROJ
guarantee that it won't hold onto those pointers? Unless it's actually
specified, even if the current version doesn't, some future version
might.
If in doubt, it's safer to just not free the data. Memory leaks at
this level don't matter to GRASS. They might matter to people trying
to build persistent applications on top of GRASS libraries, but I'm
not sure that should be our problem. Especially when they will have
plenty of other problems, e.g. G_fatal_error().
--
Glynn Clements <glynn@gclements.plus.com>
On Mon, Aug 25, 2008 at 4:37 PM, Glynn Clements
<glynn@gclements.plus.com> wrote:
Markus Neteler wrote:
>> Please take a look at
>> http://trac.osgeo.org/gdal/ticket/2537
>
> Someone else needs to be reminded that the solution to memory leaks in
> GRASS libraries is not to use those libraries from persistent
> applications. The GRASS libraries exist for GRASS, and are designed
> for GRASS; whether or not they are useful for anything else isn't
> really an issue.
>
> OTOH, I've committed a fix for the descriptor leak in
> lib/proj/datum.c; that actually matters.Here the followup:
Comment (by rouault):
Markus, thanks, but as far as I can see, only the leak of FILE* in
lib/proj/datum.c have been applied. There's still a leak of FILE* in
lib/proj/ellipse.c that was addressed by my patch.Okay, I've fixed that one also.
I'll relay your comments to the other ticket (maybe in future it would be
better to write directly there and add grass-dev@... in CC there. The
latter I have done). The OSGeo-ID should also work for the GDAL trac.
Here one more comment from there:
rouault wrote:
I've applied in r15209 my patch, without the part related to
G_find_file2() where it is not clear if the return value must be freed or
not.
Markus