From bf4cd88262afbfcdae29266b51adc5f35901d09c Mon Sep 17 00:00:00 2001 From: Bryce Kwon Date: Sat, 31 Oct 2026 14:48:03 -0700 Subject: Share one lock-file helper between the caches --- source/cache.c | 74 +++++++++++++++++++++++++++++++++++----------------------- source/cache.h | 10 ++++++++ source/cgit.c | 38 +++--------------------------- 3 files changed, 58 insertions(+), 64 deletions(-) (limited to 'source') diff --git a/source/cache.c b/source/cache.c index 6317a12..0a3166d 100644 --- a/source/cache.c +++ b/source/cache.c @@ -198,55 +198,71 @@ static int is_modified(struct cache_slot *slot) * 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; - // Close-on-exec, so that a program the fill runs gets the file only - // as its stdout, and holds the lock no longer than it can write there. - slot->lock_fd = open(slot->lock_path, O_RDWR | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (slot->lock_fd == -1) { + // 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", slot->lock_path, strerror(err), err); + fprintf(stderr, "[cgit] Unable to open %s: %s (%d)\n", path, strerror(err), err); return err; } - // 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 + // 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) { + // 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, silently keeps the slot - // from ever being filled or refreshed and does deserve one. + // 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", slot->lock_path, strerror(err), err); - close(slot->lock_fd); - slot->lock_fd = -1; - return err; + 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 || - write_in_full(slot->lock_fd, slot->key, slot->keylen + 1) < 0 - ) { + 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); diff --git a/source/cache.h b/source/cache.h index b73a6f4..cfe1fbf 100644 --- a/source/cache.h +++ b/source/cache.h @@ -40,4 +40,14 @@ extern int cache_abandon_fill(void); extern unsigned long cache_hash_str(const char *str); +/* + * Open the lock file at path, creating it if need be, then lock it without + * waiting and empty it, leaving the descriptor in *fd. The lock holds until + * the last descriptor on the file closes, and the caller renames or removes + * the file before that. Returns 0, EAGAIN or EWOULDBLOCK when another process + * holds the lock or has renamed the file away, or the errno of the step that + * failed, which has been logged. *fd is -1 on failure. + */ +extern int cache_lock_file(const char *path, int *fd); + #endif // CGIT_CACHE_H diff --git a/source/cgit.c b/source/cgit.c index bccc58f..245e77d 100644 --- a/source/cgit.c +++ b/source/cgit.c @@ -27,7 +27,6 @@ #include "ui-shared.h" #include "ui-snapshot.h" #include "ui-stats.h" -#include // Deliberately small. The whole body is read at once and handed to the auth // filter, so a filter never has to defend itself against a huge or a @@ -329,47 +328,16 @@ static void parse_args(int argc, const char **argv) static int generate_cached_repolist(const char *path, const char *cached_rc) { struct strbuf locked_rc = STRBUF_INIT; - struct stat held, named; int err = 0; int fd; int first; FILE *f; strbuf_addf(&locked_rc, "%s.lock", cached_rc); - fd = open(locked_rc.buf, 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", locked_rc.buf, strerror(err), err); - goto out; - } - 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, silently costs a scan - // on every request and does deserve one. - err = errno; - if (err != EWOULDBLOCK) - fprintf(stderr, "[cgit] Unable to lock %s: %s (%d)\n", locked_rc.buf, strerror(err), err); - close(fd); + err = cache_lock_file(locked_rc.buf, &fd); + if (err) goto out; - } - // The lock landed on whatever inode the path named at open. A holder - // finishing in between renames that inode into place as the live - // list, 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(fd, &held) || - stat(locked_rc.buf, &named) || - held.st_ino != named.st_ino || - held.st_dev != named.st_dev - ) { - err = EAGAIN; - close(fd); - goto out; - } - // 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(fd, 0) < 0 || !(f = fdopen(fd, "w"))) { + if (!(f = fdopen(fd, "w"))) { err = errno; fprintf(stderr, "[cgit] Unable to write %s: %s (%d)\n", locked_rc.buf, strerror(err), err); unlink(locked_rc.buf); -- cgit