tempfile: do not delete tempfile on failed close
When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) || close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King committed
Sep 5, 2017 at 08:14 UTC
49bd0fc2220eef17d8f5fd3ee76e391d03df8a6d
7 files changed
+40
-37
diff.c
+1
-1
@@ -3739,7 +3739,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
3739
size = buf.len;
3740
}
3741
if (write_in_full(fd, blob, size) != size ||
3742
- close_tempfile(&temp->tempfile))
3742
+ close_tempfile_gently(&temp->tempfile))
3743
die_errno("unable to write temp-file");
3744
temp->name = get_tempfile_path(&temp->tempfile);
3745
oid_to_hex_r(temp->hex, oid);
gpg-interface.c
+1
-1
@@ -210,7 +210,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
210
if (fd < 0)
211
return error_errno(_("could not create temporary file"));
212
if (write_in_full(fd, signature, signature_size) < 0 ||
213
- close_tempfile(&temp) < 0) {
213
+ close_tempfile_gently(&temp) < 0) {
214
error_errno(_("failed writing detached signature to '%s'"),
215
temp.filename.buf);
216
delete_tempfile(&temp);
lockfile.h
+7
-1
@@ -246,7 +246,13 @@ extern char *get_locked_file_path(struct lock_file *lk);
246
*/
247
static inline int close_lock_file(struct lock_file *lk)
248
{
249
- return close_tempfile(&lk->tempfile);
249
+ int ret = close_tempfile_gently(&lk->tempfile);
250
+ if (ret) {
251
+ int saved_errno = errno;
252
+ delete_tempfile(&lk->tempfile);
253
+ errno = saved_errno;
254
+ }
255
+ return ret;
256
}
257
258
/*
read-cache.c
+5
-2
@@ -2309,8 +2309,11 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2309
2310
if (ce_flush(&c, newfd, istate->sha1))
2311
return -1;
2312
- if (close_tempfile(tempfile))
2313
- return error(_("could not close '%s'"), tempfile->filename.buf);
2312
+ if (close_tempfile_gently(tempfile)) {
2313
+ error(_("could not close '%s'"), tempfile->filename.buf);
2314
+ delete_tempfile(tempfile);
2315
+ return -1;
2316
+ }
2317
if (stat(tempfile->filename.buf, &st))
2318
return -1;
2319
istate->timestamp.sec = (unsigned int)st.st_mtime;
shallow.c
+1
-1
@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
296
fd = xmks_tempfile(&temp, git_path("shallow_XXXXXX"));
297
298
if (write_in_full(fd, sb.buf, sb.len) != sb.len ||
299
- close_tempfile(&temp) < 0)
299
+ close_tempfile_gently(&temp) < 0)
300
die_errno("failed to write to %s",
301
get_tempfile_path(&temp));
302
strbuf_release(&sb);
tempfile.c
+12
-19
@@ -30,13 +30,12 @@
30
* `fdopen_tempfile()` has been called on the object
31
* - `owner` holds the PID of the process that created the file
32
*
33
- * - Active, file closed (after successful `close_tempfile()`). Same
33
+ * - Active, file closed (after `close_tempfile_gently()`). Same
34
* as the previous state, except that the temporary file is closed,
35
* `fd` is -1, and `fp` is `NULL`.
36
*
37
- * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, a
38
- * failed attempt to create a temporary file, or a failed
39
- * `close_tempfile()`). In this state:
37
+ * - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a
38
+ * failed attempt to create a temporary file). In this state:
39
*
40
* - `active` is unset
41
* - `filename` is empty (usually, though there are transitory
@@ -235,7 +234,7 @@ FILE *get_tempfile_fp(struct tempfile *tempfile)
234
return tempfile->fp;
235
}
236
238
-int close_tempfile(struct tempfile *tempfile)
237
+int close_tempfile_gently(struct tempfile *tempfile)
238
{
239
int fd = tempfile->fd;
240
FILE *fp = tempfile->fp;
@@ -258,14 +257,7 @@ int close_tempfile(struct tempfile *tempfile)
257
err = close(fd);
258
}
259
261
- if (err) {
262
- int save_errno = errno;
263
- delete_tempfile(tempfile);
264
- errno = save_errno;
265
- return -1;
266
- }
267
-
268
- return 0;
260
+ return err ? -1 : 0;
261
}
262
263
int reopen_tempfile(struct tempfile *tempfile)
@@ -283,8 +275,10 @@ int rename_tempfile(struct tempfile *tempfile, const char *path)
275
if (!tempfile->active)
276
die("BUG: rename_tempfile called for inactive object");
277
286
- if (close_tempfile(tempfile))
278
+ if (close_tempfile_gently(tempfile)) {
279
+ delete_tempfile(tempfile);
280
return -1;
281
+ }
282
283
if (rename(tempfile->filename.buf, path)) {
284
int save_errno = errno;
@@ -303,9 +297,8 @@ void delete_tempfile(struct tempfile *tempfile)
297
if (!tempfile->active)
298
return;
299
306
- if (!close_tempfile(tempfile)) {
307
- unlink_or_warn(tempfile->filename.buf);
308
- tempfile->active = 0;
309
- strbuf_reset(&tempfile->filename);
310
- }
300
+ close_tempfile_gently(tempfile);
301
+ unlink_or_warn(tempfile->filename.buf);
302
+ tempfile->active = 0;
303
+ strbuf_reset(&tempfile->filename);
304
}
tempfile.h
+13
-12
@@ -47,7 +47,7 @@
47
* control of the file.
48
*
49
* * Close the file descriptor without removing or renaming the
50
- * temporary file by calling `close_tempfile()`, and later call
50
+ * temporary file by calling `close_tempfile_gently()`, and later call
51
* `delete_tempfile()` or `rename_tempfile()`.
52
*
53
* Even after the temporary file is renamed or deleted, the `tempfile`
@@ -59,7 +59,7 @@
59
* and remove the temporary file.
60
*
61
* If you need to close the file descriptor yourself, do so by calling
62
- * `close_tempfile()`. You should never call `close(2)` or `fclose(3)`
62
+ * `close_tempfile_gently()`. You should never call `close(2)` or `fclose(3)`
63
* yourself, otherwise the `struct tempfile` structure would still
64
* think that the file descriptor needs to be closed, and a later
65
* cleanup would result in duplicate calls to `close(2)`. Worse yet,
@@ -74,9 +74,10 @@
74
* `create_tempfile()` returns a file descriptor on success or -1 on
75
* failure. On errors, `errno` describes the reason for failure.
76
*
77
- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile()`
78
- * return 0 on success. On failure they set `errno` appropriately, do
79
- * their best to delete the temporary file, and return -1.
77
+ * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
78
+ * return 0 on success. On failure they set `errno` appropriately and return
79
+ * -1. `delete` and `rename` (but not `close`) do their best to delete the
80
+ * temporary file before returning.
81
*/
82
83
struct tempfile {
@@ -203,7 +204,7 @@ static inline int xmks_tempfile(struct tempfile *tempfile,
204
/*
205
* Associate a stdio stream with the temporary file (which must still
206
* be open). Return `NULL` (*without* deleting the file) on error. The
206
- * stream is closed automatically when `close_tempfile()` is called or
207
+ * stream is closed automatically when `close_tempfile_gently()` is called or
208
* when the file is deleted or renamed.
209
*/
210
extern FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode);
@@ -226,20 +227,20 @@ extern FILE *get_tempfile_fp(struct tempfile *tempfile);
227
* If the temporary file is still open, close it (and the file pointer
228
* too, if it has been opened using `fdopen_tempfile()`) without
229
* deleting the file. Return 0 upon success. On failure to `close(2)`,
229
- * return a negative value and delete the file. Usually
230
- * `delete_tempfile()` or `rename_tempfile()` should eventually be
231
- * called if `close_tempfile()` succeeds.
230
+ * return a negative value. Usually `delete_tempfile()` or `rename_tempfile()`
231
+ * should eventually be called regardless of whether `close_tempfile_gently()`
232
+ * succeeds.
233
*/
233
-extern int close_tempfile(struct tempfile *tempfile);
234
+extern int close_tempfile_gently(struct tempfile *tempfile);
235
236
/*
237
* Re-open a temporary file that has been closed using
237
- * `close_tempfile()` but not yet deleted or renamed. This can be used
238
+ * `close_tempfile_gently()` but not yet deleted or renamed. This can be used
239
* to implement a sequence of operations like the following:
240
*
241
* * Create temporary file.
242
*
242
- * * Write new contents to file, then `close_tempfile()` to cause the
243
+ * * Write new contents to file, then `close_tempfile_gently()` to cause the
244
* contents to be written to disk.
245
*
246
* * Pass the name of the temporary file to another program to allow