Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
On 04/02/2014 06:53 PM, Junio C Hamano wrote:
> Jeff King writes:
>
>> On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote:
>>
>>> By the time the "if" block is entered, the lock_file instance from the
>>> main function block is no longer in use, so re-use that one instead of
>>> allocating a second one.
>>>
>>> Note that the "lock" variable in the "if" block used to shadow the
>>> "lock" variable at function scope, so the only change needed is to
>>> remove the inner definition.
>>
>> I wonder if this would also be simpler if "lock" were simply declared as
>> a static variable, and we drop the allocation entirely. I suppose that
>> does create more cognitive load, though, in that it is only correct if
>> the function is not recursive. On the other hand, the current code makes
>> a reader unfamiliar with "struct lock" wonder if there is a free(lock)
>> missing.
>
> Another thing that makes a reader wonder if this is a valid rewrite
> is if it is safe to reuse a lock_file structure, especially because
> the original gives a piece of memory _cleared_ with xcalloc(). The
> second invocation of hold_locked_index() is now done on a dirty
> piece of memory, and the reader needs to drill down the callchain to
> see if that is safe (and if not, hold_locked_index() and probably
> the underlying lock_file() needs to memset() it to NULs).
It's good that you and Peff asked questions about this sort of thing.
We reuse lock_file structures *all over the place*; for example, just
search for "static struct lock_file". It has to be safe...
...and yet it isn't. Look in the definition of lock_file() (before my
changes):
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
...
strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
strcat(lk->filename, ".lock");
Remember that a reused lock_file structure is already in lock_file_list,
and there is already a signal handler registered that will call
remove_lock_file(), which looks like:
static void remove_lock_file(void)
{
pid_t me = getpid();
while (lock_file_list) {
if (lock_file_list->owner == me &&
lock_file_list->filename[0]) {
if (lock_file_list->fd >= 0)
close(lock_file_list->fd);
unlink_or_warn(lock_file_list->filename);
}
lock_file_list = lock_file_list->next;
}
}
So, if the process gets a signal during the call to resolve_symlink(),
the atexit() cleanup routine will delete the valuable file (the one
being locked)!
It definitely looks like this area needs more work.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
Jeff King writes: > On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote: > >> By the time the "if" block is entered, the lock_file instance from the >> main function block is no longer in use, so re-use that one instead of >> allocating a second one. >> >> Note that the "lock" variable in the "if" block used to shadow the >> "lock" variable at function scope, so the only change needed is to >> remove the inner definition. > > I wonder if this would also be simpler if "lock" were simply declared as > a static variable, and we drop the allocation entirely. I suppose that > does create more cognitive load, though, in that it is only correct if > the function is not recursive. On the other hand, the current code makes > a reader unfamiliar with "struct lock" wonder if there is a free(lock) > missing. Another thing that makes a reader wonder if this is a valid rewrite is if it is safe to reuse a lock_file structure, especially because the original gives a piece of memory _cleared_ with xcalloc(). The second invocation of hold_locked_index() is now done on a dirty piece of memory, and the reader needs to drill down the callchain to see if that is safe (and if not, hold_locked_index() and probably the underlying lock_file() needs to memset() it to NULs). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
On 04/01/2014 09:56 PM, Jeff King wrote: > On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote: > >> By the time the "if" block is entered, the lock_file instance from the >> main function block is no longer in use, so re-use that one instead of >> allocating a second one. >> >> Note that the "lock" variable in the "if" block used to shadow the >> "lock" variable at function scope, so the only change needed is to >> remove the inner definition. > > I wonder if this would also be simpler if "lock" were simply declared as > a static variable, and we drop the allocation entirely. I suppose that > does create more cognitive load, though, in that it is only correct if > the function is not recursive. On the other hand, the current code makes > a reader unfamiliar with "struct lock" wonder if there is a free(lock) > missing. Yes, a single lock_file object should suffice. I will make this change in the next version of the patch series. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
On Tue, Apr 01, 2014 at 05:58:10PM +0200, Michael Haggerty wrote: > By the time the "if" block is entered, the lock_file instance from the > main function block is no longer in use, so re-use that one instead of > allocating a second one. > > Note that the "lock" variable in the "if" block used to shadow the > "lock" variable at function scope, so the only change needed is to > remove the inner definition. I wonder if this would also be simpler if "lock" were simply declared as a static variable, and we drop the allocation entirely. I suppose that does create more cognitive load, though, in that it is only correct if the function is not recursive. On the other hand, the current code makes a reader unfamiliar with "struct lock" wonder if there is a free(lock) missing. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/22] try_merge_strategy(): remove redundant lock_file allocation
By the time the "if" block is entered, the lock_file instance from the
main function block is no longer in use, so re-use that one instead of
allocating a second one.
Note that the "lock" variable in the "if" block used to shadow the
"lock" variable at function scope, so the only change needed is to
remove the inner definition.
Signed-off-by: Michael Haggerty
---
builtin/merge.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index e15d0e1..f714961 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,7 +671,6 @@ static int try_merge_strategy(const char *strategy, struct
commit_list *common,
if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
int clean, x;
struct commit *result;
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int index_fd;
struct commit_list *reversed = NULL;
struct merge_options o;
--
1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
