[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix memory leak like situation when parsing a svn_config_t object repeatedly

From: Bernd Rinn <bernd_at_sdf.lonestar.org>
Date: 2005-07-27 23:52:54 CEST

Greg Hudson wrote:
> Committed with minor adjustments in r15436 and r15437.
>
> While reviewing the patch, it occurred to me that the enumeration
> callbacks should also take a pool parameter, which would be an iteration
> subpool of the pool passed to the enumeration functions. If you feel up
> to making this change (requires revving svn_config_enumerator_t and
> svn_config_section_enumerator_t, and presents some challenges for the
> compatibility wrappers), that would be welcome. Since we haven't had a
> release yet with svn_config_enumerate2 et al, the API for those
> functions can change without having to create svn_config_enumerate3.

I agree. Please find attached my suggestion of a patch.

The callback function then needs to distinguish whether a particular
allocation has to survive the complete enumeration or only one iteration
and choose the appropriate pool.

Since creating a new pool adds overhead, I feel that the caller of
svn_config_enumerate2() & co. should have a choice on whether an
iteration pool is to be created or not. That's controlled by the value
of the argument create_iteration_pool.

Regarding the challenge provided by the compatibility layer: I cheated
and used the old implementation (r15434) of svn_config_enumerate() & co.
The reason is that when adapting the compatibility layer I found that I
would have to wrap both, the callback and the baton. It would have
avoided code duplication, but the code wouldn't have been much shorter
but it would have been harder to understand and less efficient with
respect to CPU cycles.

Please have a look at the change of subversion/libsvn_client/add.c. I
adapted it to the new API but still use the baton pool for all
allocations instead of the pool argument. In practice it doesn't matter
because the two pools are the same. But the allocations that are made by
auto_props_enumerator() are added to the autopros.properties hash map
and thus need to survive the complete enumeration. The callback function
shouldn't assume that create_iteration_pool == FALSE in my opinion.

--
Bernd Rinn


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Received on Wed Jul 27 23:54:21 2005

This is an archived mail posted to the Subversion Dev mailing list.