Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)
Am 19.04.2017 um 03:28 schrieb Jonathan Nieder:
Show 25 quoted lines
> 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 <l.s.r@web.de>
---
builtin/gc.c | 151 +++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 111 insertions(+), 40 deletions(-)
Show changes to builtin/gc.c +111 −40
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
David Turner
