cache.c: do not ignore errors from print_slot()

If print_slot() fails, the client will be served an inferior response.
This patch makes sure that such an error will be returned to main(), which
in turn will try to inform about the error in the response itself.

The error is also printed to the cache_log, i.e. stderr, which will make
the error message appear in error_log (atleast when httpd==apache).

Noticed-by: Jim Meyering <jim@meyering.net>
Signed-off-by: Lars Hjemli <hjemli@gmail.com>

2 files changed, 15 insertions, 5 deletions

diff --git a/cache.c b/cache.c
index a996109..aa97ae1 100644
--- a/cache.c
+++ b/cache.c
@@ -223,102 +223,112 @@ static int process_slot(struct cache_slot *slot)
 	int err;
 
 	err = open_slot(slot);
 	if (!err && slot->match) {
 		if (is_expired(slot)) {
 			if (!lock_slot(slot)) {
 				/* If the cachefile has been replaced between
 				 * `open_slot` and `lock_slot`, we'll just
 				 * serve the stale content from the original
 				 * cachefile. This way we avoid pruning the
 				 * newly generated slot. The same code-path
 				 * is chosen if fill_slot() fails for some
 				 * reason.
 				 *
 				 * TODO? check if the new slot contains the
 				 * same key as the old one, since we would
 				 * prefer to serve the newest content.
 				 * This will require us to open yet another
 				 * file-descriptor and read and compare the
 				 * key from the new file, so for now we're
 				 * lazy and just ignore the new file.
 				 */
 				if (is_modified(slot) || fill_slot(slot)) {
 					unlock_slot(slot, 0);
 					close_lock(slot);
 				} else {
 					close_slot(slot);
 					unlock_slot(slot, 1);
 					slot->cache_fd = slot->lock_fd;
 				}
 			}
 		}
-		print_slot(slot);
+		if ((err = print_slot(slot)) != 0) {
+			cache_log("[cgit] error printing cache %s: %s (%d)\n",
+				  slot->cache_name,
+				  strerror(err),
+				  err);
+		}
 		close_slot(slot);
-		return 0;
+		return err;
 	}
 
 	/* If the cache slot does not exist (or its key doesn't match the
 	 * current key), lets try to create a new cache slot for this
 	 * request. If this fails (for whatever reason), lets just generate
 	 * the content without caching it and fool the caller to belive
 	 * everything worked out (but print a warning on stdout).
 	 */
 
 	close_slot(slot);
 	if ((err = lock_slot(slot)) != 0) {
 		cache_log("[cgit] Unable to lock slot %s: %s (%d)\n",
 			  slot->lock_name, strerror(err), err);
 		slot->fn(slot->cbdata);
 		return 0;
 	}
 
 	if ((err = fill_slot(slot)) != 0) {
 		cache_log("[cgit] Unable to fill slot %s: %s (%d)\n",
 			  slot->lock_name, strerror(err), err);
 		unlock_slot(slot, 0);
 		close_lock(slot);
 		slot->fn(slot->cbdata);
 		return 0;
 	}
 	// We've got a valid cache slot in the lock file, which
 	// is about to replace the old cache slot. But if we
 	// release the lockfile and then try to open the new cache
 	// slot, we might get a race condition with a concurrent
 	// writer for the same cache slot (with a different key).
 	// Lets avoid such a race by just printing the content of
 	// the lock file.
 	slot->cache_fd = slot->lock_fd;
 	unlock_slot(slot, 1);
-	err = print_slot(slot);
+	if ((err = print_slot(slot)) != 0) {
+		cache_log("[cgit] error printing cache %s: %s (%d)\n",
+			  slot->cache_name,
+			  strerror(err),
+			  err);
+	}
 	close_slot(slot);
 	return err;
 }
 
 /* Print cached content to stdout, generate the content if necessary. */
 int cache_process(int size, const char *path, const char *key, int ttl,
 		  cache_fill_fn fn, void *cbdata)
 {
 	unsigned long hash;
 	int len, i;
 	char filename[1024];
 	char lockname[1024 + 5];  /* 5 = ".lock" */
 	struct cache_slot slot;
 
 	/* If the cache is disabled, just generate the content */
 	if (size <= 0) {
 		fn(cbdata);
 		return 0;
 	}
 
 	/* Verify input, calculate filenames */
 	if (!path) {
 		cache_log("[cgit] Cache path not specified, caching is disabled\n");
 		fn(cbdata);
 		return 0;
 	}
 	len = strlen(path);
 	if (len > sizeof(filename) - 10) { /* 10 = "/01234567\0" */
 		cache_log("[cgit] Cache path too long, caching is disabled: %s\n",
 			  path);
 		fn(cbdata);
 		return 0;
diff --git a/cgit.c b/cgit.c
index 2036ceb..ac882c3 100644
--- a/cgit.c
+++ b/cgit.c
@@ -351,36 +351,36 @@ static int calc_ttl()
 		return ctx.cfg.cache_static_ttl;
 
 	return ctx.cfg.cache_repo_ttl;
 }
 
 int main(int argc, const char **argv)
 {
 	const char *cgit_config_env = getenv("CGIT_CONFIG");
 	int err, ttl;
 
 	prepare_context(&ctx);
 	cgit_repolist.length = 0;
 	cgit_repolist.count = 0;
 	cgit_repolist.repos = NULL;
 
 	parse_configfile(cgit_config_env ? cgit_config_env : CGIT_CONFIG,
 			 config_cb);
 	ctx.repo = NULL;
 	if (getenv("SCRIPT_NAME"))
 		ctx.cfg.script_name = xstrdup(getenv("SCRIPT_NAME"));
 	if (getenv("QUERY_STRING"))
 		ctx.qry.raw = xstrdup(getenv("QUERY_STRING"));
 	cgit_parse_args(argc, argv);
 	http_parse_querystring(ctx.qry.raw, querystring_cb);
 
 	ttl = calc_ttl();
 	ctx.page.expires += ttl*60;
 	if (ctx.cfg.nocache)
 		ctx.cfg.cache_size = 0;
 	err = cache_process(ctx.cfg.cache_size, ctx.cfg.cache_root,
 			    ctx.qry.raw, ttl, process_request, &ctx);
 	if (err)
-		cache_log("[cgit] error %d - %s\n",
-			  err, strerror(err));
+		cgit_print_error(fmt("Error processing page: %s (%d)",
+				     strerror(err), err));
 	return err;
 }