unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* Clean up glib / GKeyFile memory use
@ 2021-03-06 13:49 David Bremner
  2021-03-06 13:49 ` [PATCH 1/4] lib/open: use local talloc context in n_d_create_with_config David Bremner
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: David Bremner @ 2021-03-06 13:49 UTC (permalink / raw)
  To: notmuch

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).

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/4] lib/open: use local talloc context in n_d_create_with_config
  2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
@ 2021-03-06 13:49 ` David Bremner
  2021-03-06 13:49 ` [PATCH 2/4] lib/open: free value from g_key_file_get_value David Bremner
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2021-03-06 13:49 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/4] lib/open: free value from g_key_file_get_value
  2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
  2021-03-06 13:49 ` [PATCH 1/4] lib/open: use local talloc context in n_d_create_with_config David Bremner
@ 2021-03-06 13:49 ` David Bremner
  2021-03-06 13:49 ` [PATCH 3/4] lib/config: free memory from traversing GKeyFile David Bremner
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2021-03-06 13:49 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/4] lib/config: free memory from traversing GKeyFile
  2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
  2021-03-06 13:49 ` [PATCH 1/4] lib/open: use local talloc context in n_d_create_with_config David Bremner
  2021-03-06 13:49 ` [PATCH 2/4] lib/open: free value from g_key_file_get_value David Bremner
@ 2021-03-06 13:49 ` David Bremner
  2021-03-06 13:49 ` [PATCH 4/4] lib/open: free GKeyFile David Bremner
  2021-03-14 12:53 ` Clean up glib / GKeyFile memory use David Bremner
  4 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2021-03-06 13:49 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 4/4] lib/open: free GKeyFile
  2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
                   ` (2 preceding siblings ...)
  2021-03-06 13:49 ` [PATCH 3/4] lib/config: free memory from traversing GKeyFile David Bremner
@ 2021-03-06 13:49 ` David Bremner
  2021-03-14 12:53 ` Clean up glib / GKeyFile memory use David Bremner
  4 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2021-03-06 13:49 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: Clean up glib / GKeyFile memory use
  2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
                   ` (3 preceding siblings ...)
  2021-03-06 13:49 ` [PATCH 4/4] lib/open: free GKeyFile David Bremner
@ 2021-03-14 12:53 ` David Bremner
  4 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2021-03-14 12:53 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-14 12:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-06 13:49 Clean up glib / GKeyFile memory use David Bremner
2021-03-06 13:49 ` [PATCH 1/4] lib/open: use local talloc context in n_d_create_with_config David Bremner
2021-03-06 13:49 ` [PATCH 2/4] lib/open: free value from g_key_file_get_value David Bremner
2021-03-06 13:49 ` [PATCH 3/4] lib/config: free memory from traversing GKeyFile David Bremner
2021-03-06 13:49 ` [PATCH 4/4] lib/open: free GKeyFile David Bremner
2021-03-14 12:53 ` Clean up glib / GKeyFile memory use David Bremner

unofficial mirror of notmuch@notmuchmail.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


code repositories for project(s) associated with this inbox:

	notmuch.git.git (no URL configured)

AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git