Re: [PATCH v2] xgethostname: handle long hostnames

[Junio C Hamano <gitster@pobox.com>]

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)