On 09.11.2010 23:37, Daniel Shahaf wrote:
stef...@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.
