Re: [PATCH 1/2] builtin/maintenance: fix locking with "--detach"

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

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3a71e314c9..09cb92ac97 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1810,10 +1810,32 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts,
>  				   TASK_PHASE_FOREGROUND))
>  			result = 1;
>  
> -	/* Failure to daemonize is ok, we'll continue in foreground. */
>  	if (opts->detach > 0) {
> +		pid_t child_pid;
> +
>  		trace2_region_enter("maintenance", "detach", the_repository);
> -		daemonize();
> +
> +		child_pid = daemonize_without_exit();
> +		if (!child_pid) {
> +			/*
> +			 * We're in the child process, so we take ownership of
> +			 * the lockfile.
> +			 */
> +			lock_file_reassign_owner(&lk, getpid());
> +		} else if (child_pid > 0) {
> +			/*
> +			 * We're in the parent process, so we assign ownership
> +			 * of the lockfile to the child and then exit immediately.
> +			 */
> +			lock_file_reassign_owner(&lk, child_pid);
> +			exit(0);

The point of reassigning the owner to somebody else is so that we
won't clean them when we exit as the tempfile.c::remove_tempfile()
function checks the "owner" is "me" and refrains from unlinking
those that do not belong to us, so there is nothing wrong in this
code, but this somehow felt awkward.  In a sense, child_pid here
does not have to be what fork() returned but anything that is not
our own pid.  Perhaps "we assign ... to the child" -> "we relinquish
... to prevent us removing upon exiting" would convey the intention
better?  I dunno.

> -int daemonize(void)
> +pid_t daemonize_without_exit(void)
>  {
>  #ifdef NO_POSIX_GOODIES
>  	errno = ENOSYS;
>  	return -1;
>  #else
> -	switch (fork()) {
> -		case 0:
> -			break;
> -		case -1:
> -			die_errno(_("fork failed"));
> -		default:
> -			exit(0);
> -	}
> +	pid_t pid = fork();
> +	if (pid < 0)
> +		return -1;
> +	if (pid > 0)
> +		return pid;
> +
>  	if (setsid() == -1)
>  		die_errno(_("setsid failed"));
>  	close(0);
> @@ -2180,6 +2178,21 @@ int daemonize(void)
>  #endif
>  }
>  
> +int daemonize(void)
> +{
> +#ifdef NO_POSIX_GOODIES
> +	errno = ENOSYS;
> +	return -1;
> +#else
> +	pid_t pid = daemonize_without_exit();
> +	if (pid < 0)
> +		die_errno(_("fork failed"));
> +	if (pid > 0)
> +		exit(0);
> +	return 0;
> +#endif
> +}

I was hoping that we can do without the #ifdef in this caller as
daemonize_without_exit() already has exactly the same condtional
compilation.  If the NO_POSIX_GOODIES side can just return silently
wit ENOSYS, shouldn't the callers be also fine if we return failure
instead of calling die_errno(_("fork failed")), I have to wonder.

But because (1) as long as we have to call die_errno() here, we must
keep the conditional compilation in daemonize() as well as
daemonize_without_exit(), and (2) changing what the callers get when
fork failed here is totally outside of this topic, I would say that
the code around here is good as-is.