Re: [PATCH 2/2] server-info.c: remove temporary info files on exit

From: Jeff King <peff@peff.net>
Date: Jun 8, 2024, 10:25 UTC
Message-ID: <20240608102502.GC2659849@coredump.intra.peff.net>
In-Reply-To: <2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com>

On Thu, Jun 06, 2024 at 06:19:31PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> The update_info_file() function within server-info.c is responsible for
> moving the info/refs and info/packs files around when updating server
> info.
> 
> These updates are staged into a temporary file and then moved into place
> atomically to avoid race conditions when reading those files. However,
> the temporary file used to stage these changes is managed outside of the
> tempfile.h API, and thus survives process death.
> 
> Manage these files instead with the tempfile.h API so that they are
> automatically cleaned up upon abnormal process death.

Makes sense. I was going to suggest that these could even be lockfiles, but it is intentional to let two simultaneous processes race (with an atomic last-one-wins result). See d38379ece9 (make update-server-info more robust, 2014-09-13).

> Unfortunately, and unlike in the previous step, there isn't a
> straightforward way to inject a failure into the update-server-info step
> that causes us to die() rather than take the cleanup path in label
> 'out', hence the lack of a test here.

That sounds like a challenge. ;)

  $ echo garbage >.git/packed-refs
  $ git update-server-info
  fatal: unexpected line in .git/packed-refs: garbage
  $ ls .git/info/
  exclude  refs  refs_QYvQGb

I don't know if it's worth adding such a test. It seems rather brittle to assume that we'd die() here (let alone that we are using the files backend at all).

Show 11 quoted lines

> @@ -86,13 +86,12 @@ static int update_info_file(char *path,
>  	};
>  
>  	safe_create_leading_directories(path);
> -	fd = git_mkstemp_mode(tmp, 0666);
> -	if (fd < 0)
> +	f = mks_tempfile_m(tmp, 0666);
> +	if (!f)
>  		goto out;
> -	to_close = uic.cur_fp = fdopen(fd, "w");
> +	uic.cur_fp = fdopen_tempfile(f, "w");

OK, good, fdopen_tempfile() means that the FILE handle is owned by the tempfile, too.

Show 6 quoted lines

> @@ -121,27 +120,22 @@ static int update_info_file(char *path,
>  	}
>  
>  	uic.cur_fp = NULL;
> -	if (fclose(to_close))
> -		goto out;

And we don't need to fclose() anymore since the tempfile code handles that for us. Nice.

Show 12 quoted lines

>  	if (uic_is_stale(&uic)) {
> -		if (adjust_shared_perm(tmp) < 0)
> +		if (adjust_shared_perm(get_tempfile_path(f)) < 0)
>  			goto out;
> -		if (rename(tmp, path) < 0)
> +		if (rename_tempfile(&f, path) < 0)
>  			goto out;
>  	} else {
> -		unlink(tmp);
> +		delete_tempfile(&f);
>  	}
>  	ret = 0;

OK, so we always rename or delete here, unless we jumped to the error path...

Show 11 quoted lines

>  out:
>  	if (ret) {
>  		error_errno("unable to update %s", path);
> -		if (uic.cur_fp)
> -			fclose(uic.cur_fp);
> -		else if (fd >= 0)
> -			close(fd);
> -		unlink(tmp);
> +		if (f)
> +			delete_tempfile(&f);
>  	}

And here we do an explicit delete, which is good for a lib-ified world where the process doesn't just exit immediately.

I think you could actually call delete_tempfile() unconditionally, even outside the "if (ret)" block. It is a noop for a NULL tempfile (so OK even if we jump to "out" before opening it). And a renamed tempfile goes back to NULL as well.

I.e., one of the advantages to using the tempfile interface is that it's always in a consistent state, and you just use delete() on exit, like we do strbuf_release().

That said, it's a pretty minor style question, and I don't think is worth a re-roll.

-Peff