unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] Release memory allocated by internet_address_list_parse_string()
@ 2011-12-07 20:13 Tomi Ollila
  2011-12-07 21:29 ` Tomi Ollila
  0 siblings, 1 reply; 7+ messages in thread
From: Tomi Ollila @ 2011-12-07 20:13 UTC (permalink / raw)
  To: notmuch

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---
When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

Note that this fixes one potential memory leak only in case --format=mbox
as _extract_email_address is called only there.

 notmuch-show.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..3abfa07 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -255,7 +255,7 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    g_object_unref (addresses);
     return email;
    }
 
-- 
1.7.7.3

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

* Re: [PATCH] Release memory allocated by internet_address_list_parse_string()
  2011-12-07 20:13 [PATCH] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
@ 2011-12-07 21:29 ` Tomi Ollila
  2011-12-09 13:52   ` [Patch 1/2] separate handling when addresses == NULL Tomi Ollila
  2011-12-09 13:53   ` [PATCH 2/2] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
  0 siblings, 2 replies; 7+ messages in thread
From: Tomi Ollila @ 2011-12-07 21:29 UTC (permalink / raw)
  To: notmuch

On Wed, 07 Dec 2011 22:13:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> g_object_unref() releases the memory of the InternetAddressList object
> returned by internet_address_list_parse_string() -- when last (only)
> reference is released, internet_address_list_finalize() will do cleanup.
> ---
> When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
> I tested this patch by entering:  notmuch show --format=mbox '*' | wc
> 
> Note that this fixes one potential memory leak only in case --format=mbox
> as _extract_email_address is called only there.

There is at least one bug here -- I failed to check whether addresses
is non-NULL before attempting to g_object_unref (). So please ignore
that when reviewing whether g_object_unref() is the correct action here.

I provide a new patch when I know whether re-check or separate the
NULL test in the beginning of _extract_email_address().

>  notmuch-show.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..3abfa07 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -255,7 +255,7 @@ _extract_email_address (const void *ctx, const char *from)
>      email = talloc_strdup (ctx, email);
>  
>    DONE:
> -    /* XXX: How to free addresses here? */
> +    g_object_unref (addresses);
>      return email;
>     }
>  
> -- 
> 1.7.7.3

Tomi

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

* [Patch 1/2] separate handling when addresses == NULL
  2011-12-07 21:29 ` Tomi Ollila
@ 2011-12-09 13:52   ` Tomi Ollila
  2011-12-09 15:57     ` Austin Clements
  2011-12-09 13:53   ` [PATCH 2/2] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
  1 sibling, 1 reply; 7+ messages in thread
From: Tomi Ollila @ 2011-12-09 13:52 UTC (permalink / raw)
  To: notmuch

When addresses is NULL, (future) addresses object cleanup is not needed.
---
 notmuch-show.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..c27ef6a 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -239,7 +239,11 @@ _extract_email_address (const void *ctx, const char *from)
     addresses = internet_address_list_parse_string (from);
 
     /* Bail if there is no address here. */
-    if (addresses == NULL || internet_address_list_length (addresses) < 1)
+    if (addresses == NULL)
+	return email;
+
+    /* Bail if there is no address here. */
+    if (internet_address_list_length (addresses) < 1)
 	goto DONE;
 
     /* Otherwise, just use the first address. */
-- 
1.7.7.3

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

* [PATCH 2/2] Release memory allocated by internet_address_list_parse_string()
  2011-12-07 21:29 ` Tomi Ollila
  2011-12-09 13:52   ` [Patch 1/2] separate handling when addresses == NULL Tomi Ollila
@ 2011-12-09 13:53   ` Tomi Ollila
  2011-12-10 10:18     ` [PATCH v3] " Tomi Ollila
  1 sibling, 1 reply; 7+ messages in thread
From: Tomi Ollila @ 2011-12-09 13:53 UTC (permalink / raw)
  To: notmuch

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---
When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

Note that this fixes one potential memory leak only in case --format=mbox
as _extract_email_address is called only there.

 notmuch-show.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index c27ef6a..db02891 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -259,7 +259,8 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    g_object_unref(addresses);
+
     return email;
    }
 
-- 
1.7.7.3

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

* Re: [Patch 1/2] separate handling when addresses == NULL
  2011-12-09 13:52   ` [Patch 1/2] separate handling when addresses == NULL Tomi Ollila
@ 2011-12-09 15:57     ` Austin Clements
  0 siblings, 0 replies; 7+ messages in thread
From: Austin Clements @ 2011-12-09 15:57 UTC (permalink / raw)
  To: Tomi Ollila; +Cc: notmuch

Quoth Tomi Ollila on Dec 09 at  3:52 pm:
> When addresses is NULL, (future) addresses object cleanup is not needed.
> ---
>  notmuch-show.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..c27ef6a 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -239,7 +239,11 @@ _extract_email_address (const void *ctx, const char *from)
>      addresses = internet_address_list_parse_string (from);
>  
>      /* Bail if there is no address here. */
> -    if (addresses == NULL || internet_address_list_length (addresses) < 1)
> +    if (addresses == NULL)
> +	return email;
> +
> +    /* Bail if there is no address here. */
> +    if (internet_address_list_length (addresses) < 1)
>  	goto DONE;

Personally, I would much prefer to see the code as it was---with the
sometimes unnecessary goto DONE---and an if (addresses) around the
later cleanup that the second patch adds.  It's more maintainable in
case someone ever adds more code earlier in this function and this
microoptimization isn't buying you anything.

>  
>      /* Otherwise, just use the first address. */

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

* [PATCH v3] Release memory allocated by internet_address_list_parse_string()
  2011-12-09 13:53   ` [PATCH 2/2] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
@ 2011-12-10 10:18     ` Tomi Ollila
  2011-12-11 14:49       ` David Bremner
  0 siblings, 1 reply; 7+ messages in thread
From: Tomi Ollila @ 2011-12-10 10:18 UTC (permalink / raw)
  To: notmuch

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---

hmm, have to agree amdragon's "suggested alternative" is more robust 
(in maintainability point of view) than the one I took previously.

The 3 other alternatives are not common in notmuch code and therefore dropped:
1) use multiple goto labels to jump in
2) use deeply nested if blocks
3) the ultimate solution: do { .. } while (0) blocks and break;s >;)

When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

 notmuch-show.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..873a7c4 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -255,7 +255,9 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    if (addresses)
+	g_object_unref (addresses);
+
     return email;
    }
 
-- 
1.7.7.3

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

* Re: [PATCH v3] Release memory allocated by internet_address_list_parse_string()
  2011-12-10 10:18     ` [PATCH v3] " Tomi Ollila
@ 2011-12-11 14:49       ` David Bremner
  0 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2011-12-11 14:49 UTC (permalink / raw)
  To: Tomi Ollila, notmuch

On Sat, 10 Dec 2011 12:18:54 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> g_object_unref() releases the memory of the InternetAddressList object
> returned by internet_address_list_parse_string() -- when last (only)
> reference is released, internet_address_list_finalize() will do cleanup.

Pushed this version.

d

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

end of thread, other threads:[~2011-12-11 14:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-07 20:13 [PATCH] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
2011-12-07 21:29 ` Tomi Ollila
2011-12-09 13:52   ` [Patch 1/2] separate handling when addresses == NULL Tomi Ollila
2011-12-09 15:57     ` Austin Clements
2011-12-09 13:53   ` [PATCH 2/2] Release memory allocated by internet_address_list_parse_string() Tomi Ollila
2011-12-10 10:18     ` [PATCH v3] " Tomi Ollila
2011-12-11 14:49       ` David Bremner

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).