It turns out I was too optimistic / lazy about how the g_key_file API manages memory. None of the resulting memory leaks are large (unless you somehow keep War and Peace in your config file), but it is more tidy to clean them up, and makes it easier to spot more significant leaks in the already noisy output from valgrind. This series applies on top of master. I'll have to rebase the other config series on top (and possibly clean up a few more glib related memory leaks).
This better matches the memory allocation semantics in notmuch_database_open_with_config. --- lib/open.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/open.cc b/lib/open.cc index 3b86065b..3b66d2eb 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -423,6 +423,7 @@ notmuch_database_create_with_config (const char *database_path, GKeyFile *key_file = NULL; struct stat st; int err; + void *local = talloc_new (NULL); if ((status = _choose_database_path (config_path, profile, &key_file, &database_path, &message))) goto DONE; @@ -443,7 +444,7 @@ notmuch_database_create_with_config (const char *database_path, goto DONE; } - notmuch_path = talloc_asprintf (NULL, "%s/%s", database_path, ".notmuch"); + notmuch_path = talloc_asprintf (local, "%s/%s", database_path, ".notmuch"); err = mkdir (notmuch_path, 0755); if (err) { @@ -479,8 +480,7 @@ notmuch_database_create_with_config (const char *database_path, } DONE: - if (notmuch_path) - talloc_free (notmuch_path); + talloc_free (local); if (message) { if (status_string) -- 2.30.1
This fixes a small memory leak. --- lib/open.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/open.cc b/lib/open.cc index 3b66d2eb..b424fa0c 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -153,7 +153,8 @@ DONE: } static notmuch_status_t -_choose_database_path (const char *config_path, +_choose_database_path (void *ctx, + const char *config_path, const char *profile, GKeyFile **key_file, const char **database_path, @@ -167,8 +168,13 @@ _choose_database_path (const char *config_path, return status; } - if (! *database_path && *key_file) - *database_path = g_key_file_get_value (*key_file, "database", "path", NULL); + if (! *database_path && *key_file) { + char *path = g_key_file_get_value (*key_file, "database", "path", NULL); + if (path) { + *database_path = talloc_strdup (ctx, path); + g_free (path); + } + } if (*database_path == NULL) { *message = strdup ("Error: Cannot open a database for a NULL path.\n"); @@ -201,7 +207,7 @@ notmuch_database_open_with_config (const char *database_path, GKeyFile *key_file = NULL; static int initialized = 0; - if ((status = _choose_database_path (config_path, profile, &key_file, &database_path, &message))) + if ((status = _choose_database_path (local, config_path, profile, &key_file, &database_path, &message))) goto DONE; if (! (notmuch_path = talloc_asprintf (local, "%s/%s", database_path, ".notmuch"))) { @@ -425,7 +431,7 @@ notmuch_database_create_with_config (const char *database_path, int err; void *local = talloc_new (NULL); - if ((status = _choose_database_path (config_path, profile, &key_file, &database_path, &message))) + if ((status = _choose_database_path (local, config_path, profile, &key_file, &database_path, &message))) goto DONE; err = stat (database_path, &st); -- 2.30.1
This fixes a few small memory leaks. --- lib/config.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/config.cc b/lib/config.cc index 948751bc..6ace6e52 100644 --- a/lib/config.cc +++ b/lib/config.cc @@ -330,7 +330,7 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch, GKeyFile *file) { notmuch_status_t status = NOTMUCH_STATUS_SUCCESS; - gchar **groups, **keys, *val; + gchar **groups = NULL, **keys, *val; if (notmuch->config == NULL) notmuch->config = _notmuch_string_map_create (notmuch); @@ -340,22 +340,29 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch, goto DONE; } - for (groups = g_key_file_get_groups (file, NULL); *groups; groups++) { - for (keys = g_key_file_get_keys (file, *groups, NULL, NULL); *keys; keys++) { - char *absolute_key = talloc_asprintf(notmuch, "%s.%s", *groups, *keys); - val = g_key_file_get_value (file, *groups, *keys, NULL); + groups = g_key_file_get_groups (file, NULL); + for (gchar **grp=groups; *grp; grp++) { + keys = g_key_file_get_keys (file, *grp, NULL, NULL); + for (gchar **keys_p = keys; *keys_p; keys_p++) { + char *absolute_key = talloc_asprintf(notmuch, "%s.%s", *grp, *keys_p); + val = g_key_file_get_value (file, *grp, *keys_p, NULL); if (! val) { status = NOTMUCH_STATUS_FILE_ERROR; goto DONE; } _notmuch_string_map_set (notmuch->config, absolute_key, val); + g_free (val); talloc_free (absolute_key); if (status) goto DONE; } + g_strfreev (keys); } DONE: + if (groups) + g_strfreev (groups); + return status; } -- 2.30.1
This fixes a small-to-medium (depending on size of config file) memory leak. --- lib/open.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/open.cc b/lib/open.cc index b424fa0c..a5211746 100644 --- a/lib/open.cc +++ b/lib/open.cc @@ -372,6 +372,9 @@ notmuch_database_open_with_config (const char *database_path, DONE: talloc_free (local); + if (key_file) + g_key_file_free (key_file); + if (message) { if (status_string) *status_string = message; @@ -488,6 +491,9 @@ notmuch_database_create_with_config (const char *database_path, DONE: talloc_free (local); + if (key_file) + g_key_file_free (key_file); + if (message) { if (status_string) *status_string = message; -- 2.30.1
David Bremner <david@tethera.net> writes:
> It turns out I was too optimistic / lazy about how the g_key_file API
> manages memory. None of the resulting memory leaks are large (unless
> you somehow keep War and Peace in your config file), but it is more
> tidy to clean them up, and makes it easier to spot more significant
> leaks in the already noisy output from valgrind.
>
> This series applies on top of master. I'll have to rebase the other
> config series on top (and possibly clean up a few more glib related
> memory leaks).
I have a applied a rebased version (on top of the uncrustify patches) to
master.
d