* [PATCH v2] xgethostname: handle long hostnames
@ 2017-04-17 16:17 David Turner
2017-04-18 1:19 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: David Turner @ 2017-04-17 16:17 UTC (permalink / raw)
To: git; +Cc: peff, l.s.r, jrnieder, David Turner
If the full hostname doesn't fit in the buffer supplied to
gethostname, POSIX does not specify whether the buffer will be
null-terminated, so to be safe, we should do it ourselves. Introduce
new function, xgethostname, which ensures that there is always a \0
at the end of the buffer.
Always use a consistent buffer size when calling xgethostname. We use
HOST_NAME_MAX + 1. When this is unavailable, we fall back to 256, a
decision we previously made in daemon.c, but which we now move to
git-compat-util.h so that it can be used everywhere.
Signed-off-by: David Turner <dturner@twosigma.com>
---
This version addresses some comments by René Scharfe. René, we're
still silently truncating, but as I noted in my message to Jonathan:
Looking at the users of this function, I think most would be happier
with a truncated buffer than an error:
* gc.c: used to see if we are the same machine as the machine that
locked the repo. it is unlikely that two machines have hostnames that
differ only in the 256th-or-above character, and it would be weird to
fail to gc just because our hostname is long.
* fetch-pack.c, receive-pack.c: similar to gc.c; the hostname is a note
in the .keep file for human consumption only
* ident.c: used to make up a fake email address. On my laptop,
gethostname returns "corey" (no domain part), so the email address is
not likely to be valid anyway.
builtin/gc.c | 6 +++---
builtin/receive-pack.c | 4 ++--
daemon.c | 4 ----
fetch-pack.c | 4 ++--
git-compat-util.h | 6 ++++++
ident.c | 4 ++--
wrapper.c | 13 +++++++++++++
7 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index c2c61a57bb..5de0209c59 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -238,7 +238,7 @@ static int need_to_gc(void)
static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
{
static struct lock_file lock;
- char my_host[128];
+ char my_host[HOST_NAME_MAX + 1];
struct strbuf sb = STRBUF_INIT;
struct stat st;
uintmax_t pid;
@@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
/* already locked */
return NULL;
- if (gethostname(my_host, sizeof(my_host)))
+ if (xgethostname(my_host, sizeof(my_host)))
xsnprintf(my_host, sizeof(my_host), "unknown");
pidfile_path = git_pathdup("gc.pid");
fd = hold_lock_file_for_update(&lock, pidfile_path,
LOCK_DIE_ON_ERROR);
if (!force) {
- static char locking_host[128];
+ static char locking_host[HOST_NAME_MAX + 1];
int should_exit;
fp = fopen(pidfile_path, "r");
memset(locking_host, 0, sizeof(locking_host));
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index aca9c33d8d..0ca423a711 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1695,12 +1695,12 @@ static const char *unpack(int err_fd, struct shallow_info *si)
if (status)
return "unpack-objects abnormal exit";
} else {
- char hostname[256];
+ char hostname[HOST_NAME_MAX + 1];
argv_array_pushl(&child.args, "index-pack",
"--stdin", hdr_arg, NULL);
- if (gethostname(hostname, sizeof(hostname)))
+ if (xgethostname(hostname, sizeof(hostname)))
xsnprintf(hostname, sizeof(hostname), "localhost");
argv_array_pushf(&child.args,
"--keep=receive-pack %"PRIuMAX" on %s",
diff --git a/daemon.c b/daemon.c
index 473e6b6b63..1503e1ed6f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -4,10 +4,6 @@
#include "strbuf.h"
#include "string-list.h"
-#ifndef HOST_NAME_MAX
-#define HOST_NAME_MAX 256
-#endif
-
#ifdef NO_INITGROUPS
#define initgroups(x, y) (0) /* nothing */
#endif
diff --git a/fetch-pack.c b/fetch-pack.c
index d07d85ce30..15d59a0440 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -802,8 +802,8 @@ static int get_pack(struct fetch_pack_args *args,
if (args->use_thin_pack)
argv_array_push(&cmd.args, "--fix-thin");
if (args->lock_pack || unpack_limit) {
- char hostname[256];
- if (gethostname(hostname, sizeof(hostname)))
+ char hostname[HOST_NAME_MAX + 1];
+ if (xgethostname(hostname, sizeof(hostname)))
xsnprintf(hostname, sizeof(hostname), "localhost");
argv_array_pushf(&cmd.args,
"--keep=fetch-pack %"PRIuMAX " on %s",
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..bd04564a69 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -884,6 +884,12 @@ static inline size_t xsize_t(off_t len)
__attribute__((format (printf, 3, 4)))
extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
+#ifndef HOST_NAME_MAX
+#define HOST_NAME_MAX 256
+#endif
+
+extern int xgethostname(char *buf, size_t len);
+
/* in ctype.c, for kwset users */
extern const unsigned char tolower_trans_tbl[256];
diff --git a/ident.c b/ident.c
index c0364fe3a1..bea871c8e0 100644
--- a/ident.c
+++ b/ident.c
@@ -120,9 +120,9 @@ static int canonical_name(const char *host, struct strbuf *out)
static void add_domainname(struct strbuf *out, int *is_bogus)
{
- char buf[1024];
+ char buf[HOST_NAME_MAX + 1];
- if (gethostname(buf, sizeof(buf))) {
+ if (xgethostname(buf, sizeof(buf))) {
warning_errno("cannot get host name");
strbuf_addstr(out, "(none)");
*is_bogus = 1;
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..d837417709 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -655,3 +655,16 @@ void sleep_millisec(int millisec)
{
poll(NULL, 0, millisec);
}
+
+int xgethostname(char *buf, size_t len)
+{
+ /*
+ * If the full hostname doesn't fit in buf, POSIX does not
+ * specify whether the buffer will be null-terminated, so to
+ * be safe, do it ourselves.
+ */
+ int ret = gethostname(buf, len);
+ if (!ret)
+ buf[len - 1] = 0;
+ return ret;
+}
--
2.11.GIT
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-17 16:17 [PATCH v2] xgethostname: handle long hostnames David Turner
@ 2017-04-18 1:19 ` Junio C Hamano
2017-04-18 1:41 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2017-04-18 1:19 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, l.s.r, jrnieder
David Turner <dturner@twosigma.com> writes:
> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> ...
> if (!force) {
> - static char locking_host[128];
> + static char locking_host[HOST_NAME_MAX + 1];
> int should_exit;
> fp = fopen(pidfile_path, "r");
> memset(locking_host, 0, sizeof(locking_host));
I compared the result of applying this v2 directly on top of master
and applying René's "Use HOST_NAME_MAX"and then applying your v1.
This hunk is the only difference.
As this locking_host is used like so in the later part of the code:
time(NULL) - st.st_mtime <= 12 * 3600 &&
fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
/* be gentle to concurrent "gc" on remote hosts */
(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
the format "%127c" gives us an inconsistent resulting code.
Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
comparing it with locking_host, so perhaps we'd need to take this
version to size locking_host to also HOST_NAME_MAX + 1, and then
scan with %255c (but then shouldn't we scan with %256c instead? I
am not sure where these +1 comes from).
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 1:19 ` Junio C Hamano
@ 2017-04-18 1:41 ` Junio C Hamano
2017-04-18 16:07 ` René Scharfe
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2017-04-18 1:41 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, l.s.r, jrnieder
Junio C Hamano <gitster@pobox.com> writes:
> David Turner <dturner@twosigma.com> writes:
>
>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>> ...
>> if (!force) {
>> - static char locking_host[128];
>> + static char locking_host[HOST_NAME_MAX + 1];
>> int should_exit;
>> fp = fopen(pidfile_path, "r");
>> memset(locking_host, 0, sizeof(locking_host));
>
> I compared the result of applying this v2 directly on top of master
> and applying René's "Use HOST_NAME_MAX"and then applying your v1.
> This hunk is the only difference.
>
> As this locking_host is used like so in the later part of the code:
>
> time(NULL) - st.st_mtime <= 12 * 3600 &&
> fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> /* be gentle to concurrent "gc" on remote hosts */
> (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
>
> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
> the format "%127c" gives us an inconsistent resulting code.
>
> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
> comparing it with locking_host, so perhaps we'd need to take this
> version to size locking_host to also HOST_NAME_MAX + 1, and then
> scan with %255c (but then shouldn't we scan with %256c instead? I
> am not sure where these +1 comes from).
That is, something along this line...
builtin/gc.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index be75508292..4f85610d87 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
LOCK_DIE_ON_ERROR);
if (!force) {
static char locking_host[HOST_NAME_MAX + 1];
+ static char *scan_fmt;
int should_exit;
+
+ if (!scan_fmt)
+ scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX, HOST_NAME_MAX);
fp = fopen(pidfile_path, "r");
memset(locking_host, 0, sizeof(locking_host));
should_exit =
@@ -256,7 +260,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 &&
/* be gentle to concurrent "gc" on remote hosts */
(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
if (fp != NULL)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 1:41 ` Junio C Hamano
@ 2017-04-18 16:07 ` René Scharfe
2017-04-18 16:17 ` Jeff King
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: René Scharfe @ 2017-04-18 16:07 UTC (permalink / raw)
To: Junio C Hamano, David Turner; +Cc: git, peff, jrnieder, Duy Nguyen
Am 18.04.2017 um 03:41 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> David Turner <dturner@twosigma.com> writes:
>>
>>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
>>> ...
>>> if (!force) {
>>> - static char locking_host[128];
>>> + static char locking_host[HOST_NAME_MAX + 1];
>>> int should_exit;
>>> fp = fopen(pidfile_path, "r");
>>> memset(locking_host, 0, sizeof(locking_host));
>>
>> I compared the result of applying this v2 directly on top of master
>> and applying René's "Use HOST_NAME_MAX"and then applying your v1.
>> This hunk is the only difference.
>>
>> As this locking_host is used like so in the later part of the code:
>>
>> time(NULL) - st.st_mtime <= 12 * 3600 &&
>> fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
>> /* be gentle to concurrent "gc" on remote hosts */
>> (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
>>
>> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
>> the format "%127c" gives us an inconsistent resulting code.
Oh, missed that. Thanks for catching it!
>> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
>> comparing it with locking_host, so perhaps we'd need to take this
>> version to size locking_host to also HOST_NAME_MAX + 1, and then
>> scan with %255c (but then shouldn't we scan with %256c instead? I
>> am not sure where these +1 comes from).
>
> That is, something along this line...
>
> builtin/gc.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index be75508292..4f85610d87 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> LOCK_DIE_ON_ERROR);
> if (!force) {
> static char locking_host[HOST_NAME_MAX + 1];
> + static char *scan_fmt;
> int should_exit;
> +
> + if (!scan_fmt)
> + scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX, HOST_NAME_MAX);
> fp = fopen(pidfile_path, "r");
> memset(locking_host, 0, sizeof(locking_host));
> should_exit =
> @@ -256,7 +260,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 &&
> /* be gentle to concurrent "gc" on remote hosts */
> (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
> if (fp != NULL)
>
How important is it to scan the whole file in one call? We could split
it up like this and use a strbuf to handle host names of any length. We
need to be permissive here to allow machines with different values for
HOST_NAME_MAX to work with the same file on a network file system, so
this would have to be the first patch, right?
NB: That && cascade has enough meat for a whole function.
René
---
builtin/gc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 1fca84c19d..d5e880028e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,10 +251,9 @@ 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];
+ static struct strbuf locking_host = STRBUF_INIT;
int should_exit;
fp = fopen(pidfile_path, "r");
- memset(locking_host, 0, sizeof(locking_host));
should_exit =
fp != NULL &&
!fstat(fileno(fp), &st) &&
@@ -268,9 +267,10 @@ 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, "%"SCNuMAX" ", &pid) == 1 &&
+ !strbuf_getwholeline(&locking_host, fp, '\0') &&
/* be gentle to concurrent "gc" on remote hosts */
- (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
+ (strcmp(locking_host.buf, my_host) || !kill(pid, 0) || errno == EPERM);
if (fp != NULL)
fclose(fp);
if (should_exit) {
@@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
rollback_lock_file(&lock);
*ret_pid = pid;
free(pidfile_path);
- return locking_host;
+ return locking_host.buf;
}
}
--
2.12.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 16:07 ` René Scharfe
@ 2017-04-18 16:17 ` Jeff King
2017-04-19 1:47 ` Junio C Hamano
2017-04-18 17:52 ` David Turner
2017-04-19 1:49 ` Junio C Hamano
2 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2017-04-18 16:17 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, David Turner, git, jrnieder, Duy Nguyen
On Tue, Apr 18, 2017 at 06:07:43PM +0200, René Scharfe wrote:
> > - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> > + fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
> > /* be gentle to concurrent "gc" on remote hosts */
> > (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
> > if (fp != NULL)
> >
>
> How important is it to scan the whole file in one call? We could split
> it up like this and use a strbuf to handle host names of any length. We
> need to be permissive here to allow machines with different values for
> HOST_NAME_MAX to work with the same file on a network file system, so
> this would have to be the first patch, right?
I doubt that doing it in one call matters. It's not like stdio promises
us any atomicity in the first place.
> - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> + fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
> + !strbuf_getwholeline(&locking_host, fp, '\0') &&
I don't think there is anything wrong with using fscanf here, but it has
enough pitfalls in general that I don't really like its use as a parser
(and the general lack of it in Git's code base seems to agree).
I wonder if this should just read a line (or the whole file) into a
strbuf and parse it there. That would better match our usual style, I
think.
I can live with it either way.
> NB: That && cascade has enough meat for a whole function.
Yeah.
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 16:07 ` René Scharfe
2017-04-18 16:17 ` Jeff King
@ 2017-04-18 17:52 ` David Turner
2017-04-19 3:49 ` Junio C Hamano
2017-04-19 1:49 ` Junio C Hamano
2 siblings, 1 reply; 9+ messages in thread
From: David Turner @ 2017-04-18 17:52 UTC (permalink / raw)
To: 'René Scharfe', Junio C Hamano
Cc: git, peff, jrnieder, Duy Nguyen
> -----Original Message-----
> From: René Scharfe [mailto:l.s.r@web.de]
> Sent: Tuesday, April 18, 2017 12:08 PM
> To: Junio C Hamano <gitster@pobox.com>; David Turner
...
> >> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are comparing
> >> it with locking_host, so perhaps we'd need to take this version to
> >> size locking_host to also HOST_NAME_MAX + 1, and then scan with %255c
> >> (but then shouldn't we scan with %256c instead? I am not sure where
> >> these +1 comes from).
> >
> > That is, something along this line...
> >
> > builtin/gc.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c index be75508292..4f85610d87
> > 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> > LOCK_DIE_ON_ERROR);
> > if (!force) {
> > static char locking_host[HOST_NAME_MAX + 1];
> > + static char *scan_fmt;
> > int should_exit;
> > +
> > + if (!scan_fmt)
> > + scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX,
> HOST_NAME_MAX);
> > fp = fopen(pidfile_path, "r");
> > memset(locking_host, 0, sizeof(locking_host));
> > should_exit =
> > @@ -256,7 +260,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 &&
> > /* be gentle to concurrent "gc" on remote hosts */
> > (strcmp(locking_host, my_host) || !kill(pid, 0) || errno
> == EPERM);
> > if (fp != NULL)
> >
>
> How important is it to scan the whole file in one call? We could split it up like
> this and use a strbuf to handle host names of any length. We need to be
> permissive here to allow machines with different values for HOST_NAME_MAX
> to work with the same file on a network file system, so this would have to be the
> first patch, right?
If the writer has the smaller HOST_NAME_MAX, this will work fine. If the reader
has the smaller HOST_NAME_MAX, and the writer's actual value is too long,
then there's no way the strcmp would succeed anyway. So I don't think we need
to worry about it.
> NB: That && cascade has enough meat for a whole function.
+1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 16:17 ` Jeff King
@ 2017-04-19 1:47 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2017-04-19 1:47 UTC (permalink / raw)
To: Jeff King; +Cc: René Scharfe, David Turner, git, jrnieder, Duy Nguyen
Jeff King <peff@peff.net> writes:
> I doubt that doing it in one call matters. It's not like stdio promises
> us any atomicity in the first place.
>
>> - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
>> + fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
>> + !strbuf_getwholeline(&locking_host, fp, '\0') &&
>
> I don't think there is anything wrong with using fscanf here, but it has
> enough pitfalls in general that I don't really like its use as a parser
> (and the general lack of it in Git's code base seems to agree).
>
> I wonder if this should just read a line (or the whole file) into a
> strbuf and parse it there. That would better match our usual style, I
> think.
Yeah, I think it would be a good change.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 16:07 ` René Scharfe
2017-04-18 16:17 ` Jeff King
2017-04-18 17:52 ` David Turner
@ 2017-04-19 1:49 ` Junio C Hamano
2 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2017-04-19 1:49 UTC (permalink / raw)
To: René Scharfe; +Cc: David Turner, git, peff, jrnieder, Duy Nguyen
René Scharfe <l.s.r@web.de> writes:
> How important is it to scan the whole file in one call? We could split
> it up like this and use a strbuf to handle host names of any length. We
> need to be permissive here to allow machines with different values for
> HOST_NAME_MAX to work with the same file on a network file system, so
> this would have to be the first patch, right?
Absolutely. FWIW, I agree with Peff that we do not need to use
fscanf here; just reading a line into strbuf and picking pieces
would be sufficient.
> NB: That && cascade has enough meat for a whole function.
True, too.
>
> René
>
> ---
> builtin/gc.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 1fca84c19d..d5e880028e 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -251,10 +251,9 @@ 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];
> + static struct strbuf locking_host = STRBUF_INIT;
> int should_exit;
> fp = fopen(pidfile_path, "r");
> - memset(locking_host, 0, sizeof(locking_host));
> should_exit =
> fp != NULL &&
> !fstat(fileno(fp), &st) &&
> @@ -268,9 +267,10 @@ 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, "%"SCNuMAX" ", &pid) == 1 &&
> + !strbuf_getwholeline(&locking_host, fp, '\0') &&
> /* be gentle to concurrent "gc" on remote hosts */
> - (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
> + (strcmp(locking_host.buf, my_host) || !kill(pid, 0) || errno == EPERM);
> if (fp != NULL)
> fclose(fp);
> if (should_exit) {
> @@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
> rollback_lock_file(&lock);
> *ret_pid = pid;
> free(pidfile_path);
> - return locking_host;
> + return locking_host.buf;
> }
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] xgethostname: handle long hostnames
2017-04-18 17:52 ` David Turner
@ 2017-04-19 3:49 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2017-04-19 3:49 UTC (permalink / raw)
To: David Turner; +Cc: 'René Scharfe', git, peff, jrnieder, Duy Nguyen
David Turner <David.Turner@twosigma.com> writes:
> If the writer has the smaller HOST_NAME_MAX, this will work fine. If the reader
> has the smaller HOST_NAME_MAX, and the writer's actual value is too long,
> then there's no way the strcmp would succeed anyway. So I don't think we need
> to worry about it.
Hmph, I have to agree with that reasoning, only because the value we
read into locking_host[] is not used for error reporting at all. I
would have insisted to read what is on the filesystem anyway if that
were not the case.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-04-19 3:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-17 16:17 [PATCH v2] xgethostname: handle long hostnames David Turner
2017-04-18 1:19 ` Junio C Hamano
2017-04-18 1:41 ` Junio C Hamano
2017-04-18 16:07 ` René Scharfe
2017-04-18 16:17 ` Jeff King
2017-04-19 1:47 ` Junio C Hamano
2017-04-18 17:52 ` David Turner
2017-04-19 3:49 ` Junio C Hamano
2017-04-19 1:49 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.