FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 11-21-2007, 03:30 PM
Xavier
 
Default [pacman-dev] FREELIST macros / little memleak fix

1)
I remember having already asked that, but it was probably just on irc, and
anyway I forgot what the answer was (if there was one ).
It's about commit a57b2f233 :
http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=a57b2f233f28c275b0b17 1cb291415351f9ec87d

What was wrong / messy about these macros :

-/* TODO we should do away with these... they're messy*/
-#define _FREELIST(p, f) do { alpm_list_free_inner(p, f); alpm_list_free(p); p = NULL; } while(0)
-#define FREELIST(p) _FREELIST(p, free)
-#define FREELISTPTR(p) do { alpm_list_free(p); p = NULL; } while(0)
+#define FREELIST(p) do { alpm_list_free_inner(p, free); alpm_list_free(p); p = NULL; } while(0)

Hm, actually while writing this mail, I realize this it not very problematic :P
In the worst case, we just need two little additional calls. But I still
don't find these very messy.

2)
If I mentioning this, it's because I found a little memleak in
_alpm_sync_free: a missing alpm_list_free call. So maybe the above
_FREELIST(p, f) macro could have helped.

Also, for some reasons, both _alpm_sync_free and _alpm_trans_free did the job
of alpm_list_free_inner manually. So I changed that.

3)
A little problem : all _alpm_*_free don't match the alpm_list_fn_free
prototype except one : _alpm_graph_free .
All others don't take a void * argument, so they need to be casted to
alpm_list_fn_free for being used with alpm_list_free_inner .

So should "static void _alpm_graph_free(void *data)" be changed to
"static void _alpm_graph_free(pmgraph_t *graph)" ?


>From fdf689932ca1fa919b95533a2469f797f22970f1 Mon Sep 17 00:00:00 2001
From: Chantry Xavier <shiningxc@gmail.com>
Date: Wed, 21 Nov 2007 17:10:20 +0100
Subject: [PATCH] Fix a memleak in _alpm_sync_free.

An alpm_list_free call was missing.
Also make use of alpm_list_free_inner in both _alpm_sync_free and
_alpm_trans_free.

Signed-off-by: Chantry Xavier <shiningxc@gmail.com>
---
lib/libalpm/sync.c | 7 ++-----
lib/libalpm/trans.c | 10 ++--------
2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 16b1998..f03a78b 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -74,11 +74,8 @@ void _alpm_sync_free(pmsyncpkg_t *sync)

/* TODO wow this is ugly */
if(sync->type == PM_SYNC_TYPE_REPLACE) {
- alpm_list_t *tmp;
- for(tmp = sync->data; tmp; tmp = alpm_list_next(tmp)) {
- _alpm_pkg_free(tmp->data);
- tmp->data = NULL;
- }
+ alpm_list_free_inner(sync->data, (alpm_list_fn_free)_alpm_pkg_free);
+ alpm_list_free(sync->data);
sync->data = NULL;
} else {
_alpm_pkg_free(sync->data);
diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index 19ae2b6..d998826 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -246,8 +246,6 @@ pmtrans_t *_alpm_trans_new()

void _alpm_trans_free(pmtrans_t *trans)
{
- alpm_list_t *i;
-
ALPM_LOG_FUNC;

if(trans == NULL) {
@@ -256,13 +254,9 @@ void _alpm_trans_free(pmtrans_t *trans)

FREELIST(trans->targets);
if(trans->type == PM_TRANS_TYPE_SYNC) {
- for(i = trans->packages; i; i = alpm_list_next(i)) {
- _alpm_sync_free(i->data);
- }
+ alpm_list_free_inner(trans->packages, (alpm_list_fn_free)_alpm_sync_free);
} else {
- for(i = trans->packages; i; i = alpm_list_next(i)) {
- _alpm_pkg_free(i->data);
- }
+ alpm_list_free_inner(trans->packages, (alpm_list_fn_free)_alpm_pkg_free);
}
alpm_list_free(trans->packages);

--
1.5.3.6


_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 
Old 11-21-2007, 04:49 PM
"Dan McGee"
 
Default [pacman-dev] FREELIST macros / little memleak fix

On Nov 21, 2007 11:30 AM, Xavier <shiningxc@gmail.com> wrote:
> 1)
> I remember having already asked that, but it was probably just on irc, and
> anyway I forgot what the answer was (if there was one ).
> It's about commit a57b2f233 :
> http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=a57b2f233f28c275b0b17 1cb291415351f9ec87d
>
> What was wrong / messy about these macros :
>
> -/* TODO we should do away with these... they're messy*/
> -#define _FREELIST(p, f) do { alpm_list_free_inner(p, f); alpm_list_free(p); p = NULL; } while(0)
> -#define FREELIST(p) _FREELIST(p, free)
> -#define FREELISTPTR(p) do { alpm_list_free(p); p = NULL; } while(0)
> +#define FREELIST(p) do { alpm_list_free_inner(p, free); alpm_list_free(p); p = NULL; } while(0)

There were three macros that you didn't know what needed using without
actually looking at them, so instead I simplified it down to one
macro. FREELISTPTR saved zero coding while only adding unnecessary
abstraction.

Macros calling macros = suck, IMO.

> 2)
> If I mentioning this, it's because I found a little memleak in
> _alpm_sync_free: a missing alpm_list_free call. So maybe the above
> _FREELIST(p, f) macro could have helped.
>
> Also, for some reasons, both _alpm_sync_free and _alpm_trans_free did the job
> of alpm_list_free_inner manually. So I changed that.
>
> 3)
> A little problem : all _alpm_*_free don't match the alpm_list_fn_free
> prototype except one : _alpm_graph_free .
> All others don't take a void * argument, so they need to be casted to
> alpm_list_fn_free for being used with alpm_list_free_inner .
>
> So should "static void _alpm_graph_free(void *data)" be changed to
> "static void _alpm_graph_free(pmgraph_t *graph)" ?

Hmm, interesting. I like type-safe functions, but that (_alpm_fn_free)
cast seems so dirty. Not sure what to do here.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 

Thread Tools




All times are GMT. The time now is 07:33 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org