1 file changed, 116 insertions, 130 deletions
diff --git a/source/cache.c b/source/cache.c
index 9594fd6..0a3166d 100644
--- a/source/cache.c
+++ b/source/cache.c
@@ -12,7 +12,6 @@
 #include "cache.h"
 #include "cgit.h"
 #include "html.h"
-#include "shared.h"
 #include <sys/file.h>
 #ifdef HAVE_LINUX_SENDFILE
 #include <sys/sendfile.h>
@@ -31,21 +30,6 @@
 #define FNV_OFFSET 0x811c9dc5
 #define FNV_PRIME  0x01000193
 
-/*
- * Cache trouble goes to stderr, which under CGI is the web server's error
- * log, so that it cannot land in the middle of the page being written to
- * stdout.
- */
-__attribute__((format (printf,1,2)))
-static void log_error(const char *format, ...)
-{
-	va_list args;
-
-	va_start(args, format);
-	vfprintf(stderr, format, args);
-	va_end(args);
-}
-
 struct cache_slot {
 	const char *key;
 	size_t keylen;
@@ -112,17 +96,12 @@ static int key_fits_slot(const char *key)
 	return strlen(key) + 1 <= CACHE_BUFSIZE;
 }
 
-static int close_slot(struct cache_slot *slot)
+static void close_fd(int *fd)
 {
-	int err = 0;
-
-	if (slot->cache_fd > 0) {
-		if (close(slot->cache_fd))
-			err = errno;
-		else
-			slot->cache_fd = -1;
+	if (*fd > 0) {
+		close(*fd);
+		*fd = -1;
 	}
-	return err;
 }
 
 static int print_slot(struct cache_slot *slot)
@@ -182,7 +161,7 @@ static int serve_slot(struct cache_slot *slot)
 	err = print_slot(slot);
 	// A client that went away mid-page is not trouble worth a line.
 	if (err && err != EPIPE)
-		log_error("[cgit] Unable to send slot %s: %s (%d)\n", slot->path, strerror(err), err);
+		fprintf(stderr, "[cgit] Unable to send slot %s: %s (%d)\n", slot->path, strerror(err), err);
 	return err;
 }
 
@@ -212,63 +191,95 @@ static int is_modified(struct cache_slot *slot)
 	);
 }
 
-static int close_lock(struct cache_slot *slot)
-{
-	int err = 0;
-
-	if (slot->lock_fd > 0) {
-		if (close(slot->lock_fd))
-			err = errno;
-		else
-			slot->lock_fd = -1;
-	}
-	return err;
-}
-
 /*
  * The lock file becomes the slot once it is renamed, so it has to open with
  * the key the same way a slot does. The lock is taken without blocking,
  * because failing to get it is how a second process learns that this slot is
- * already being rebuilt, so it returns an errno instead of waiting.
+ * already being rebuilt, so it returns an errno instead of waiting. Every
+ * other failure is logged here, for both callers.
  */
-static int lock_slot(struct cache_slot *slot)
+int cache_lock_file(const char *path, int *fd)
 {
 	struct stat held, named;
+	int err;
 
-	slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
-	if (slot->lock_fd == -1)
-		return errno;
-	// An flock rather than a POSIX record lock. The descriptor is put on
-	// stdout for the fill, and a record lock is dropped as soon as any
+	// Close-on-exec, so that a program a cache fill runs gets the file
+	// only as its stdout, and holds the lock no longer than it can write
+	// there.
+	*fd = open(path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR);
+	if (*fd == -1) {
+		err = errno;
+		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err);
+		return err;
+	}
+	// An flock rather than a POSIX record lock. A slot's descriptor is put
+	// on stdout for the fill, and a record lock is dropped as soon as any
 	// descriptor on the file closes, which an exec filter does to stdout,
-	// while an flock holds until the last descriptor closes.
-	if (flock(slot->lock_fd, LOCK_EX | LOCK_NB) < 0) {
-		int saved_errno = errno;
-		close(slot->lock_fd);
-		slot->lock_fd = -1;
-		return saved_errno;
+	// while an flock holds until the last descriptor closes. Either way a
+	// holder that dies takes its lock with it.
+	if (flock(*fd, LOCK_EX | LOCK_NB) < 0) {
+		// A lock held elsewhere only means concurrent requests, which
+		// is not worth a line in the server log. Any other failure, a
+		// filesystem without lock support say, keeps the cache from
+		// ever being written and does deserve one.
+		err = errno;
+		if (err != EWOULDBLOCK)
+			fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", path, strerror(err), err);
+		goto fail;
 	}
 	// The lock landed on whatever inode the path named at open. A holder
-	// finishing in between renames that inode into place as the live
-	// slot, and once the path is confirmed to still name this file that
-	// rename can no longer happen, because it takes the lock held here.
+	// finishing in between renames that inode into place, and once the
+	// path is confirmed to still name this file that rename can no longer
+	// happen, because it takes the lock held here.
 	if (
-		fstat(slot->lock_fd, &held)   ||
-		stat(slot->lock_path, &named) ||
-		held.st_ino != named.st_ino   ||
+		fstat(*fd, &held)           ||
+		stat(path, &named)          ||
+		held.st_ino != named.st_ino ||
 		held.st_dev != named.st_dev
 	) {
-		close(slot->lock_fd);
-		slot->lock_fd = -1;
-		return EAGAIN;
+		err = EAGAIN;
+		goto fail;
 	}
 	// A run that died before its rename leaves the lock file behind, so
 	// start from empty now that nobody else can be writing it.
-	if (ftruncate(slot->lock_fd, 0) < 0)
-		return errno;
-	if (write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0)
-		return errno;
+	if (ftruncate(*fd, 0) < 0) {
+		err = errno;
+		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", path, strerror(err), err);
+		unlink(path);
+		goto fail;
+	}
 	return 0;
+
+fail:
+	close(*fd);
+	*fd = -1;
+	return err;
+}
+
+static int lock_slot(struct cache_slot *slot)
+{
+	int err = cache_lock_file(slot->lock_path, &slot->lock_fd);
+
+	if (err)
+		return err;
+	if (write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0) {
+		err = errno;
+		fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", slot->lock_path, strerror(err), err);
+		unlink(slot->lock_path);
+		close(slot->lock_fd);
+		slot->lock_fd = -1;
+		return err;
+	}
+	return 0;
+}
+
+static void restore_stdout(struct cache_slot *slot)
+{
+	if (slot->saved_stdout >= 0) {
+		dup2(slot->saved_stdout, STDOUT_FILENO);
+		close(slot->saved_stdout);
+		slot->saved_stdout = -1;
+	}
 }
 
 static int unlock_slot(struct cache_slot *slot, int replace_old_slot)
@@ -280,11 +291,7 @@ static int unlock_slot(struct cache_slot *slot, int replace_old_slot)
 	else
 		err = unlink(slot->lock_path);
 
-	if (slot->saved_stdout >= 0) {
-		dup2(slot->saved_stdout, STDOUT_FILENO);
-		close(slot->saved_stdout);
-		slot->saved_stdout = -1;
-	}
+	restore_stdout(slot);
 
 	if (err)
 		return errno;
@@ -328,11 +335,7 @@ int cache_abandon_fill(void)
 	// caller that could not write it must not try again here.
 	fflush(stdout);
 
-	if (slot->saved_stdout >= 0) {
-		dup2(slot->saved_stdout, STDOUT_FILENO);
-		close(slot->saved_stdout);
-		slot->saved_stdout = -1;
-	}
+	restore_stdout(slot);
 	replay_lock_file(slot);
 	unlink(slot->lock_path);
 	return 1;
@@ -347,7 +350,7 @@ static void publish_slot(struct cache_slot *slot)
 	int err = unlock_slot(slot, 1);
 
 	if (err)
-		log_error("[cgit] Unable to publish cache slot %s: %s (%d)\n", slot->path,
+		fprintf(stderr, "[cgit] Unable to publish cache slot %s: %s (%d)\n", slot->path,
 			strerror(err), err);
 }
 
@@ -405,13 +408,13 @@ static void refresh_slot(struct cache_slot *slot)
 	// lock_slot, the copy already open is served rather than the newer.
 	if (is_modified(slot) || fill_slot(slot)) {
 		unlock_slot(slot, 0);
-		close_lock(slot);
+		close_fd(&slot->lock_fd);
 	} else if (slot->abandoned) {
 		// The abandoned fill answered the visitor itself and removed
 		// the lock file, so only the descriptor is left to clean up.
-		close_lock(slot);
+		close_fd(&slot->lock_fd);
 	} else {
-		close_slot(slot);
+		close_fd(&slot->cache_fd);
 		if (slot->oversized) {
 			// The expired copy goes too, or every later request
 			// would refill it and drop the result again.
@@ -436,11 +439,11 @@ static int process_slot(struct cache_slot *slot)
 		// visitor, and serving the stale copy still open would append
 		// a second page to that answer.
 		if (slot->abandoned) {
-			close_slot(slot);
+			close_fd(&slot->cache_fd);
 			return 0;
 		}
 		err = serve_slot(slot);
-		close_slot(slot);
+		close_fd(&slot->cache_fd);
 		return err;
 	}
 
@@ -448,23 +451,22 @@ static int process_slot(struct cache_slot *slot)
 	// and two popular pages sharing one slot evict each other on every
 	// alternating visit.
 	if (!err)
-		log_error("[cgit] Cache slot %s holds a different key, consider a larger cache-size\n",
+		fprintf(stderr, "[cgit] Cache slot %s holds a different key, consider a larger cache-size\n",
 			slot->path);
 
 	// If any part of creating a slot fails the page is still rendered
 	// straight to the client and the caller is told the request succeeded,
 	// because it did.
-	close_slot(slot);
-	if ((err = lock_slot(slot)) != 0) {
-		log_error("[cgit] Unable to lock slot %s: %s (%d)\n", slot->lock_path, strerror(err), err);
+	close_fd(&slot->cache_fd);
+	if (lock_slot(slot)) {
 		slot->fn();
 		return 0;
 	}
 
 	if ((err = fill_slot(slot)) != 0) {
-		log_error("[cgit] Unable to fill slot %s: %s (%d)\n", slot->lock_path, strerror(err), err);
+		fprintf(stderr, "[cgit] Unable to fill slot %s: %s (%d)\n", slot->lock_path, strerror(err), err);
 		unlock_slot(slot, 0);
-		close_lock(slot);
+		close_fd(&slot->lock_fd);
 		// Rendering again is only right when nothing was delivered,
 		// and an abandoned fill has already sent the error page.
 		if (!slot->abandoned)
@@ -473,7 +475,7 @@ static int process_slot(struct cache_slot *slot)
 	}
 
 	if (slot->abandoned) {
-		close_lock(slot);
+		close_fd(&slot->lock_fd);
 		return 0;
 	}
 
@@ -486,26 +488,11 @@ static int process_slot(struct cache_slot *slot)
 	else
 		publish_slot(slot);
 	err = serve_slot(slot);
-	close_slot(slot);
+	close_fd(&slot->cache_fd);
 	return err;
 }
 
 /*
- * The result lives in a static buffer and is only good until the next call.
- */
-static const char *format_time(const char *format, time_t when)
-{
-	static char buf[64];
-	struct tm tm;
-
-	if (!when)
-		return "-";
-	gmtime_r(&when, &tm);
-	strftime(buf, sizeof(buf) - 1, format, &tm);
-	return buf;
-}
-
-/*
  * The accumulator is unsigned long, so on a 64 bit host this is not the
  * published FNV-1 value. Only slot selection depends on it.
  */
@@ -514,9 +501,6 @@ unsigned long cache_hash_str(const char *str)
 	unsigned long h = FNV_OFFSET;
 	unsigned char *s = (unsigned char *)str;
 
-	if (!s)
-		return h;
-
 	while (*s) {
 		h *= FNV_PRIME;
 		h ^= *s++;
@@ -524,6 +508,16 @@ unsigned long cache_hash_str(const char *str)
 	return h;
 }
 
+/*
+ * Unlike git's strbuf_complete, an empty path gains the slash too, so an empty
+ * cache-root keeps its slots under / rather than in the working directory.
+ */
+static void end_with_slash(struct strbuf *path)
+{
+	if (!path->len || path->buf[path->len - 1] != '/')
+		strbuf_addch(path, '/');
+}
+
 int cache_process(int size, const char *path, const char *key, int ttl, size_t max_bytes,
 	cache_fill_fn fn)
 {
@@ -539,21 +533,14 @@ int cache_process(int size, const char *path, const char *key, int ttl, size_t m
 		return 0;
 	}
 
-	if (!path) {
-		log_error("[cgit] Cache path not specified, caching is disabled\n");
-		fn();
-		return 0;
-	}
-	if (!key)
-		key = "";
 	if (!key_fits_slot(key)) {
-		log_error("[cgit] Cache key too long for a slot, caching is disabled for this request\n");
+		fprintf(stderr, "[cgit] Cache key too long for a slot, caching is disabled for this request\n");
 		fn();
 		return 0;
 	}
 	hash = cache_hash_str(key) % size;
 	strbuf_addstr(&slot_path, path);
-	strbuf_ensure_end(&slot_path, '/');
+	end_with_slash(&slot_path);
 	for (i = 0; i < SLOT_NAME_LEN; i++) {
 		strbuf_addf(&slot_path, "%x", (unsigned char)(hash & 0xf));
 		hash >>= 4;
@@ -577,7 +564,7 @@ int cache_process(int size, const char *path, const char *key, int ttl, size_t m
 	return result;
 }
 
-int cache_ls(const char *path)
+void cache_ls(const char *path)
 {
 	DIR *dir;
 	struct dirent *ent;
@@ -590,42 +577,41 @@ int cache_ls(const char *path)
 	char *nul;
 	int keylen;
 
-	if (!path) {
-		log_error("[cgit] Cache path not specified\n");
-		return -1;
-	}
 	dir = opendir(path);
 	if (!dir) {
 		err = errno;
-		log_error("[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err);
-		return err;
+		fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err);
+		return;
 	}
 	strbuf_addstr(&slot_path, path);
-	strbuf_ensure_end(&slot_path, '/');
+	end_with_slash(&slot_path);
 	prefixlen = slot_path.len;
 	while ((ent = readdir(dir))) {
+		char mtime[64] = "-";
+		struct tm tm;
+
 		if (strlen(ent->d_name) != SLOT_NAME_LEN)
 			continue;
 		strbuf_setlen(&slot_path, prefixlen);
 		strbuf_addstr(&slot_path, ent->d_name);
 		slot.path = slot_path.buf;
 		if ((err = open_slot(&slot)) != 0) {
-			log_error("[cgit] Unable to open %s: %s (%d)\n", slot_path.buf, strerror(err), err);
-			close_slot(&slot);
+			fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", slot_path.buf, strerror(err), err);
+			close_fd(&slot.cache_fd);
 			continue;
 		}
+		if (slot.st.st_mtime && gmtime_r(&slot.st.st_mtime, &tm))
+			strftime(mtime, sizeof(mtime), "%Y-%m-%d %H:%M:%S", &tm);
 		// A truncated or corrupt slot may hold no NUL, so the print is
 		// bounded by what was read and cannot run off the end.
 		nul = memchr(slot.buf, 0, slot.buflen);
 		keylen = nul ? (int)(nul - slot.buf) : slot.buflen;
 		htmlf(
-			"%s %s %10"PRIuMAX" %.*s\n", slot_path.buf,
-			format_time("%Y-%m-%d %H:%M:%S", slot.st.st_mtime),
+			"%s %s %10"PRIuMAX" %.*s\n", slot_path.buf, mtime,
 			(uintmax_t)slot.st.st_size, keylen, slot.buf
 		);
-		close_slot(&slot);
+		close_fd(&slot.cache_fd);
 	}
 	closedir(dir);
 	strbuf_release(&slot_path);
-	return 0;
 }