Re: [PATCH 2/2] server-info.c: remove temporary info files on exit
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
Taylor Blau
