mirror/ git
1
0
Fork 0
mirror of https://github.com/git/git.git synced 2024-05-12 05:56:09 +02:00
Code
git/tempfile.c

314 lines
8.1 KiB
C
Raw Normal View History

tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
/*
* State diagram and cleanup
* -------------------------
*
* If the program exits while a temporary file is active, we want to
* make sure that we remove it. This is done by remembering the active
* temporary files in a linked list, `tempfile_list`. An `atexit(3)`
* handler and a signal handler are registered, to clean up any active
* temporary files.
*
* Because the signal handler can run at any time, `tempfile_list` and
* the `tempfile` objects that comprise it must be kept in
* self-consistent states at all times.
*
* The possible states of a `tempfile` object are as follows:
*
* - Uninitialized. In this state the object's `on_list` field must be
* zero but the rest of its contents need not be initialized. As
* soon as the object is used in any way, it is irrevocably
* registered in `tempfile_list`, and `on_list` is set.
*
* - Active, file open (after `create_tempfile()` or
* `reopen_tempfile()`). In this state:
*
* - the temporary file exists
* - `active` is set
* - `filename` holds the filename of the temporary file
* - `fd` holds a file descriptor open for writing to it
* - `fp` holds a pointer to an open `FILE` object if and only if
* `fdopen_tempfile()` has been called on the object
* - `owner` holds the PID of the process that created the file
*
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>
2017-09-05 14:14:30 +02:00
* - Active, file closed (after `close_tempfile_gently()`). Same
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
* as the previous state, except that the temporary file is closed,
* `fd` is -1, and `fp` is `NULL`.
*
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>
2017-09-05 14:14:30 +02:00
* - Inactive (after `delete_tempfile()`, `rename_tempfile()`, or a
* failed attempt to create a temporary file). In this state:
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
*
* - `active` is unset
* - `filename` is empty (usually, though there are transitory
* states in which this condition doesn't hold). Client code should
* *not* rely on the filename being empty in this state.
* - `fd` is -1 and `fp` is `NULL`
tempfile: remove deactivated list entries Once a "struct tempfile" is added to the global cleanup list, it is never removed. This means that its storage must remain valid for the lifetime of the program. For single-use tempfiles and locks, this isn't a big deal: we just declare the struct static. But for library code which may take multiple simultaneous locks (like the ref code), they're forced to allocate a struct on the heap and leak it. This is mostly OK in practice. The size of the leak is bounded by the number of refs, and most programs exit after operating on a fixed number of refs (and allocate simultaneous memory proportional to the number of ref updates in the first place). But: 1. It isn't hard to imagine a real leak: a program which runs for a long time taking a series of ref update instructions and fulfilling them one by one. I don't think we have such a program now, but it's certainly plausible. 2. The leaked entries appear as false positives to tools like valgrind. Let's relax this rule by keeping only "active" tempfiles on the list. We can do this easily by moving the list-add operation from prepare_tempfile_object to activate_tempfile, and adding a deletion in deactivate_tempfile. Existing callers do not need to be updated immediately. They'll continue to leak any tempfile objects they may have allocated, but that's no different than the status quo. We can clean them up individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:04 +02:00
* - the object is removed from `tempfile_list` (but could be used again)
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
*
* A temporary file is owned by the process that created it. The
* `tempfile` has an `owner` field that records the owner's PID. This
* field is used to prevent a forked process from deleting a temporary
* file created by its parent.
*/
#include "cache.h"
#include "tempfile.h"
#include "sigchain.h"
tempfile: use list.h for linked list The tempfile API keeps to-be-cleaned tempfiles in a singly-linked list and never removes items from the list. A future patch would like to start removing items, but removal from a singly linked list is O(n), as we have to walk the list to find the predecessor element. This means that a process which takes "n" simultaneous lockfiles (for example, an atomic transaction on "n" refs) may end up quadratic in "n". Before we start allowing items to be removed, it would be nice to have a way to cover this case in linear time. The simplest solution is to make an assumption about the order in which tempfiles are added and removed from the list. If both operations iterate over the tempfiles in the same order, then by putting new items at the end of the list our removal search will always find its items at the beginning of the list. And indeed, that would work for the case of refs. But it creates a hidden dependency between unrelated parts of the code. If anybody changes the ref code (or if we add a new caller that opens multiple simultaneous tempfiles) they may unknowingly introduce a performance regression. Another solution is to use a better data structure. A doubly-linked list works fine, and we already have an implementation in list.h. But there's one snag: the elements of "struct tempfile" are all marked as "volatile", since a signal handler may interrupt us and iterate over the list at any moment (even if we were in the middle of adding a new entry). We can declare a "volatile struct list_head", but we can't actually use it with the normal list functions. The compiler complains about passing a pointer-to-volatile via a regular pointer argument. And rightfully so, as the sub-function would potentially need different code to deal with the volatile case. That leaves us with a few options: 1. Drop the "volatile" modifier for the list items. This is probably a bad idea. I checked the assembly output from "gcc -O2", and the "volatile" really does impact the order in which it updates memory. 2. Use macros instead of inline functions. The irony here is that list.h is entirely implemented as trivial inline functions. So we basically are already generating custom code for each call. But sadly there's no way in C to declare the inline function to take a more generic type. We could do so by switching the inline functions to macros, but it does make the end result harder to read. And it doesn't fully solve the problem (for instance, the declaration of list_head needs to change so that its "prev" and "next" pointers point to other volatile structs). 3. Don't use list.h, and just make our own ad-hoc doubly-linked list. It's not that much code to implement the basics that we need here. But if we're going to do so, why not add the few extra lines required to model it after the actual list.h interface? We can even reuse a few of the macro helpers. So this patch takes option 3, but actually implements a parallel "volatile list" interface in list.h, where it could potentially be reused by other code. This implements just enough for tempfile.c's use, though we could easily port other functions later if need be. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:00 +02:00
static VOLATILE_LIST_HEAD(tempfile_list);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
tempfile: robustify cleanup handler We may call remove_tempfiles() from an atexit handler, or from a signal handler. In the latter case we must take care to avoid functions which may deadlock if the process is in an unknown state, including looking at any stdio handles (which may be in the middle of doing I/O and locked) or calling malloc() or free(). The current implementation calls delete_tempfile(). We unset the tempfile's stdio handle (if any) to avoid deadlocking there. But delete_tempfile() still calls unlink_or_warn(), which can deadlock writing to stderr if the unlink fails. Since delete_tempfile() isn't very long, let's just open-code our own simple conservative version of the same thing. Notably: 1. The "skip_fclose" flag is now called "in_signal_handler", because it should inform more decisions than just the fclose handling. 2. We can replace close_tempfile() with just close(fd). That skips the fclose() question altogether. This is fine for the atexit() case, too; there's no point flushing data to a file which we're about to delete anyway. 3. We can choose between unlink/unlink_or_warn based on whether it's safe to use stderr. 4. We can replace the deactivate_tempfile() call with a simple setting of the active flag. There's no need to do any further cleanup since we know the program is exiting. And even though the current deactivation code is safe in a signal handler, this frees us up in future patches to make non-signal deactivation more complicated (e.g., by freeing resources). 5. There's no need to remove items from the tempfile_list. The "active" flag is the ultimate answer to whether an entry has been handled or not. Manipulating the list just introduces more chance of recursive signals stomping on each other, and the whole list will go away when the program exits anyway. Less is more. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:53 +02:00
static void remove_tempfiles(int in_signal_handler)
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
{
pid_t me = getpid();
tempfile: use list.h for linked list The tempfile API keeps to-be-cleaned tempfiles in a singly-linked list and never removes items from the list. A future patch would like to start removing items, but removal from a singly linked list is O(n), as we have to walk the list to find the predecessor element. This means that a process which takes "n" simultaneous lockfiles (for example, an atomic transaction on "n" refs) may end up quadratic in "n". Before we start allowing items to be removed, it would be nice to have a way to cover this case in linear time. The simplest solution is to make an assumption about the order in which tempfiles are added and removed from the list. If both operations iterate over the tempfiles in the same order, then by putting new items at the end of the list our removal search will always find its items at the beginning of the list. And indeed, that would work for the case of refs. But it creates a hidden dependency between unrelated parts of the code. If anybody changes the ref code (or if we add a new caller that opens multiple simultaneous tempfiles) they may unknowingly introduce a performance regression. Another solution is to use a better data structure. A doubly-linked list works fine, and we already have an implementation in list.h. But there's one snag: the elements of "struct tempfile" are all marked as "volatile", since a signal handler may interrupt us and iterate over the list at any moment (even if we were in the middle of adding a new entry). We can declare a "volatile struct list_head", but we can't actually use it with the normal list functions. The compiler complains about passing a pointer-to-volatile via a regular pointer argument. And rightfully so, as the sub-function would potentially need different code to deal with the volatile case. That leaves us with a few options: 1. Drop the "volatile" modifier for the list items. This is probably a bad idea. I checked the assembly output from "gcc -O2", and the "volatile" really does impact the order in which it updates memory. 2. Use macros instead of inline functions. The irony here is that list.h is entirely implemented as trivial inline functions. So we basically are already generating custom code for each call. But sadly there's no way in C to declare the inline function to take a more generic type. We could do so by switching the inline functions to macros, but it does make the end result harder to read. And it doesn't fully solve the problem (for instance, the declaration of list_head needs to change so that its "prev" and "next" pointers point to other volatile structs). 3. Don't use list.h, and just make our own ad-hoc doubly-linked list. It's not that much code to implement the basics that we need here. But if we're going to do so, why not add the few extra lines required to model it after the actual list.h interface? We can even reuse a few of the macro helpers. So this patch takes option 3, but actually implements a parallel "volatile list" interface in list.h, where it could potentially be reused by other code. This implements just enough for tempfile.c's use, though we could easily port other functions later if need be. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:00 +02:00
volatile struct volatile_list_head *pos;
list_for_each(pos, &tempfile_list) {
struct tempfile *p = list_entry(pos, struct tempfile, list);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
tempfile: robustify cleanup handler We may call remove_tempfiles() from an atexit handler, or from a signal handler. In the latter case we must take care to avoid functions which may deadlock if the process is in an unknown state, including looking at any stdio handles (which may be in the middle of doing I/O and locked) or calling malloc() or free(). The current implementation calls delete_tempfile(). We unset the tempfile's stdio handle (if any) to avoid deadlocking there. But delete_tempfile() still calls unlink_or_warn(), which can deadlock writing to stderr if the unlink fails. Since delete_tempfile() isn't very long, let's just open-code our own simple conservative version of the same thing. Notably: 1. The "skip_fclose" flag is now called "in_signal_handler", because it should inform more decisions than just the fclose handling. 2. We can replace close_tempfile() with just close(fd). That skips the fclose() question altogether. This is fine for the atexit() case, too; there's no point flushing data to a file which we're about to delete anyway. 3. We can choose between unlink/unlink_or_warn based on whether it's safe to use stderr. 4. We can replace the deactivate_tempfile() call with a simple setting of the active flag. There's no need to do any further cleanup since we know the program is exiting. And even though the current deactivation code is safe in a signal handler, this frees us up in future patches to make non-signal deactivation more complicated (e.g., by freeing resources). 5. There's no need to remove items from the tempfile_list. The "active" flag is the ultimate answer to whether an entry has been handled or not. Manipulating the list just introduces more chance of recursive signals stomping on each other, and the whole list will go away when the program exits anyway. Less is more. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:53 +02:00
if (!is_tempfile_active(p) || p->owner != me)
continue;
if (p->fd >= 0)
close(p->fd);
if (in_signal_handler)
unlink(p->filename.buf);
else
unlink_or_warn(p->filename.buf);
p->active = 0;
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
}
}
static void remove_tempfiles_on_exit(void)
{
remove_tempfiles(0);
}
static void remove_tempfiles_on_signal(int signo)
{
remove_tempfiles(1);
sigchain_pop(signo);
raise(signo);
}
prepare_tempfile_object(): new function, extracted from create_tempfile() This makes the next step easier. The old code used to use "path" to set the initial length of tempfile->filename. This was not helpful because path was usually relative whereas the value stored to filename will be absolute. So just initialize the length to 0. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:42 +02:00
static void prepare_tempfile_object(struct tempfile *tempfile)
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
{
tempfile: remove deactivated list entries Once a "struct tempfile" is added to the global cleanup list, it is never removed. This means that its storage must remain valid for the lifetime of the program. For single-use tempfiles and locks, this isn't a big deal: we just declare the struct static. But for library code which may take multiple simultaneous locks (like the ref code), they're forced to allocate a struct on the heap and leak it. This is mostly OK in practice. The size of the leak is bounded by the number of refs, and most programs exit after operating on a fixed number of refs (and allocate simultaneous memory proportional to the number of ref updates in the first place). But: 1. It isn't hard to imagine a real leak: a program which runs for a long time taking a series of ref update instructions and fulfilling them one by one. I don't think we have such a program now, but it's certainly plausible. 2. The leaked entries appear as false positives to tools like valgrind. Let's relax this rule by keeping only "active" tempfiles on the list. We can do this easily by moving the list-add operation from prepare_tempfile_object to activate_tempfile, and adding a deletion in deactivate_tempfile. Existing callers do not need to be updated immediately. They'll continue to leak any tempfile objects they may have allocated, but that's no different than the status quo. We can clean them up individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:04 +02:00
tempfile->fd = -1;
tempfile->fp = NULL;
tempfile->active = 0;
tempfile->owner = 0;
INIT_LIST_HEAD(&tempfile->list);
strbuf_init(&tempfile->filename, 0);
prepare_tempfile_object(): new function, extracted from create_tempfile() This makes the next step easier. The old code used to use "path" to set the initial length of tempfile->filename. This was not helpful because path was usually relative whereas the value stored to filename will be absolute. So just initialize the length to 0. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:42 +02:00
}
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
static void activate_tempfile(struct tempfile *tempfile)
{
tempfile: remove deactivated list entries Once a "struct tempfile" is added to the global cleanup list, it is never removed. This means that its storage must remain valid for the lifetime of the program. For single-use tempfiles and locks, this isn't a big deal: we just declare the struct static. But for library code which may take multiple simultaneous locks (like the ref code), they're forced to allocate a struct on the heap and leak it. This is mostly OK in practice. The size of the leak is bounded by the number of refs, and most programs exit after operating on a fixed number of refs (and allocate simultaneous memory proportional to the number of ref updates in the first place). But: 1. It isn't hard to imagine a real leak: a program which runs for a long time taking a series of ref update instructions and fulfilling them one by one. I don't think we have such a program now, but it's certainly plausible. 2. The leaked entries appear as false positives to tools like valgrind. Let's relax this rule by keeping only "active" tempfiles on the list. We can do this easily by moving the list-add operation from prepare_tempfile_object to activate_tempfile, and adding a deletion in deactivate_tempfile. Existing callers do not need to be updated immediately. They'll continue to leak any tempfile objects they may have allocated, but that's no different than the status quo. We can clean them up individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:04 +02:00
static int initialized;
if (is_tempfile_active(tempfile))
BUG("activate_tempfile called for active object");
if (!initialized) {
sigchain_push_common(remove_tempfiles_on_signal);
atexit(remove_tempfiles_on_exit);
initialized = 1;
}
volatile_list_add(&tempfile->list, &tempfile_list);
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
tempfile->owner = getpid();
tempfile->active = 1;
}
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
static void deactivate_tempfile(struct tempfile *tempfile)
{
tempfile->active = 0;
tempfile: release deactivated strbufs instead of resetting When a tempfile is deactivated, we reset its strbuf to the empty string, which means we hold onto the memory for later reuse. Since we'd like to move to a system where tempfile structs can actually be freed, deactivating one should drop all resources it is currently using. And thus "release" rather than "reset" is the appropriate function to call. In theory the reset may have saved a malloc() when a tempfile (or a lockfile) is reused multiple times. But in practice this happened rarely. Most of our tempfiles are single-use, since in cases where we might actually use many (like ref locking) we xcalloc() a fresh one for each ref. In fact, we leak those locks (to appease the rule that tempfile storage can never be freed). Which means that using reset is actively hurting us: instead of leaking just the tempfile struct, we're leaking the extra heap chunk for the filename, too. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:56 +02:00
strbuf_release(&tempfile->filename);
tempfile: remove deactivated list entries Once a "struct tempfile" is added to the global cleanup list, it is never removed. This means that its storage must remain valid for the lifetime of the program. For single-use tempfiles and locks, this isn't a big deal: we just declare the struct static. But for library code which may take multiple simultaneous locks (like the ref code), they're forced to allocate a struct on the heap and leak it. This is mostly OK in practice. The size of the leak is bounded by the number of refs, and most programs exit after operating on a fixed number of refs (and allocate simultaneous memory proportional to the number of ref updates in the first place). But: 1. It isn't hard to imagine a real leak: a program which runs for a long time taking a series of ref update instructions and fulfilling them one by one. I don't think we have such a program now, but it's certainly plausible. 2. The leaked entries appear as false positives to tools like valgrind. Let's relax this rule by keeping only "active" tempfiles on the list. We can do this easily by moving the list-add operation from prepare_tempfile_object to activate_tempfile, and adding a deletion in deactivate_tempfile. Existing callers do not need to be updated immediately. They'll continue to leak any tempfile objects they may have allocated, but that's no different than the status quo. We can clean them up individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:15:04 +02:00
volatile_list_del(&tempfile->list);
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
}
prepare_tempfile_object(): new function, extracted from create_tempfile() This makes the next step easier. The old code used to use "path" to set the initial length of tempfile->filename. This was not helpful because path was usually relative whereas the value stored to filename will be absolute. So just initialize the length to 0. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:42 +02:00
/* Make sure errno contains a meaningful value on error */
int create_tempfile(struct tempfile *tempfile, const char *path)
{
prepare_tempfile_object(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
strbuf_add_absolute_path(&tempfile->filename, path);
mingw: ensure temporary file handles are not inherited by child processes When the index is locked and child processes inherit the handle to said lock and the parent process wants to remove the lock before the child process exits, on Windows there is a problem: it won't work because files cannot be deleted if a process holds a handle on them. The symptom: Rename from 'xxx/.git/index.lock' to 'xxx/.git/index' failed. Should I try again? (y/n) Spawning child processes with bInheritHandles==FALSE would not work because no file handles would be inherited, not even the hStdXxx handles in STARTUPINFO (stdin/stdout/stderr). Opening every file with O_NOINHERIT does not work, either, as e.g. git-upload-pack expects inherited file handles. This leaves us with the only way out: creating temp files with the O_NOINHERIT flag. This flag is Windows-specific, however. For our purposes, it is equivalent to O_CLOEXEC (which does not exist on Windows), so let's just open temporary files with the O_CLOEXEC flag and map that flag to O_NOINHERIT on Windows. As Eric Wong pointed out, we need to be careful to handle the case where the Linux headers used to compile Git support O_CLOEXEC but the Linux kernel used to run Git does not: it returns an EINVAL. This fixes the test that we just introduced to demonstrate the problem. Signed-off-by: Ben Wijen <ben@wijen.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-08-22 14:47:55 +02:00
tempfile->fd = open(tempfile->filename.buf,
O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
/* Try again w/o O_CLOEXEC: the kernel might not support it */
tempfile->fd = open(tempfile->filename.buf,
O_RDWR | O_CREAT | O_EXCL, 0666);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
if (tempfile->fd < 0) {
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
deactivate_tempfile(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return -1;
}
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
activate_tempfile(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
if (adjust_shared_perm(tempfile->filename.buf)) {
int save_errno = errno;
error("cannot fix permission bits on %s", tempfile->filename.buf);
delete_tempfile(tempfile);
errno = save_errno;
return -1;
}
return tempfile->fd;
}
register_tempfile(): new function to handle an existing temporary file Allow an existing file to be registered with the tempfile-handling infrastructure; in particular, arrange for it to be deleted on program exit. This can be used if the temporary file has to be created in a more complicated way than just open(). For example: * If the file itself needs to be created via the lockfile API * If it is not a regular file (e.g., a socket) Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:44 +02:00
void register_tempfile(struct tempfile *tempfile, const char *path)
{
prepare_tempfile_object(tempfile);
strbuf_add_absolute_path(&tempfile->filename, path);
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
activate_tempfile(tempfile);
register_tempfile(): new function to handle an existing temporary file Allow an existing file to be registered with the tempfile-handling infrastructure; in particular, arrange for it to be deleted on program exit. This can be used if the temporary file has to be created in a more complicated way than just open(). For example: * If the file itself needs to be created via the lockfile API * If it is not a regular file (e.g., a socket) Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:44 +02:00
}
tempfile: add several functions for creating temporary files Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:43 +02:00
int mks_tempfile_sm(struct tempfile *tempfile,
const char *template, int suffixlen, int mode)
{
prepare_tempfile_object(tempfile);
strbuf_add_absolute_path(&tempfile->filename, template);
tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
if (tempfile->fd < 0) {
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
deactivate_tempfile(tempfile);
tempfile: add several functions for creating temporary files Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:43 +02:00
return -1;
}
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
activate_tempfile(tempfile);
tempfile: add several functions for creating temporary files Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:43 +02:00
return tempfile->fd;
}
int mks_tempfile_tsm(struct tempfile *tempfile,
const char *template, int suffixlen, int mode)
{
const char *tmpdir;
prepare_tempfile_object(tempfile);
tmpdir = getenv("TMPDIR");
if (!tmpdir)
tmpdir = "/tmp";
strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, template);
tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
if (tempfile->fd < 0) {
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
deactivate_tempfile(tempfile);
tempfile: add several functions for creating temporary files Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:43 +02:00
return -1;
}
tempfile: factor out activation There are a few steps required to "activate" a tempfile struct. Let's pull these out into a function. That saves a few repeated lines now, but more importantly will make it easier to change the activation scheme later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:47 +02:00
activate_tempfile(tempfile);
tempfile: add several functions for creating temporary files Add several functions for creating temporary files with automatically-generated names, analogous to mkstemps(), but also arranging for the files to be deleted on program exit. The functions are named according to a pattern depending how they operate. They will be used to replace many places in the code where temporary files are created and cleaned up ad-hoc. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:43 +02:00
return tempfile->fd;
}
int xmks_tempfile_m(struct tempfile *tempfile, const char *template, int mode)
{
int fd;
struct strbuf full_template = STRBUF_INIT;
strbuf_add_absolute_path(&full_template, template);
fd = mks_tempfile_m(tempfile, full_template.buf, mode);
if (fd < 0)
die_errno("Unable to create temporary file '%s'",
full_template.buf);
strbuf_release(&full_template);
return fd;
}
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
FILE *fdopen_tempfile(struct tempfile *tempfile, const char *mode)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("fdopen_tempfile() called for inactive object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
if (tempfile->fp)
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("fdopen_tempfile() called for open object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
tempfile->fp = fdopen(tempfile->fd, mode);
return tempfile->fp;
}
const char *get_tempfile_path(struct tempfile *tempfile)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("get_tempfile_path() called for inactive object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return tempfile->filename.buf;
}
int get_tempfile_fd(struct tempfile *tempfile)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("get_tempfile_fd() called for inactive object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return tempfile->fd;
}
FILE *get_tempfile_fp(struct tempfile *tempfile)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("get_tempfile_fp() called for inactive object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return tempfile->fp;
}
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>
2017-09-05 14:14:30 +02:00
int close_tempfile_gently(struct tempfile *tempfile)
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
{
tempfile: handle NULL tempfile pointers gracefully The tempfile functions all take pointers to tempfile objects, but do not check whether the argument is NULL. This isn't a big deal in practice, since the lifetime of any tempfile object is defined to last for the whole program. So even if we try to call delete_tempfile() on an already-deleted tempfile, our "active" check will tell us that it's a noop. In preparation for transitioning to a new system that loosens the "tempfile objects can never be freed" rule, let's tighten up our active checks: 1. A NULL pointer is now defined as "inactive" (so it will BUG for most functions, but works as a silent noop for things like delete_tempfile). 2. Functions should always do the "active" check before looking at any of the struct fields. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:40 +02:00
int fd;
FILE *fp;
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
int err;
tempfile: handle NULL tempfile pointers gracefully The tempfile functions all take pointers to tempfile objects, but do not check whether the argument is NULL. This isn't a big deal in practice, since the lifetime of any tempfile object is defined to last for the whole program. So even if we try to call delete_tempfile() on an already-deleted tempfile, our "active" check will tell us that it's a noop. In preparation for transitioning to a new system that loosens the "tempfile objects can never be freed" rule, let's tighten up our active checks: 1. A NULL pointer is now defined as "inactive" (so it will BUG for most functions, but works as a silent noop for things like delete_tempfile). 2. Functions should always do the "active" check before looking at any of the struct fields. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:40 +02:00
if (!is_tempfile_active(tempfile) || tempfile->fd < 0)
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return 0;
tempfile: handle NULL tempfile pointers gracefully The tempfile functions all take pointers to tempfile objects, but do not check whether the argument is NULL. This isn't a big deal in practice, since the lifetime of any tempfile object is defined to last for the whole program. So even if we try to call delete_tempfile() on an already-deleted tempfile, our "active" check will tell us that it's a noop. In preparation for transitioning to a new system that loosens the "tempfile objects can never be freed" rule, let's tighten up our active checks: 1. A NULL pointer is now defined as "inactive" (so it will BUG for most functions, but works as a silent noop for things like delete_tempfile). 2. Functions should always do the "active" check before looking at any of the struct fields. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:40 +02:00
fd = tempfile->fd;
fp = tempfile->fp;
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
tempfile: set errno to a known value before calling ferror() In close_tempfile(), we return an error if ferror() indicated a previous failure, or if fclose() failed. In the latter case, errno is set and it is useful for callers to report it. However, if _only_ ferror() triggers, then the value of errno is based on whatever syscall happened to last fail, which may not be related to our filehandle at all. A caller cannot tell the difference between the two cases, and may use "die_errno()" or similar to report a nonsense errno value. One solution would be to actually pass back separate return values for the two cases, so a caller can write a more appropriate message for each case. But that makes the interface clunky. Instead, let's just set errno to the generic EIO in this case. That's not as descriptive as we'd like, but at least it's predictable. So it's better than the status quo in all cases but one: when the last syscall really did involve a failure on our filehandle, we'll be wiping that out. But that's a fragile thing for us to rely on. In any case, we'll let the errno result from fclose() take precedence over our value, as we know that's recent and accurate (and many I/O errors will persist through the fclose anyway). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-02-17 22:07:49 +01:00
if (ferror(fp)) {
err = -1;
if (!fclose(fp))
errno = EIO;
} else {
err = fclose(fp);
}
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
} else {
err = close(fd);
}
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>
2017-09-05 14:14:30 +02:00
return err ? -1 : 0;
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
}
int reopen_tempfile(struct tempfile *tempfile)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("reopen_tempfile called for an inactive object");
tempfile: handle NULL tempfile pointers gracefully The tempfile functions all take pointers to tempfile objects, but do not check whether the argument is NULL. This isn't a big deal in practice, since the lifetime of any tempfile object is defined to last for the whole program. So even if we try to call delete_tempfile() on an already-deleted tempfile, our "active" check will tell us that it's a noop. In preparation for transitioning to a new system that loosens the "tempfile objects can never be freed" rule, let's tighten up our active checks: 1. A NULL pointer is now defined as "inactive" (so it will BUG for most functions, but works as a silent noop for things like delete_tempfile). 2. Functions should always do the "active" check before looking at any of the struct fields. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:40 +02:00
if (0 <= tempfile->fd)
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("reopen_tempfile called for an open object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
tempfile->fd = open(tempfile->filename.buf, O_WRONLY);
return tempfile->fd;
}
int rename_tempfile(struct tempfile *tempfile, const char *path)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: replace die("BUG") with BUG() Compared to die(), using BUG() triggers abort(). That may give us an actual coredump, which should make it easier to get a stack trace. And since the programming error for these assertions is not in the functions themselves but in their callers, such a stack trace is needed to actually find the source of the bug. In addition, abort() raises SIGABRT, which is more likely to be caught by our test suite. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:43 +02:00
BUG("rename_tempfile called for inactive object");
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
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>
2017-09-05 14:14:30 +02:00
if (close_tempfile_gently(tempfile)) {
delete_tempfile(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return -1;
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>
2017-09-05 14:14:30 +02:00
}
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
if (rename(tempfile->filename.buf, path)) {
int save_errno = errno;
delete_tempfile(tempfile);
errno = save_errno;
return -1;
}
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
deactivate_tempfile(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return 0;
}
void delete_tempfile(struct tempfile *tempfile)
{
tempfile: prefer is_tempfile_active to bare access The tempfile code keeps an "active" flag, and we have a number of assertions to make sure that the objects are being used in the right order. Most of these directly check "active" rather than using the is_tempfile_active() accessor. Let's prefer using the accessor, in preparation for it growing more complicated logic (like checking for NULL). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:36 +02:00
if (!is_tempfile_active(tempfile))
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
return;
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>
2017-09-05 14:14:30 +02:00
close_tempfile_gently(tempfile);
unlink_or_warn(tempfile->filename.buf);
tempfile: factor out deactivation When we deactivate a tempfile, we also have to clean up the "filename" strbuf. Let's pull this out into its own function to keep the logic in one place (which will become more important when a future patch makes it more complicated). Note that we can use the same function when deactivating an object that _isn't_ actually active yet (like when we hit an error creating a tempfile). These callsites don't currently reset the "active" flag to 0, but it's OK to do so (it's just a noop for these cases). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-09-05 14:14:50 +02:00
deactivate_tempfile(tempfile);
tempfile: a new module for handling temporary files A lot of work went into defining the state diagram for lockfiles and ensuring correct, race-resistant cleanup in all circumstances. Most of that infrastructure can be applied directly to *any* temporary file. So extract a new "tempfile" module from the "lockfile" module. Reimplement lockfile on top of tempfile. Subsequent commits will add more users of the new module. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10 11:47:41 +02:00
}