Re: [PATCH 2/2] server-info.c: remove temporary info files on exit
From: Jeff King <hidden>
Date: 2024-06-08 10:25:03
On Thu, Jun 06, 2024 at 06:19:31PM -0400, Taylor Blau wrote:
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).
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
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...
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