Re: [PATCH v2] xgethostname: handle long hostnames

["René Scharfe" <l.s.r@web.de>]

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