Re: [PATCH] path-list.c: always free strdup'ed paths

Re: [PATCH] path-list.c: always free strdup'ed paths

[Johannes Schindelin]

Hi,

On Sat, 7 Jul 2007, Ren? Scharfe wrote:

> Always free .paths if .strdup_paths is set, no matter if the
> parameter free_items is set or not, plugging a minor memory leak.
> And to clarify the meaning of the flag, rename it to free_util,
> since it now only affects the freeing of the .util field.

The rational was that it might very well be possible that the pointers you 
hand to the path_list are already strdup()ed. So you do not set 
strdup_paths, but you want them free()d.

The .util field is in many cases something that is not trivially free()d, 
but has to call a type-specific function, such as path_list_clear() 
itself.

So I'm mildly negative about your patch.

Ciao,
Dscho

[PATCH] path-list.c: always free strdup'ed paths

[René Scharfe]

Always free .paths if .strdup_paths is set, no matter if the
parameter free_items is set or not, plugging a minor memory leak.
And to clarify the meaning of the flag, rename it to free_util,
since it now only affects the freeing of the .util field.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 path-list.c |   14 ++++++++------
 path-list.h |    2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/path-list.c b/path-list.c
index dcb4b3a..3d83b7b 100644
--- a/path-list.c
+++ b/path-list.c
@@ -76,16 +76,18 @@ struct path_list_item *path_list_lookup(const char *path, struct path_list *list
 	return list->items + i;
 }
 
-void path_list_clear(struct path_list *list, int free_items)
+void path_list_clear(struct path_list *list, int free_util)
 {
 	if (list->items) {
 		int i;
-		if (free_items)
-			for (i = 0; i < list->nr; i++) {
-				if (list->strdup_paths)
-					free(list->items[i].path);
+		if (list->strdup_paths) {
+			for (i = 0; i < list->nr; i++)
+				free(list->items[i].path);
+		}
+		if (free_util) {
+			for (i = 0; i < list->nr; i++)
 				free(list->items[i].util);
-			}
+		}
 		free(list->items);
 	}
 	list->items = NULL;
diff --git a/path-list.h b/path-list.h
index ce5ffab..5931e2c 100644
--- a/path-list.h
+++ b/path-list.h
@@ -15,7 +15,7 @@ struct path_list
 void print_path_list(const char *text, const struct path_list *p);
 
 int path_list_has_path(const struct path_list *list, const char *path);
-void path_list_clear(struct path_list *list, int free_items);
+void path_list_clear(struct path_list *list, int free_util);
 struct path_list_item *path_list_insert(const char *path, struct path_list *list);
 struct path_list_item *path_list_lookup(const char *path, struct path_list *list);
 

Re: [PATCH] path-list.c: always free strdup'ed paths

[René Scharfe]

Johannes Schindelin schrieb:
> Hi,
> 
> On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> 
>> Always free .paths if .strdup_paths is set, no matter if the 
>> parameter free_items is set or not, plugging a minor memory leak. 
>> And to clarify the meaning of the flag, rename it to free_util, 
>> since it now only affects the freeing of the .util field.
> 
> The rational was that it might very well be possible that the
> pointers you hand to the path_list are already strdup()ed. So you do
> not set strdup_paths, but you want them free()d.

The patch doesn't take that away, i.e. .path fields are not freed if
.strdup_paths is not set, both before and after the patch.  And the
workaround used in builtin-shortlog.c, viz. setting .strdup_paths just
before calling path_list_clear(), still works.

> The .util field is in many cases something that is not trivially
> free()d, but has to call a type-specific function, such as
> path_list_clear() itself.

Indeed; this is a good reason to separate freeing of .util and .path, no?

René

Re: [PATCH] path-list.c: always free strdup'ed paths

[Johannes Schindelin]

Hi,

On Sat, 7 Jul 2007, Ren? Scharfe wrote:

> Johannes Schindelin schrieb:
> > 
> > On Sat, 7 Jul 2007, Ren? Scharfe wrote:
> > 
> >> Always free .paths if .strdup_paths is set, no matter if the 
> >> parameter free_items is set or not, plugging a minor memory leak. 
> >> And to clarify the meaning of the flag, rename it to free_util, 
> >> since it now only affects the freeing of the .util field.
> > 
> > The rational was that it might very well be possible that the
> > pointers you hand to the path_list are already strdup()ed. So you do
> > not set strdup_paths, but you want them free()d.
> 
> The patch doesn't take that away, i.e. .path fields are not freed if
> .strdup_paths is not set, both before and after the patch.

Yeah, the old code was buggy.

> And the workaround used in builtin-shortlog.c, viz. setting 
> .strdup_paths just before calling path_list_clear(), still works.

Yeah, but it is ugly.

> > The .util field is in many cases something that is not trivially
> > free()d, but has to call a type-specific function, such as
> > path_list_clear() itself.
> 
> Indeed; this is a good reason to separate freeing of .util and .path, no?

Yes.

Ciao,
Dscho