The current lockfile API often requires leaking memory:
- the list of lockfiles to remove at exit is maintained in a
singly linked list, so one cannot easily remove a lockfile
record when it is no longer needed
- some lockfile records are statically allocated and others are on
the heap, so the remove_lock_file() atexit handler cannot easily
free them at exit (not to mention that it would be a waste of time
since _exit() takes care of freeing them anyway)
As the git libification effort proceeds, maybe this will become worth
fixing. For now, since there are so few lock files used by a given
process, it is simpler to tolerate the leak.
Add a alloc_lock_file() helper to make such tolerance easier.
Allocations made through this helper function will not be reported as
leaks by valgrind even if there is no corresponding free(). The
((optimize("-fno-optimize-sibling-calls"))) attribute is needed on
platforms with GCC to ensure that the stack frame for
alloc_lock_file() is not replaced by the stack frame for xcalloc().
For illustration, this patch updates a few example xcalloc(1,
sizeof(struct lockfile)) call sites to use the new wrapper.
Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
---
Documentation/technical/api-lockfile.txt | 9 +++++++++
builtin/update-index.c | 4 +---
cache-tree.c | 7 +------
cache.h | 1 +
config.c | 2 +-
lockfile.c | 6 ++++++
t/valgrind/default.supp | 8 ++++++++
7 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index dd89404..8eac22a 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -22,6 +22,15 @@ The lockfile API serves two purposes:
The functions
-------------
+alloc_lock_file::
+
+ Allocate and clear a `struct lock_file`. Because the
+ structure is used in an `atexit(3)` handler, its
+ storage has to stay throughout the life of the
+ program. Using this allocation function instead of
+ `malloc` allows profiling tools like valgrind to keep
+ track of such storage and avoid reporting it as a leak.
+
hold_lock_file_for_update::
Take a pointer to `struct lock_file`, the filename of
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..7697631 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -591,9 +591,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
- /* We can't free this memory, it becomes part of a linked list parsed atexit() */
- lock_file = xcalloc(1, sizeof(struct lock_file));
-
+ lock_file = alloc_lock_file();
newfd = hold_locked_index(lock_file, 0);
if (newfd < 0)
lock_error = errno;
diff --git a/cache-tree.c b/cache-tree.c
index f755590..81cf785 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -550,12 +550,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
int entries, was_valid, newfd;
struct lock_file *lock_file;
- /*
- * We can't free this memory, it becomes part of a linked list
- * parsed atexit()
- */
- lock_file = xcalloc(1, sizeof(struct lock_file));
-
+ lock_file = alloc_lock_file();
newfd = hold_locked_index(lock_file, 1);
entries = read_cache();
diff --git a/cache.h b/cache.h
index 2ef2fa3..9ee61a2 100644
--- a/cache.h
+++ b/cache.h
@@ -517,6 +517,7 @@ struct lock_file {
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+extern struct lock_file *alloc_lock_file(void);
extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
extern int commit_lock_file(struct lock_file *);
diff --git a/config.c b/config.c
index 4b0a820..3f30e2b 100644
--- a/config.c
+++ b/config.c
@@ -1158,7 +1158,7 @@ int git_config_set_multivar(const char *key, const char *value,
* The lock serves a purpose in addition to locking: the new
* contents of .git/config will be written into it.
*/
- lock = xcalloc(sizeof(struct lock_file), 1);
+ lock = alloc_lock_file();
fd = hold_lock_file_for_update(lock, config_filename, 0);
if (fd < 0) {
error("could not lock config file %s: %s", config_filename, strerror(errno));
diff --git a/lockfile.c b/lockfile.c
index b0d74cd..161dea8 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -184,6 +184,12 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)
die("%s", unable_to_lock_message(path, err));
}
+__attribute__((optimize("-fno-optimize-sibling-calls")))
+struct lock_file *alloc_lock_file(void)
+{
+ return xcalloc(1, sizeof(struct lock_file));
+}
+
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
{
int fd = lock_file(lk, path, flags);
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 9e013fa..7789edd 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -43,3 +43,11 @@
fun:write_buffer
fun:write_loose_object
}
+
+{
+ see api-lockfile.txt
+ Memcheck:Leak
+ fun:calloc
+ fun:xcalloc
+ fun:alloc_lock_file
+}
--
1.7.2.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html