On 09.11.2010 23:37, Daniel Shahaf wrote:
> stefan2_at_apache.org wrote on Tue, Nov 09, 2010 at 15:51:54 -0000:
>> Author: stefan2
>> Date: Tue Nov 9 15:51:54 2010
>> New Revision: 1033040
>>
>> URL: http://svn.apache.org/viewvc?rev=1033040&view=rev
>> Log:
>> Fix error leaks in cache creation code. Also, having a NULL
>> file handle cache is not allowed (it is only allowed for the
>> membuffer cache).
>>
>> * subversion/libsvn_fs_util/caching.c
>> (svn_fs__get_global_membuffer_cache): fix error leak
>> (svn_fs__get_global_file_handle_cache): prevent PFs later by exiting
>> the process immediately upon cache creation failure
>>
>> Suggested by: stsp
>>
>> Modified:
>> subversion/branches/performance/subversion/libsvn_fs_util/caching.c
>>
>> Modified: subversion/branches/performance/subversion/libsvn_fs_util/caching.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_fs_util/caching.c?rev=1033040&r1=1033039&r2=1033040&view=diff
>> ==============================================================================
>> --- subversion/branches/performance/subversion/libsvn_fs_util/caching.c (original)
>> +++ subversion/branches/performance/subversion/libsvn_fs_util/caching.c Tue Nov 9 15:51:54 2010
>> @@ -49,7 +49,8 @@ svn_fs_get_cache_config(void)
>>
>> /* Access the process-global (singleton) membuffer cache. The first call
>> * will automatically allocate the cache using the current cache config.
>> - * NULL will be returned if the desired cache size is 0.
>> + * NULL will be returned if the desired cache size is 0 or if the cache
>> + * could not be created for some reason.
>> */
>> svn_membuffer_t *
>> svn_fs__get_global_membuffer_cache(void)
>> @@ -75,12 +76,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 +97,23 @@ svn_fs__get_global_file_handle_cache(voi
>> 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_t* err = svn_file_handle_cache__create_cache(
>> +&cache,
>> + cache_settings.file_handle_count,
>> + !cache_settings.single_threaded,
>> + svn_pool_create(NULL));
>> + if (err)
>> + {
>> + svn_error_clear(err);
>> +
>> + /* We need the file handle cache. The only way that an error could
>> + * occur would be some threading error. In that case, there is no
>> + * way we could continue - not even in some limp home mode.
>> + */
>> + SVN_ERR_MALFUNCTION_NO_RETURN();
> Haven't looked at it closely, but:
>
> It seems a shame to lose ERR here. Could we (better) pass it to the
> malfunction handler, or (at least) log it to the FS warning function?
>
Yes, losing the error is somewhat unsatisfying. Basically,
we had to duplicate SVN_ERR_ASSERT_NO_RETURN
to set the #expr parameter of svn_error__malfunction().
Since svn_file_handle_cache__create_cache() gets called
by svn_fs_set_cache_config(), we don't have a FS context
(not even in the caller) that we could use to dump ERR.
>> + }
>> + }
>>
>> return cache;
>> }
>>
>>
> BTW, what's the status of the performance branch? Last I check, its
> STATUS File was empty...
>
Next item to review is the integrate-cache-membuffer branch.
Once that passed the review, large parts of the remaining stuff
could be reviewed and merged again using the STATUS file.
Some changes may require a further integration branch, e.g.
my extensions to the stream interface.
The file handle cache should not be part of SVN 1.7 due to the
high risk of side-effects.
Before I select more revisions for the review, I want go through
my mail and fix the issues that you and others found.
-- Stefan^2.
Received on 2010-11-10 00:32:54 CET