Am 19.04.2017 um 03:28 schrieb Jonathan Nieder: > David Turner wrote: >> @@ -274,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* >> ret_pid) >> * running. >> */ >> time(NULL) - st.st_mtime <= 12 * 3600 && >> - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 >> && >> + fscanf(fp, scan_fmt, &pid, locking_host) == 2 && > > I hoped this could be simplified since HOST_NAME_MAX is a numeric literal, > using the double-expansion trick: > > #define STR_(s) # s > #define STR(s) STR_(s) > > fscanf(fp, "%" SCNuMAX " %" STR(HOST_NAME_MAX) "c", > &pid, locking_host); > > Unfortunately, I don't think there's anything stopping a platform from > defining > > #define HOST_NAME_MAX 0x100 > > which would break that. > > So this run-time calculation appears to be necessary.
I had another look at this last night and cooked up the following patch. Might have gone overboard with it.. -- >8 -- Subject: [PATCH] gc: support arbitrary hostnames and pids in lock_repo_for_gc() git gc writes its pid and hostname into a pidfile to prevent concurrent garbage collection. Repositories may be shared between systems with different limits for host name length and different pid ranges. Use a strbuf to store the file contents to allow for arbitrarily long hostnames and pids to be shown to the user on early abort. Signed-off-by: Rene Scharfe <[email protected]> --- builtin/gc.c | 151 +++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 111 insertions(+), 40 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 2daede7820..4c1c01e87d 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -228,21 +228,99 @@ static int need_to_gc(void) return 1; } +struct pidfile { + struct strbuf buf; + char *hostname; +}; + +#define PIDFILE_INIT { STRBUF_INIT } + +static void pidfile_release(struct pidfile *pf) +{ + pf->hostname = NULL; + strbuf_release(&pf->buf); +} + +static int pidfile_read(struct pidfile *pf, const char *path, + unsigned int max_age_seconds) +{ + int fd; + struct stat st; + ssize_t len; + char *space; + int rc = -1; + + fd = open(path, O_RDONLY); + if (fd < 0) + return rc; + + if (fstat(fd, &st)) + goto out; + if (time(NULL) - st.st_mtime > max_age_seconds) + goto out; + if (st.st_size > (size_t)st.st_size) + goto out; + + len = strbuf_read(&pf->buf, fd, st.st_size); + if (len < 0) + goto out; + + space = strchr(pf->buf.buf, ' '); + if (!space) { + pidfile_release(pf); + goto out; + } + pf->hostname = space + 1; + *space = '\0'; + + rc = 0; +out: + close(fd); + return rc; +} + +static int parse_pid(const char *value, pid_t *ret) +{ + if (value && *value) { + char *end; + intmax_t val; + + errno = 0; + val = strtoimax(value, &end, 0); + if (errno == ERANGE) + return 0; + if (*end) + return 0; + if (labs(val) > maximum_signed_value_of_type(pid_t)) { + errno = ERANGE; + return 0; + } + *ret = val; + return 1; + } + errno = EINVAL; + return 0; +} + +static int pidfile_process_exists(const struct pidfile *pf) +{ + pid_t pid; + return parse_pid(pf->buf.buf, &pid) && + (!kill(pid, 0) || errno == EPERM); +} + /* return NULL on success, else hostname running the gc */ -static const char *lock_repo_for_gc(int force, pid_t* ret_pid) +static int lock_repo_for_gc(int force, struct pidfile *pf) { static struct lock_file lock; char my_host[128]; struct strbuf sb = STRBUF_INIT; - struct stat st; - uintmax_t pid; - FILE *fp; int fd; char *pidfile_path; if (is_tempfile_active(&pidfile)) /* already locked */ - return NULL; + return 0; if (gethostname(my_host, sizeof(my_host))) xsnprintf(my_host, sizeof(my_host), "unknown"); @@ -251,34 +329,27 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) fd = hold_lock_file_for_update(&lock, pidfile_path, LOCK_DIE_ON_ERROR); if (!force) { - static char locking_host[128]; - int should_exit; - fp = fopen(pidfile_path, "r"); - memset(locking_host, 0, sizeof(locking_host)); - should_exit = - fp != NULL && - !fstat(fileno(fp), &st) && - /* - * 12 hour limit is very generous as gc should - * never take that long. On the other hand we - * don't really need a strict limit here, - * running gc --auto one day late is not a big - * problem. --force can be used in manual gc - * after the user verifies that no gc is - * running. - */ - time(NULL) - st.st_mtime <= 12 * 3600 && - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 && + /* + * 12 hour limit is very generous as gc should + * never take that long. On the other hand we + * don't really need a strict limit here, + * running gc --auto one day late is not a big + * problem. --force can be used in manual gc + * after the user verifies that no gc is + * running. + */ + const unsigned max_age_seconds = 12 * 3600; + + if (!pidfile_read(pf, pidfile_path, max_age_seconds)) { /* be gentle to concurrent "gc" on remote hosts */ - (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM); - if (fp != NULL) - fclose(fp); - if (should_exit) { - if (fd >= 0) - rollback_lock_file(&lock); - *ret_pid = pid; - free(pidfile_path); - return locking_host; + if (strcmp(pf->hostname, my_host) || + pidfile_process_exists(pf)) { + if (fd >= 0) + rollback_lock_file(&lock); + free(pidfile_path); + return -1; + } + pidfile_release(pf); } } @@ -289,7 +360,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) commit_lock_file(&lock); register_tempfile(&pidfile, pidfile_path); free(pidfile_path); - return NULL; + return 0; } static int report_last_gc_error(void) @@ -344,8 +415,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) int auto_gc = 0; int quiet = 0; int force = 0; - const char *name; - pid_t pid; + struct pidfile pf = PIDFILE_INIT; int daemonized = 0; struct option builtin_gc_options[] = { @@ -420,12 +490,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix) } else add_repack_all_option(); - name = lock_repo_for_gc(force, &pid); - if (name) { - if (auto_gc) + if (lock_repo_for_gc(force, &pf)) { + if (auto_gc) { + pidfile_release(&pf); return 0; /* be quiet on --auto */ - die(_("gc is already running on machine '%s' pid %"PRIuMAX" (use --force if not)"), - name, (uintmax_t)pid); + } + die(_("gc is already running on machine '%s' pid %s (use --force if not)"), + pf.hostname, pf.buf.buf); } if (daemonized) { -- 2.12.2
