Re: [PATCH v3 1/2] use HOST_NAME_MAX to size buffers for gethostname(2)

[René Scharfe]

Am 19.04.2017 um 21:09 schrieb Torsten Bögershausen:
On 2017-04-19 19:28, René Scharfe wrote:
[]
One or two minor comments inline
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)

Minor: we need xsize_t here ?
if (st.st_size > xsize_t(st.st_size))

No, xsize_t() would do the same check and die on overflow, and 
pidfile_read() is supposed to handle big pids gracefully.


+               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];

Huh ?
should this be increased, may be in another path ?

It should, but not in this patch.

René