Hi Stefan,
Just checking if there is anything remainig with this patch?
I haven't noticed a "committed" reply or any further comments.
Gavin "Beau" Baumanis
E: gav...@thespidernet.com
On 09/11/2010, at 3:33 AM, Stefan Sperling wrote:
> On Mon, Nov 08, 2010 at 06:56:16AM -0600, Hyrum K. Wright wrote:
>> On Sun, Nov 7, 2010 at 11:58 AM, Stefan Sperling <s...@elego.de> wrote:
>>> [[[
>>> * subversion/libsvn_fs_util/caching.c
>>> (svn_fs__get_global_membuffer_cache): Fix a possible error leak.
>>> ]]]
>>>
>>> Index: subversion/libsvn_fs_util/caching.c
>>> ===================================================================
>>> --- subversion/libsvn_fs_util/caching.c (revision 1032308)
>>> +++ subversion/libsvn_fs_util/caching.c (working copy)
>>> @@ -62,6 +62,7 @@ svn_fs__get_global_membuffer_cache(void)
>>> /* auto-allocate cache*/
>>> apr_allocator_t *allocator = NULL;
>>> apr_pool_t *pool = NULL;
>>> + svn_error_t *err;
>>>
>>> if (apr_allocator_create(&allocator))
>>> return NULL;
>>> @@ -75,12 +76,17 @@ svn_fs__get_global_membuffer_cache(void)
>>> apr_allocator_max_free_set(allocator, 1);
>>> pool = svn_pool_create_ex(NULL, allocator);
>>>
>>> - svn_cache__membuffer_cache_create
>>> - (&cache,
>>> - (apr_size_t)cache_size,
>>> - (apr_size_t)(cache_size / 16),
>>> - ! svn_fs_get_cache_config()->single_threaded,
>>> - pool);
>>
>> If we're choosing to ignore the error above, you can just wrap the
>> entire call in an svn_error_clear(). No need to introduce and check a
>> temp variable.
>
> Ah, nice trick. Thanks.
>
>>> + err = svn_cache__membuffer_cache_create
>>> + (&cache,
>>> + (apr_size_t)cache_size,
>>> + (apr_size_t)(cache_size / 16),
>>> + ! svn_fs_get_cache_config()->single_threaded,
>>> + pool);
>>> + if (err)
>>> + {
>>> + svn_error_clear(err);
>>> + return NULL;
>>> + }
>>
>> Does this early return actually change functionality? If so, this
>> patch is about more than just fixing an error leak...
>
> It doesn't change functionality.
> Cache is NULL in this case, and we return it next:
>
>>
>>> }
>>>
>>> return cache;
>>>
>
>
> I'm not sure though if svn_cache__membuffer_cache_create() guarantees
> that cache remains NULL in case of error. Maybe this should be
> documented as such to allow callers to ignore errors this way.
>
> New patch below, also fixing a space-before-paren and another
> similar error leak.
>
> [[[
> * subversion/libsvn_fs_util/caching.c
> (svn_fs__get_global_membuffer_cache,
> svn_fs__get_global_file_handle_cache): Fix a possible error leak.
> ]]]
>
> Index: subversion/libsvn_fs_util/caching.c
> ===================================================================
> --- subversion/libsvn_fs_util/caching.c (revision 1032333)
> +++ subversion/libsvn_fs_util/caching.c (working copy)
> @@ -75,12 +75,12 @@ svn_fs__get_global_membuffer_cache(void)
> apr_allocator_max_free_set(allocator, 1);
> pool = svn_pool_create_ex(NULL, allocator);
>
> - svn_cache__membuffer_cache_create
> - (&cache,
> - (apr_size_t)cache_size,
> - (apr_size_t)(cache_size / 16),
> - ! svn_fs_get_cache_config()->single_threaded,
> - pool);
> + svn_error_clear(svn_cache__membuffer_cache_create(
> + &cache,
> + (apr_size_t)cache_size,
> + (apr_size_t)(cache_size / 16),
> + ! svn_fs_get_cache_config()->single_threaded,
> + pool));
> }
>
> return cache;
> @@ -96,10 +96,10 @@ svn_fs__get_global_file_handle_cache(void)
> static svn_file_handle_cache_t *cache = NULL;
>
> if (!cache)
> - svn_file_handle_cache__create_cache(&cache,
> - cache_settings.file_handle_count,
> - !cache_settings.single_threaded,
> - svn_pool_create(NULL));
> + svn_error_clear(svn_file_handle_cache__create_cache(
> + &cache, cache_settings.file_handle_count,
> + !cache_settings.single_threaded,
> + svn_pool_create(NULL)));
>
> return cache;
> }
>
