Re: [PATCH v2] xgethostname: handle long hostnames
- From
René Scharfe <l.s.r@web.de>
- Date
- Apr 18, 2017, 16:07 UTC
- Message-ID
- <281d0843-d48a-b7ab-737b-b9528689d44e@web.de>
- In-Reply-To
- <xmqqwpaih4q2.fsf@gitster.mtv.corp.google.com>
Am 18.04.2017 um 03:41 schrieb Junio C Hamano:
Show 26 quoted lines
> 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!
Show 37 quoted lines
>> 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