You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2010/09/16 02:05:15 UTC
svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h
modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Author: minfrin
Date: Thu Sep 16 00:05:14 2010
New Revision: 997545
URL: http://svn.apache.org/viewvc?rev=997545&view=rev
Log:
mod_cache: Add a discrete commit_entity() provider function within the
mod_cache provider interface which is called to indicate to the
provider that caching is complete, giving the provider the opportunity
to commit temporary files permanently to the cache in an atomic
fashion. Move all "rename" functionality of temporary files to permanent
files within mod_disk_cache from ad hoc locations in the code to the
commit_entity() function. Instead of reusing the same variables for
temporary file handling in mod_disk_cache, introduce separate discrete
structures for each of the three cache file types, the headers file,
vary file and data file, so that the atomic rename of all three file
types within commit_entity() becomes possible. Replace the inconsistent
use of error cleanups with a formal set of pool cleanups attached to
a subpool, which is destroyed on error.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/modules/cache/mod_cache.c
httpd/httpd/trunk/modules/cache/mod_cache.h
httpd/httpd/trunk/modules/cache/mod_disk_cache.c
httpd/httpd/trunk/modules/cache/mod_disk_cache.h
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Sep 16 00:05:14 2010
@@ -2,6 +2,20 @@
Changes with Apache 2.3.9
+ *) mod_cache: Add a discrete commit_entity() provider function within the
+ mod_cache provider interface which is called to indicate to the
+ provider that caching is complete, giving the provider the opportunity
+ to commit temporary files permanently to the cache in an atomic
+ fashion. Move all "rename" functionality of temporary files to permanent
+ files within mod_disk_cache from ad hoc locations in the code to the
+ commit_entity() function. Instead of reusing the same variables for
+ temporary file handling in mod_disk_cache, introduce separate discrete
+ structures for each of the three cache file types, the headers file,
+ vary file and data file, so that the atomic rename of all three file
+ types within commit_entity() becomes possible. Replace the inconsistent
+ use of error cleanups with a formal set of pool cleanups attached to
+ a subpool, which is destroyed on error. [Graham Leggett]
+
*) mod_cache: Change the signature of the store_body() provider function
within the mod_cache provider interface to support an "in" brigade
and an "out" brigade instead of just a single input brigade. This
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Thu Sep 16 00:05:14 2010
@@ -251,12 +251,14 @@
* 20100905.1 (2.3.9-dev) Add ap_cache_check_allowed()
* 20100912.0 (2.3.9-dev) Add an additional "out" brigade parameter to the
* mod_cache store_body() provider function.
+ * 20100916.0 (2.3.9-dev) Add commit_entity() to the mod_cache provider
+ * interface.
*/
#define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20100912
+#define MODULE_MAGIC_NUMBER_MAJOR 20100916
#endif
#define MODULE_MAGIC_NUMBER_MINOR 0 /* 0...n */
Modified: httpd/httpd/trunk/modules/cache/mod_cache.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.c?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_cache.c (original)
+++ httpd/httpd/trunk/modules/cache/mod_cache.c Thu Sep 16 00:05:14 2010
@@ -571,12 +571,14 @@ static int cache_out_filter(ap_filter_t
* case where the provider can't swallow the full brigade. In this
* case, we write the brigade we were passed out downstream, and
* loop around to try and cache some more until the in brigade is
- * completely empty.
+ * completely empty. As soon as the out brigade contains eos, call
+ * commit_entity() to finalise the cached element.
*/
static int cache_save_store(ap_filter_t *f, apr_bucket_brigade *in,
cache_server_conf *conf, cache_request_rec *cache)
{
int rv = APR_SUCCESS;
+ apr_bucket *e;
/* pass the brigade in into the cache provider, which is then
* expected to move cached buckets to the out brigade, for us
@@ -601,6 +603,17 @@ static int cache_save_store(ap_filter_t
}
+ /* does the out brigade contain eos? if so, we're done, commit! */
+ for (e = APR_BRIGADE_FIRST(cache->out);
+ e != APR_BRIGADE_SENTINEL(cache->out);
+ e = APR_BUCKET_NEXT(e))
+ {
+ if (APR_BUCKET_IS_EOS(e)) {
+ rv = cache->provider->commit_entity(cache->handle, f->r);
+ break;
+ }
+ }
+
/* conditionally remove the lock as soon as we see the eos bucket */
ap_cache_remove_lock(conf, f->r, cache->handle ?
(char *)cache->handle->cache_obj->key : NULL, cache->out);
@@ -1154,6 +1167,13 @@ static int cache_save_filter(ap_filter_t
apr_bucket *bkt;
int status;
+ /* We're just saving response headers, so we are done. Commit
+ * the response at this point, unless there was a previous error.
+ */
+ if (rv == APR_SUCCESS) {
+ rv = cache->provider->commit_entity(cache->handle, r);
+ }
+
bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
/* Restore the original request headers and see if we need to
Modified: httpd/httpd/trunk/modules/cache/mod_cache.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_cache.h?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_cache.h (original)
+++ httpd/httpd/trunk/modules/cache/mod_cache.h Thu Sep 16 00:05:14 2010
@@ -240,6 +240,7 @@ typedef struct {
int (*open_entity) (cache_handle_t *h, request_rec *r,
const char *urlkey);
int (*remove_url) (cache_handle_t *h, apr_pool_t *p);
+ apr_status_t (*commit_entity)(cache_handle_t *h, request_rec *r);
} cache_provider;
/* A linked-list of authn providers. */
Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.c?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_disk_cache.c (original)
+++ httpd/httpd/trunk/modules/cache/mod_disk_cache.c Thu Sep 16 00:05:14 2010
@@ -154,49 +154,57 @@ static apr_status_t safe_file_rename(dis
return rv;
}
-static apr_status_t file_cache_el_final(disk_cache_object_t *dobj,
+static apr_status_t file_cache_el_final(disk_cache_conf *conf, disk_cache_file_t *file,
request_rec *r)
{
- /* move the data over */
- if (dobj->tfd) {
- apr_status_t rv;
-
- apr_file_close(dobj->tfd);
-
- /* This assumes that the tempfile is on the same file system
- * as the cache_root. If not, then we need a file copy/move
- * rather than a rename.
- */
- rv = apr_file_rename(dobj->tempfile, dobj->datafile, r->pool);
+ apr_status_t rv;
+
+ /* This assumes that the tempfiles are on the same file system
+ * as the cache_root. If not, then we need a file copy/move
+ * rather than a rename.
+ */
+
+ /* move the file over */
+ if (file->tempfd) {
+
+ rv = safe_file_rename(conf, file->tempfile, file->file, file->pool);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
- "disk_cache: rename tempfile to datafile failed:"
- " %s -> %s", dobj->tempfile, dobj->datafile);
- apr_file_remove(dobj->tempfile, r->pool);
+ "disk_cache: rename tempfile to file failed:"
+ " %s -> %s", file->tempfile, file->file);
+ apr_file_remove(file->tempfile, file->pool);
}
- dobj->tfd = NULL;
+ file->tempfd = NULL;
}
- return APR_SUCCESS;
+ return rv;
}
-static apr_status_t file_cache_errorcleanup(disk_cache_object_t *dobj, request_rec *r)
-{
- /* Remove the header file and the body file. */
- apr_file_remove(dobj->hdrsfile, r->pool);
- apr_file_remove(dobj->datafile, r->pool);
-
- /* If we opened the temporary data file, close and remove it. */
- if (dobj->tfd) {
- apr_file_close(dobj->tfd);
- apr_file_remove(dobj->tempfile, r->pool);
- dobj->tfd = NULL;
+static apr_status_t file_cache_temp_cleanup(void *dummy) {
+ disk_cache_file_t *file = (disk_cache_file_t *)dummy;
+
+ /* clean up the temporary file */
+ if (file->tempfd) {
+ apr_file_remove(file->tempfile, file->pool);
+ file->tempfd = NULL;
}
+ file->tempfile = NULL;
+ file->pool = NULL;
return APR_SUCCESS;
}
+static apr_status_t file_cache_create(disk_cache_conf *conf, disk_cache_file_t *file,
+ apr_pool_t *pool)
+{
+ file->pool = pool;
+ file->tempfile = apr_pstrcat(pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+ apr_pool_cleanup_register(pool, file, file_cache_temp_cleanup, file_cache_temp_cleanup);
+
+ return APR_SUCCESS;
+}
/* These two functions get and put state information into the data
* file for an ap_cache_el, this state information will be read
@@ -329,6 +337,7 @@ static int create_entity(cache_handle_t
&disk_cache_module);
cache_object_t *obj;
disk_cache_object_t *dobj;
+ apr_pool_t *pool;
if (conf->cache_root == NULL) {
return DECLINED;
@@ -369,9 +378,16 @@ static int create_entity(cache_handle_t
/* Save the cache root */
dobj->root = apr_pstrndup(r->pool, conf->cache_root, conf->cache_root_len);
dobj->root_len = conf->cache_root_len;
- dobj->datafile = data_file(r->pool, conf, dobj, key);
- dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
- dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+ apr_pool_create(&pool, r->pool);
+
+ file_cache_create(conf, &dobj->hdrs, pool);
+ file_cache_create(conf, &dobj->vary, pool);
+ file_cache_create(conf, &dobj->data, pool);
+
+ dobj->data.file = data_file(r->pool, conf, dobj, key);
+ dobj->hdrs.file = header_file(r->pool, conf, dobj, key);
+ dobj->vary.file = header_file(r->pool, conf, dobj, key);
return OK;
}
@@ -394,6 +410,7 @@ static int open_entity(cache_handle_t *h
cache_info *info;
disk_cache_object_t *dobj;
int flags;
+ apr_pool_t *pool;
h->cache_obj = NULL;
@@ -420,42 +437,43 @@ static int open_entity(cache_handle_t *h
dobj->root = apr_pstrndup(r->pool, conf->cache_root, conf->cache_root_len);
dobj->root_len = conf->cache_root_len;
- dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
+ dobj->vary.file = header_file(r->pool, conf, dobj, key);
flags = APR_READ|APR_BINARY|APR_BUFFERED;
- rc = apr_file_open(&dobj->hfd, dobj->hdrsfile, flags, 0, r->pool);
+ rc = apr_file_open(&dobj->vary.fd, dobj->vary.file, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
return DECLINED;
}
/* read the format from the cache file */
len = sizeof(format);
- apr_file_read_full(dobj->hfd, &format, len, &len);
+ apr_file_read_full(dobj->vary.fd, &format, len, &len);
if (format == VARY_FORMAT_VERSION) {
apr_array_header_t* varray;
apr_time_t expire;
len = sizeof(expire);
- apr_file_read_full(dobj->hfd, &expire, len, &len);
+ apr_file_read_full(dobj->vary.fd, &expire, len, &len);
varray = apr_array_make(r->pool, 5, sizeof(char*));
- rc = read_array(r, varray, dobj->hfd);
+ rc = read_array(r, varray, dobj->vary.fd);
if (rc != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
"disk_cache: Cannot parse vary header file: %s",
- dobj->hdrsfile);
+ dobj->vary.file);
+ apr_file_close(dobj->vary.fd);
return DECLINED;
}
- apr_file_close(dobj->hfd);
+ apr_file_close(dobj->vary.fd);
nkey = regen_key(r->pool, r->headers_in, varray, key);
dobj->hashfile = NULL;
- dobj->prefix = dobj->hdrsfile;
- dobj->hdrsfile = header_file(r->pool, conf, dobj, nkey);
+ dobj->prefix = dobj->vary.file;
+ dobj->hdrs.file = header_file(r->pool, conf, dobj, nkey);
flags = APR_READ|APR_BINARY|APR_BUFFERED;
- rc = apr_file_open(&dobj->hfd, dobj->hdrsfile, flags, 0, r->pool);
+ rc = apr_file_open(&dobj->hdrs.fd, dobj->hdrs.file, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
return DECLINED;
}
@@ -463,56 +481,74 @@ static int open_entity(cache_handle_t *h
else if (format != DISK_FORMAT_VERSION) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
"disk_cache: File '%s' has a version mismatch. File had version: %d.",
- dobj->hdrsfile, format);
+ dobj->vary.file, format);
+ apr_file_close(dobj->vary.fd);
return DECLINED;
}
else {
apr_off_t offset = 0;
+
+ /* oops, not vary as it turns out */
+ dobj->hdrs.fd = dobj->vary.fd;
+ dobj->vary.fd = NULL;
+ dobj->hdrs.file = dobj->vary.file;
+
/* This wasn't a Vary Format file, so we must seek to the
* start of the file again, so that later reads work.
*/
- apr_file_seek(dobj->hfd, APR_SET, &offset);
+ apr_file_seek(dobj->hdrs.fd, APR_SET, &offset);
nkey = key;
}
obj->key = nkey;
dobj->key = nkey;
dobj->name = key;
- dobj->datafile = data_file(r->pool, conf, dobj, nkey);
- dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+
+ apr_pool_create(&pool, r->pool);
+
+ file_cache_create(conf, &dobj->hdrs, pool);
+ file_cache_create(conf, &dobj->vary, pool);
+ file_cache_create(conf, &dobj->data, pool);
+
+ dobj->data.file = data_file(r->pool, conf, dobj, nkey);
/* Open the data file */
flags = APR_READ|APR_BINARY;
#ifdef APR_SENDFILE_ENABLED
/* When we are in the quick handler we don't have the per-directory
- * configuration, so this check only takes the globel setting of
+ * configuration, so this check only takes the global setting of
* the EnableSendFile directive into account.
*/
flags |= AP_SENDFILE_ENABLED(coreconf->enable_sendfile);
#endif
- rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
+ rc = apr_file_open(&dobj->data.fd, dobj->data.file, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
- "disk_cache: Cannot open info header file %s", dobj->datafile);
+ "disk_cache: Cannot open data file %s", dobj->data.file);
+ apr_file_close(dobj->hdrs.fd);
return DECLINED;
}
- rc = apr_file_info_get(&finfo, APR_FINFO_SIZE, dobj->fd);
+ rc = apr_file_info_get(&finfo, APR_FINFO_SIZE, dobj->data.fd);
if (rc == APR_SUCCESS) {
dobj->file_size = finfo.size;
}
/* Read the bytes to setup the cache_info fields */
- rc = file_cache_recall_mydata(dobj->hfd, info, dobj, r);
+ rc = file_cache_recall_mydata(dobj->hdrs.fd, info, dobj, r);
if (rc != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
- "disk_cache: Cannot read header file %s", dobj->hdrsfile);
+ "disk_cache: Cannot read header file %s", dobj->hdrs.file);
+ apr_file_close(dobj->hdrs.fd);
return DECLINED;
}
/* Initialize the cache_handle callback functions */
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"disk_cache: Recalled cached URL info header %s", dobj->name);
+
+ apr_file_close(dobj->hdrs.fd);
+
return OK;
}
@@ -535,35 +571,35 @@ static int remove_url(cache_handle_t *h,
}
/* Delete headers file */
- if (dobj->hdrsfile) {
+ if (dobj->hdrs.file) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
- "disk_cache: Deleting %s from cache.", dobj->hdrsfile);
+ "disk_cache: Deleting %s from cache.", dobj->hdrs.file);
- rc = apr_file_remove(dobj->hdrsfile, p);
+ rc = apr_file_remove(dobj->hdrs.file, p);
if ((rc != APR_SUCCESS) && !APR_STATUS_IS_ENOENT(rc)) {
/* Will only result in an output if httpd is started with -e debug.
* For reason see log_error_core for the case s == NULL.
*/
ap_log_error(APLOG_MARK, APLOG_DEBUG, rc, NULL,
"disk_cache: Failed to delete headers file %s from cache.",
- dobj->hdrsfile);
+ dobj->hdrs.file);
return DECLINED;
}
}
- /* Delete data file */
- if (dobj->datafile) {
+ /* Delete data file */
+ if (dobj->data.file) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
- "disk_cache: Deleting %s from cache.", dobj->datafile);
+ "disk_cache: Deleting %s from cache.", dobj->data.file);
- rc = apr_file_remove(dobj->datafile, p);
+ rc = apr_file_remove(dobj->data.file, p);
if ((rc != APR_SUCCESS) && !APR_STATUS_IS_ENOENT(rc)) {
/* Will only result in an output if httpd is started with -e debug.
* For reason see log_error_core for the case s == NULL.
*/
ap_log_error(APLOG_MARK, APLOG_DEBUG, rc, NULL,
"disk_cache: Failed to delete data file %s from cache.",
- dobj->datafile);
+ dobj->data.file);
return DECLINED;
}
}
@@ -572,7 +608,7 @@ static int remove_url(cache_handle_t *h,
if (dobj->root) {
const char *str_to_copy;
- str_to_copy = dobj->hdrsfile ? dobj->hdrsfile : dobj->datafile;
+ str_to_copy = dobj->hdrs.file ? dobj->hdrs.file : dobj->data.file;
if (str_to_copy) {
char *dir, *slash, *q;
@@ -770,7 +806,7 @@ static apr_status_t recall_headers(cache
disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
/* This case should not happen... */
- if (!dobj->hfd) {
+ if (!dobj->hdrs.fd) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
"disk_cache: recalling headers; but no header fd for %s", dobj->name);
return APR_NOTFOUND;
@@ -780,10 +816,10 @@ static apr_status_t recall_headers(cache
h->resp_hdrs = apr_table_make(r->pool, 20);
/* Call routine to read the header lines/status line */
- read_table(h, r, h->resp_hdrs, dobj->hfd);
- read_table(h, r, h->req_hdrs, dobj->hfd);
+ read_table(h, r, h->resp_hdrs, dobj->hdrs.fd);
+ read_table(h, r, h->req_hdrs, dobj->hdrs.fd);
- apr_file_close(dobj->hfd);
+ apr_file_close(dobj->hdrs.fd);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"disk_cache: Recalled headers for URL %s", dobj->name);
@@ -795,7 +831,7 @@ static apr_status_t recall_body(cache_ha
apr_bucket *e;
disk_cache_object_t *dobj = (disk_cache_object_t*) h->cache_obj->vobj;
- apr_brigade_insert_file(bb, dobj->fd, 0, dobj->file_size, p);
+ apr_brigade_insert_file(bb, dobj->data.fd, 0, dobj->file_size, p);
e = apr_bucket_eos_create(bb->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(bb, e);
@@ -865,66 +901,53 @@ static apr_status_t store_headers(cache_
* vary format hints in the appropriate directory.
*/
if (dobj->prefix) {
- dobj->hdrsfile = dobj->prefix;
+ dobj->hdrs.file = dobj->prefix;
dobj->prefix = NULL;
}
- rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
+ rv = mkdir_structure(conf, dobj->hdrs.file, r->pool);
- rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
+ rv = apr_file_mktemp(&dobj->vary.tempfd, dobj->vary.tempfile,
APR_CREATE | APR_WRITE | APR_BINARY | APR_EXCL,
- r->pool);
+ dobj->vary.pool);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
"disk_cache: could not create temp file %s",
- dobj->tempfile);
+ dobj->vary.tempfile);
return rv;
}
amt = sizeof(format);
- apr_file_write(dobj->tfd, &format, &amt);
+ apr_file_write(dobj->vary.tempfd, &format, &amt);
amt = sizeof(info->expire);
- apr_file_write(dobj->tfd, &info->expire, &amt);
+ apr_file_write(dobj->vary.tempfd, &info->expire, &amt);
varray = apr_array_make(r->pool, 6, sizeof(char*));
tokens_to_array(r->pool, tmp, varray);
- store_array(dobj->tfd, varray);
+ store_array(dobj->vary.tempfd, varray);
- apr_file_close(dobj->tfd);
+ apr_file_close(dobj->vary.tempfd);
- dobj->tfd = NULL;
-
- rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile,
- r->pool);
- if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
- "disk_cache: rename tempfile to varyfile failed: %s -> %s",
- dobj->tempfile, dobj->hdrsfile);
- apr_file_remove(dobj->tempfile, r->pool);
- return rv;
- }
-
- dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
tmp = regen_key(r->pool, r->headers_in, varray, dobj->name);
- dobj->prefix = dobj->hdrsfile;
+ dobj->prefix = dobj->hdrs.file;
dobj->hashfile = NULL;
- dobj->datafile = data_file(r->pool, conf, dobj, tmp);
- dobj->hdrsfile = header_file(r->pool, conf, dobj, tmp);
+ dobj->data.file = data_file(r->pool, conf, dobj, tmp);
+ dobj->hdrs.file = header_file(r->pool, conf, dobj, tmp);
}
}
- rv = apr_file_mktemp(&dobj->hfd, dobj->tempfile,
+ rv = apr_file_mktemp(&dobj->hdrs.tempfd, dobj->hdrs.tempfile,
APR_CREATE | APR_WRITE | APR_BINARY |
- APR_BUFFERED | APR_EXCL, r->pool);
+ APR_BUFFERED | APR_EXCL, dobj->hdrs.pool);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
"disk_cache: could not create temp file %s",
- dobj->tempfile);
+ dobj->hdrs.tempfile);
return rv;
}
@@ -943,11 +966,12 @@ static apr_status_t store_headers(cache_
iov[1].iov_base = (void*)dobj->name;
iov[1].iov_len = disk_info.name_len;
- rv = apr_file_writev(dobj->hfd, (const struct iovec *) &iov, 2, &amt);
+ rv = apr_file_writev(dobj->hdrs.tempfd, (const struct iovec *) &iov, 2, &amt);
if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+ ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
"disk_cache: could not write info to header file %s",
- dobj->hdrsfile);
+ dobj->hdrs.tempfile);
+ apr_file_close(dobj->hdrs.tempfd);
return rv;
}
@@ -956,11 +980,12 @@ static apr_status_t store_headers(cache_
headers_out = ap_cache_cacheable_headers_out(r);
- rv = store_table(dobj->hfd, headers_out);
+ rv = store_table(dobj->hdrs.tempfd, headers_out);
if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+ ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
"disk_cache: could not write out-headers to header file %s",
- dobj->hdrsfile);
+ dobj->hdrs.tempfile);
+ apr_file_close(dobj->hdrs.tempfd);
return rv;
}
}
@@ -972,39 +997,18 @@ static apr_status_t store_headers(cache_
headers_in = ap_cache_cacheable_headers_in(r);
- rv = store_table(dobj->hfd, headers_in);
+ rv = store_table(dobj->hdrs.tempfd, headers_in);
if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+ ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
"disk_cache: could not write in-headers to header file %s",
- dobj->hdrsfile);
+ dobj->hdrs.tempfile);
+ apr_file_close(dobj->hdrs.tempfd);
return rv;
}
}
- apr_file_close(dobj->hfd); /* flush and close */
-
- /* Remove old file with the same name. If remove fails, then
- * perhaps we need to create the directory tree where we are
- * about to write the new headers file.
- */
- rv = apr_file_remove(dobj->hdrsfile, r->pool);
- if (rv != APR_SUCCESS) {
- rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
- }
-
- rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile, r->pool);
- if (rv != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
- "disk_cache: rename tempfile to hdrsfile failed: %s -> %s",
- dobj->tempfile, dobj->hdrsfile);
- apr_file_remove(dobj->tempfile, r->pool);
- return rv;
- }
-
- dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
+ apr_file_close(dobj->hdrs.tempfd); /* flush and close */
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "disk_cache: Stored headers for URL %s", dobj->name);
return APR_SUCCESS;
}
@@ -1022,10 +1026,10 @@ static apr_status_t store_body(cache_han
/* We write to a temp file and then atomically rename the file over
* in file_cache_el_final().
*/
- if (!dobj->tfd) {
- rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
+ if (!dobj->data.tempfd) {
+ rv = apr_file_mktemp(&dobj->data.tempfd, dobj->data.tempfile,
APR_CREATE | APR_WRITE | APR_BINARY |
- APR_BUFFERED | APR_EXCL, r->pool);
+ APR_BUFFERED | APR_EXCL, dobj->data.pool);
if (rv != APR_SUCCESS) {
return rv;
}
@@ -1092,19 +1096,19 @@ static apr_status_t store_body(cache_han
"disk_cache: Error when reading bucket for URL %s",
h->cache_obj->key);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
APR_BRIGADE_CONCAT(out, dobj->bb);
return rv;
}
/* write to the cache, leave if we fail */
- rv = apr_file_write_full(dobj->tfd, str, length, &written);
+ rv = apr_file_write_full(dobj->data.tempfd, str, length, &written);
if (rv != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
"disk_cache: Error when writing cache file for URL %s",
h->cache_obj->key);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
APR_BRIGADE_CONCAT(out, dobj->bb);
return rv;
}
@@ -1115,7 +1119,7 @@ static apr_status_t store_body(cache_han
"(%" APR_OFF_T_FMT ">%" APR_OFF_T_FMT ")",
h->cache_obj->key, dobj->file_size, conf->maxfs);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
APR_BRIGADE_CONCAT(out, dobj->bb);
return APR_EGENERAL;
}
@@ -1145,13 +1149,15 @@ static apr_status_t store_body(cache_han
if (seen_eos) {
const char *cl_header = apr_table_get(r->headers_out, "Content-Length");
+ apr_file_close(dobj->data.tempfd);
+
if (r->connection->aborted || r->no_cache) {
ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
"disk_cache: Discarding body for URL %s "
"because connection has been aborted.",
h->cache_obj->key);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
return APR_EGENERAL;
}
if (dobj->file_size < conf->minfs) {
@@ -1160,7 +1166,7 @@ static apr_status_t store_body(cache_han
"(%" APR_OFF_T_FMT "<%" APR_OFF_T_FMT ")",
h->cache_obj->key, dobj->file_size, conf->minfs);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
return APR_EGENERAL;
}
if (cl_header) {
@@ -1170,17 +1176,47 @@ static apr_status_t store_body(cache_han
"disk_cache: URL %s didn't receive complete response, not caching",
h->cache_obj->key);
/* Remove the intermediate cache file and return non-APR_SUCCESS */
- file_cache_errorcleanup(dobj, r);
+ apr_pool_destroy(dobj->data.pool);
return APR_EGENERAL;
}
}
- /* All checks were fine. Move tempfile to final destination */
- /* Link to the perm file, and close the descriptor */
- file_cache_el_final(dobj, r);
+ /* All checks were fine, we're good to go when the commit comes */
+ }
+
+ return APR_SUCCESS;
+}
+
+static apr_status_t commit_entity(cache_handle_t *h, request_rec *r)
+{
+ disk_cache_conf *conf = ap_get_module_config(r->server->module_config,
+ &disk_cache_module);
+ disk_cache_object_t *dobj = (disk_cache_object_t *) h->cache_obj->vobj;
+ apr_status_t rv;
+
+ /* move header and data tempfiles to the final destination */
+ rv = file_cache_el_final(conf, &dobj->hdrs, r);
+ if (APR_SUCCESS == rv) {
+ rv = file_cache_el_final(conf, &dobj->vary, r);
+ }
+ if (APR_SUCCESS == rv) {
+ rv = file_cache_el_final(conf, &dobj->data, r);
+ }
+
+ /* remove the cached items completely on any failure */
+ if (APR_SUCCESS != rv) {
+ remove_url(h, r->pool);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
- "disk_cache: Body for URL %s cached.", dobj->name);
+ "disk_cache: commit_entity: URL '%s' not cached due to earlier disk error.",
+ dobj->name);
}
+ else {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+ "disk_cache: commit_entity: Headers and body for URL %s cached.",
+ dobj->name);
+ }
+
+ apr_pool_destroy(dobj->data.pool);
return APR_SUCCESS;
}
@@ -1359,6 +1395,7 @@ static const cache_provider cache_disk_p
&create_entity,
&open_entity,
&remove_url,
+ &commit_entity
};
static void disk_cache_register_hook(apr_pool_t *p)
Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.h?rev=997545&r1=997544&r2=997545&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/cache/mod_disk_cache.h (original)
+++ httpd/httpd/trunk/modules/cache/mod_disk_cache.h Thu Sep 16 00:05:14 2010
@@ -51,24 +51,29 @@ typedef struct {
apr_time_t response_time;
} disk_cache_info_t;
+typedef struct {
+ apr_pool_t *pool;
+ const char *file;
+ apr_file_t *fd;
+ char *tempfile;
+ apr_file_t *tempfd;
+} disk_cache_file_t;
+
/*
* disk_cache_object_t
* Pointed to by cache_object_t::vobj
*/
typedef struct disk_cache_object {
- const char *root; /* the location of the cache directory */
+ const char *root; /* the location of the cache directory */
apr_size_t root_len;
- char *tempfile; /* temp file tohold the content */
const char *prefix;
- const char *datafile; /* name of file where the data will go */
- const char *hdrsfile; /* name of file where the hdrs will go */
- const char *hashfile; /* Computed hash key for this URI */
- const char *name; /* Requested URI without vary bits - suitable for mortals. */
- const char *key; /* On-disk prefix; URI with Vary bits (if present) */
- apr_file_t *fd; /* data file */
- apr_file_t *hfd; /* headers file */
- apr_file_t *tfd; /* temporary file for data */
- apr_off_t file_size; /* File size of the cached data file */
+ disk_cache_file_t data; /* data file structure */
+ disk_cache_file_t hdrs; /* headers file structure */
+ disk_cache_file_t vary; /* vary file structure */
+ const char *hashfile; /* Computed hash key for this URI */
+ const char *name; /* Requested URI without vary bits - suitable for mortals. */
+ const char *key; /* On-disk prefix; URI with Vary bits (if present) */
+ apr_off_t file_size; /* File size of the cached data file */
disk_cache_info_t disk_info; /* Header information. */
apr_bucket_brigade *bb; /* Set aside brigade */
apr_off_t offset; /* Max size to set aside */
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Sep 2010, at 9:25 AM, Ruediger Pluem wrote:
> And one new warning:
>
> mod_disk_cache.c: In function 'file_cache_el_final':
> mod_disk_cache.c:160: warning: 'rv' may be used uninitialized in
> this function
Fixed in r997664.
Regards,
Graham
--
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h
modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/16/2010 02:05 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Thu Sep 16 00:05:14 2010
> New Revision: 997545
>
> URL: http://svn.apache.org/viewvc?rev=997545&view=rev
> Log:
> mod_cache: Add a discrete commit_entity() provider function within the
> mod_cache provider interface which is called to indicate to the
> provider that caching is complete, giving the provider the opportunity
> to commit temporary files permanently to the cache in an atomic
> fashion. Move all "rename" functionality of temporary files to permanent
> files within mod_disk_cache from ad hoc locations in the code to the
> commit_entity() function. Instead of reusing the same variables for
> temporary file handling in mod_disk_cache, introduce separate discrete
> structures for each of the three cache file types, the headers file,
> vary file and data file, so that the atomic rename of all three file
> types within commit_entity() becomes possible. Replace the inconsistent
> use of error cleanups with a formal set of pool cleanups attached to
> a subpool, which is destroyed on error.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/cache/mod_cache.c
> httpd/httpd/trunk/modules/cache/mod_cache.h
> httpd/httpd/trunk/modules/cache/mod_disk_cache.c
> httpd/httpd/trunk/modules/cache/mod_disk_cache.h
>
And one new warning:
mod_disk_cache.c: In function 'file_cache_el_final':
mod_disk_cache.c:160: warning: 'rv' may be used uninitialized in this function
Regards
Rüdiger
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, Sep 15, 2010 at 8:05 PM, <mi...@apache.org> wrote:
> Author: minfrin
> Date: Thu Sep 16 00:05:14 2010
> New Revision: 997545
>
> URL: http://svn.apache.org/viewvc?rev=997545&view=rev
>
> Modified: httpd/httpd/trunk/CHANGES
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=997545&r1=997544&r2=997545&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Sep 16 00:05:14 2010
> @@ -2,6 +2,20 @@
>
> Changes with Apache 2.3.9
>
> + *) mod_cache: Add a discrete commit_entity() provider function within
> the
> + mod_cache provider interface which is called to indicate to the
> + provider that caching is complete, giving the provider the
> opportunity
> + to commit temporary files permanently to the cache in an atomic
> + fashion. Move all "rename" functionality of temporary files to
> permanent
> + files within mod_disk_cache from ad hoc locations in the code to the
> + commit_entity() function. Instead of reusing the same variables for
> + temporary file handling in mod_disk_cache, introduce separate
> discrete
> + structures for each of the three cache file types, the headers file,
> + vary file and data file, so that the atomic rename of all three file
> + types within commit_entity() becomes possible. Replace the
> inconsistent
> + use of error cleanups with a formal set of pool cleanups attached to
> + a subpool, which is destroyed on error. [Graham Leggett]
> +
> *) mod_cache: Change the signature of the store_body() provider function
> within the mod_cache provider interface to support an "in" brigade
> and an "out" brigade instead of just a single input brigade. This
>
Please simplify these two entries to focus on what the typical user needs to
know; allude to any changes that module developers need to know, without all
the API details.
Thanks!
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 16, 2010, at 5:53 AM, Graham Leggett wrote:
> On 16 Sep 2010, at 9:13 AM, Ruediger Pluem wrote:
>
>>> +static apr_status_t file_cache_create(disk_cache_conf *conf, disk_cache_file_t *file,
>>> + apr_pool_t *pool)
>>> +{
>>> + file->pool = pool;
>>> + file->tempfile = apr_pstrcat(pool, conf->cache_root, AP_TEMPFILE, NULL);
>>> +
>>> + apr_pool_cleanup_register(pool, file, file_cache_temp_cleanup, file_cache_temp_cleanup);
>>
>> Is this correct? Do we want to call file_cache_temp_cleanup when we get forked?
>
> I don't follow, when you we fork during normal request handling?
>
Well, we don't but who knows what other modules may do... :/
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES
include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h
modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Sep 16, 2010 at 11:53:39AM +0200, Graham Leggett wrote:
> On 16 Sep 2010, at 9:13 AM, Ruediger Pluem wrote:
>
> >>+static apr_status_t file_cache_create(disk_cache_conf *conf,
> >>disk_cache_file_t *file,
> >>+ apr_pool_t *pool)
> >>+{
> >>+ file->pool = pool;
> >>+ file->tempfile = apr_pstrcat(pool, conf->cache_root,
> >>AP_TEMPFILE, NULL);
> >>+
> >>+ apr_pool_cleanup_register(pool, file,
> >>file_cache_temp_cleanup, file_cache_temp_cleanup);
> >
> >Is this correct? Do we want to call file_cache_temp_cleanup when
> >we get forked?
>
> I don't follow, when you we fork during normal request handling?
If using mod_cgi the INCLUDES filter can cause fork/exec when processing
an exec or include, so getting the child cleanups right for any filter
code certainly matters.
On this vein, to say:
> In theory the thread next door has no access to our r->pool,
isn't strictly correct; if apr_proc_create() were to be used in a
threaded process all child cleanups are run for all pools descendant
from the APR global pool, which may include other threads. But this
isn't done thread-safely and it would be huge a train wreck, hence
mod_cgid.
Regards, Joe
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Sep 2010, at 12:02 PM, Plüm, Rüdiger, VF-Group wrote:
> Hm. What happens in a threaded MPM, where we a doing a caching
> operation
> and another thread causes a fork (e.g. because of mod_cgi (I know
> should not be used with threaded MPM's), or some 3rd party module that
> does a fork). Wouldn't that cause our cache file to be deleted in the
> middle of the caching?
> My comment is only about the 4th parameter to
> apr_pool_cleanup_register
> set to file_cache_temp_cleanup, I am fine with setting the 3rd to
> file_cache_temp_cleanup and fully follow that purpose.
> Shouldn't we set the 4th paramter just to apr_pool_cleanup_null?
In theory the thread next door has no access to our r->pool, but
you're right that third party modules may do all sorts of things, and
the fourth parameter is unnecessary.
Fixed in r997676.
Regards,
Graham
--
RE: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Graham Leggett
> Sent: Donnerstag, 16. September 2010 11:54
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r997545 - in /httpd/httpd/trunk:
> CHANGES include/ap_mmn.h modules/cache/mod_cache.c
> modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
> modules/cache/mod_disk_cache.h
>
> On 16 Sep 2010, at 9:13 AM, Ruediger Pluem wrote:
>
> >> +static apr_status_t file_cache_create(disk_cache_conf *conf,
> >> disk_cache_file_t *file,
> >> + apr_pool_t *pool)
> >> +{
> >> + file->pool = pool;
> >> + file->tempfile = apr_pstrcat(pool, conf->cache_root,
> >> AP_TEMPFILE, NULL);
> >> +
> >> + apr_pool_cleanup_register(pool, file,
> file_cache_temp_cleanup,
> >> file_cache_temp_cleanup);
> >
> > Is this correct? Do we want to call file_cache_temp_cleanup
> when we
> > get forked?
>
> I don't follow, when you we fork during normal request handling?
>
> This function creates the structure that keeps track of each of the
> three files we write, along with the tempfiles used to construct the
> data. The cleanup guarantees that tempfiles are deleted should the
> request end unexpectedly, and that we don't leave temp files lying
> around as we previously did.
Hm. What happens in a threaded MPM, where we a doing a caching operation
and another thread causes a fork (e.g. because of mod_cgi (I know
should not be used with threaded MPM's), or some 3rd party module that
does a fork). Wouldn't that cause our cache file to be deleted in the
middle of the caching?
My comment is only about the 4th parameter to apr_pool_cleanup_register
set to file_cache_temp_cleanup, I am fine with setting the 3rd to
file_cache_temp_cleanup and fully follow that purpose.
Shouldn't we set the 4th paramter just to apr_pool_cleanup_null?
Regards
Rüdiger
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c modules/cache/mod_disk_cache.h
Posted by Graham Leggett <mi...@sharp.fm>.
On 16 Sep 2010, at 9:13 AM, Ruediger Pluem wrote:
>> +static apr_status_t file_cache_create(disk_cache_conf *conf,
>> disk_cache_file_t *file,
>> + apr_pool_t *pool)
>> +{
>> + file->pool = pool;
>> + file->tempfile = apr_pstrcat(pool, conf->cache_root,
>> AP_TEMPFILE, NULL);
>> +
>> + apr_pool_cleanup_register(pool, file, file_cache_temp_cleanup,
>> file_cache_temp_cleanup);
>
> Is this correct? Do we want to call file_cache_temp_cleanup when we
> get forked?
I don't follow, when you we fork during normal request handling?
This function creates the structure that keeps track of each of the
three files we write, along with the tempfiles used to construct the
data. The cleanup guarantees that tempfiles are deleted should the
request end unexpectedly, and that we don't leave temp files lying
around as we previously did.
Regards,
Graham
--
Re: svn commit: r997545 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h
modules/cache/mod_cache.c modules/cache/mod_cache.h modules/cache/mod_disk_cache.c
modules/cache/mod_disk_cache.h
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/16/2010 02:05 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Thu Sep 16 00:05:14 2010
> New Revision: 997545
>
> URL: http://svn.apache.org/viewvc?rev=997545&view=rev
> Log:
> mod_cache: Add a discrete commit_entity() provider function within the
> mod_cache provider interface which is called to indicate to the
> provider that caching is complete, giving the provider the opportunity
> to commit temporary files permanently to the cache in an atomic
> fashion. Move all "rename" functionality of temporary files to permanent
> files within mod_disk_cache from ad hoc locations in the code to the
> commit_entity() function. Instead of reusing the same variables for
> temporary file handling in mod_disk_cache, introduce separate discrete
> structures for each of the three cache file types, the headers file,
> vary file and data file, so that the atomic rename of all three file
> types within commit_entity() becomes possible. Replace the inconsistent
> use of error cleanups with a formal set of pool cleanups attached to
> a subpool, which is destroyed on error.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/cache/mod_cache.c
> httpd/httpd/trunk/modules/cache/mod_cache.h
> httpd/httpd/trunk/modules/cache/mod_disk_cache.c
> httpd/httpd/trunk/modules/cache/mod_disk_cache.h
>
> Modified: httpd/httpd/trunk/modules/cache/mod_disk_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_disk_cache.c?rev=997545&r1=997544&r2=997545&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_disk_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_disk_cache.c Thu Sep 16 00:05:14 2010
> @@ -154,49 +154,57 @@ static apr_status_t safe_file_rename(dis
> return rv;
> }
>
> -static apr_status_t file_cache_el_final(disk_cache_object_t *dobj,
> +static apr_status_t file_cache_el_final(disk_cache_conf *conf, disk_cache_file_t *file,
> request_rec *r)
> {
> - /* move the data over */
> - if (dobj->tfd) {
> - apr_status_t rv;
> -
> - apr_file_close(dobj->tfd);
> -
> - /* This assumes that the tempfile is on the same file system
> - * as the cache_root. If not, then we need a file copy/move
> - * rather than a rename.
> - */
> - rv = apr_file_rename(dobj->tempfile, dobj->datafile, r->pool);
> + apr_status_t rv;
> +
> + /* This assumes that the tempfiles are on the same file system
> + * as the cache_root. If not, then we need a file copy/move
> + * rather than a rename.
> + */
> +
> + /* move the file over */
> + if (file->tempfd) {
> +
> + rv = safe_file_rename(conf, file->tempfile, file->file, file->pool);
> if (rv != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
> - "disk_cache: rename tempfile to datafile failed:"
> - " %s -> %s", dobj->tempfile, dobj->datafile);
> - apr_file_remove(dobj->tempfile, r->pool);
> + "disk_cache: rename tempfile to file failed:"
> + " %s -> %s", file->tempfile, file->file);
> + apr_file_remove(file->tempfile, file->pool);
> }
>
> - dobj->tfd = NULL;
> + file->tempfd = NULL;
> }
>
> - return APR_SUCCESS;
> + return rv;
> }
>
> -static apr_status_t file_cache_errorcleanup(disk_cache_object_t *dobj, request_rec *r)
> -{
> - /* Remove the header file and the body file. */
> - apr_file_remove(dobj->hdrsfile, r->pool);
> - apr_file_remove(dobj->datafile, r->pool);
> -
> - /* If we opened the temporary data file, close and remove it. */
> - if (dobj->tfd) {
> - apr_file_close(dobj->tfd);
> - apr_file_remove(dobj->tempfile, r->pool);
> - dobj->tfd = NULL;
> +static apr_status_t file_cache_temp_cleanup(void *dummy) {
> + disk_cache_file_t *file = (disk_cache_file_t *)dummy;
> +
> + /* clean up the temporary file */
> + if (file->tempfd) {
> + apr_file_remove(file->tempfile, file->pool);
> + file->tempfd = NULL;
> }
> + file->tempfile = NULL;
> + file->pool = NULL;
>
> return APR_SUCCESS;
> }
>
> +static apr_status_t file_cache_create(disk_cache_conf *conf, disk_cache_file_t *file,
> + apr_pool_t *pool)
> +{
> + file->pool = pool;
> + file->tempfile = apr_pstrcat(pool, conf->cache_root, AP_TEMPFILE, NULL);
> +
> + apr_pool_cleanup_register(pool, file, file_cache_temp_cleanup, file_cache_temp_cleanup);
Is this correct? Do we want to call file_cache_temp_cleanup when we get forked?
> +
> + return APR_SUCCESS;
> +}
>
> /* These two functions get and put state information into the data
> * file for an ap_cache_el, this state information will be read
Regards
Rüdiger