On Thu, 5 Oct 2006, Niklas Edmundsson wrote:
OK, here comes the latest two patches in the mod_disk_cache improvement
parody. I'll attach these patches to bug #39380, but with less comments.
I discovered a few misses, mostly not NULL:ing fd pointers when
closing them, missing close/flush, and some unneccessary code
duplication instead of calling the right helper in
replace_brigade_with_cache().
The misses are in the loadstore-patch, so I would recommend applying
this before reviewing the results even though it's generated from a
file with the read-while-caching patch applied.
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | [EMAIL PROTECTED]
---------------------------------------------------------------------------
Real men write self-modifying code
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=--- mod_disk_cache.c.rwc 2006-10-06 14:22:27.000000000 +0200
+++ mod_disk_cache.c 2006-10-08 19:17:31.000000000 +0200
@@ -676,6 +676,7 @@ static apr_status_t open_header_timeout(
while(1) {
if(dobj->hfd) {
apr_file_close(dobj->hfd);
+ dobj->hfd = NULL;
}
rc = open_header(h, r, key, conf);
if(rc != APR_SUCCESS && rc != CACHE_ENODATA) {
@@ -1209,6 +1210,7 @@ static apr_status_t recall_headers(cache
}
apr_file_close(dobj->hfd);
+ dobj->hfd = NULL;
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"disk_cache: Recalled headers for URL %s", dobj->name);
@@ -1556,6 +1558,7 @@ static apr_status_t store_headers(cache_
rv = apr_file_open(&dobj->hfd, dobj->hdrsfile,
APR_WRITE | APR_BINARY | APR_BUFFERED, 0, r->pool);
if (rv != APR_SUCCESS) {
+ dobj->hfd = NULL;
return rv;
}
}
@@ -1590,6 +1593,19 @@ static apr_status_t store_headers(cache_
return rv;
}
+ /* If the body size is unknown, the header file will be rewritten later
+ so we can't close it */
+ if(dobj->initial_size < 0) {
+ rv = apr_file_flush(dobj->hfd);
+ }
+ else {
+ rv = apr_file_close(dobj->hfd);
+ dobj->hfd = NULL;
+ }
+ if(rv != APR_SUCCESS) {
+ return rv;
+ }
+
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"disk_cache: Stored headers for URL %s", dobj->name);
return APR_SUCCESS;
@@ -1666,23 +1682,20 @@ static apr_status_t replace_brigade_with
apr_bucket_brigade *bb)
{
apr_status_t rv;
- int flags;
apr_bucket *e;
- core_dir_config *pdcfg = ap_get_module_config(r->per_dir_config,
- &core_module);
disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
- flags = APR_READ|APR_BINARY;
-#if APR_HAS_SENDFILE
- flags |= ((pdcfg->enable_sendfile == ENABLE_SENDFILE_OFF)
- ? 0 : APR_SENDFILE_ENABLED);
-#endif
-
- rv = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
+ if(dobj->fd) {
+ apr_file_close(dobj->fd);
+ dobj->fd = NULL;
+ }
+ rv = open_body_timeout(r, dobj->name, dobj);
if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
- "disk_cache: Error opening datafile %s for URL %s",
- dobj->datafile, dobj->name);
+ if(rv != CACHE_EDECLINED) {
+ ap_log_error(APLOG_MARK, APLOG_ERR, rv, r->server,
+ "disk_cache: Error opening datafile %s for URL %s",
+ dobj->datafile, dobj->name);
+ }
return rv;
}
@@ -1922,14 +1935,12 @@ static apr_status_t store_body(cache_han
/* All checks were fine, close output file */
rv = apr_file_close(dobj->fd);
+ dobj->fd = NULL;
if(rv != APR_SUCCESS) {
file_cache_errorcleanup(dobj, r);
return rv;
}
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "disk_cache: Body for URL %s cached.", dobj->name);
-
/* Redirect to cachefile if we copied a plain file */
if(copy_file) {
rv = replace_brigade_with_cache(h, r, bb);
