[pacman-dev] [PATCH 1/5] Two memleak fixes in pacman.
Both memleak was an unfreed alpm_db_whatprovides list. Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> --- src/pacman/deptest.c | 1 + src/pacman/sync.c | 1 + 2 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/pacman/deptest.c b/src/pacman/deptest.c index 301558e..d17c821 100644 --- a/src/pacman/deptest.c +++ b/src/pacman/deptest.c @@ -70,6 +70,7 @@ int pacman_deptest(alpm_list_t *targets) break; } } + alpm_list_free(provides); } if(!found) { diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5f1072f..6f3c5d5 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -532,6 +532,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) if(prov) { pmpkg_t *pkg = alpm_list_getdata(prov); pname = alpm_pkg_get_name(pkg); + alpm_list_free(prov); break; } } -- 1.5.3.5
On Nov 17, 2007 7:50 AM, Nagy Gabor <shiningxc@gmail.com> wrote:
Both memleak was an unfreed alpm_db_whatprovides list.
Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu> --- src/pacman/deptest.c | 1 + src/pacman/sync.c | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/pacman/deptest.c b/src/pacman/deptest.c index 301558e..d17c821 100644 --- a/src/pacman/deptest.c +++ b/src/pacman/deptest.c @@ -70,6 +70,7 @@ int pacman_deptest(alpm_list_t *targets) break; } } + alpm_list_free(provides); }
if(!found) { diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5f1072f..6f3c5d5 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -532,6 +532,7 @@ int sync_trans(alpm_list_t *targets, int sync_only) if(prov) { pmpkg_t *pkg = alpm_list_getdata(prov); pname = alpm_pkg_get_name(pkg); + alpm_list_free(prov); break; } } -- 1.5.3.5
Obviously I'll apply this. However, from a design standpoint, I really wish we never had to call alpm_list_free() from the frontend on anything returned by the library. When we start having to do that, it leads to double frees and all sorts of other fun stuff. -Dan
Obviously I'll apply this. However, from a design standpoint, I really wish we never had to call alpm_list_free() from the frontend on anything returned by the library. When we start having to do that, it leads to double frees and all sorts of other fun stuff.
I simply don't understand this. All known libraries _must_ use this when they _compute_ something on request. The library simply doesn't know if the result is still needed by the caller (front-end) or not. You may say we could implement whatprovides_ready, but this is just equivalent with list_free (and ugly). The main problem here, that we have no libalpm documentation where we could define for front-ends which results should be freed and which should not. Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/
On Nov 19, 2007 12:43 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
Obviously I'll apply this. However, from a design standpoint, I really wish we never had to call alpm_list_free() from the frontend on anything returned by the library. When we start having to do that, it leads to double frees and all sorts of other fun stuff.
I simply don't understand this. All known libraries _must_ use this when they _compute_ something on request. The library simply doesn't know if the result is still needed by the caller (front-end) or not. You may say we could implement whatprovides_ready, but this is just equivalent with list_free (and ugly). The main problem here, that we have no libalpm documentation where we could define for front-ends which results should be freed and which should not.
And that is my point. We don't have a clear definition of what needs freeing and what does not. The name "alpm_db_whatprovides" doesn't indicate at all that its result needs to be freed, and yet something like "alpm_fetch_pkgurl" doesn't need freeing (or does it? I don't know and that seems problematic). So yes, it comes down to documentation and good function naming, which we lack. :) -Dan
On Mon, 2007-11-19 at 13:07 -0600, Dan McGee wrote:
On Nov 19, 2007 12:43 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
Obviously I'll apply this. However, from a design standpoint, I really wish we never had to call alpm_list_free() from the frontend on anything returned by the library. When we start having to do that, it leads to double frees and all sorts of other fun stuff.
I simply don't understand this. All known libraries _must_ use this when they _compute_ something on request. The library simply doesn't know if the result is still needed by the caller (front-end) or not. You may say we could implement whatprovides_ready, but this is just equivalent with list_free (and ugly). The main problem here, that we have no libalpm documentation where we could define for front-ends which results should be freed and which should not.
And that is my point. We don't have a clear definition of what needs freeing and what does not. The name "alpm_db_whatprovides" doesn't indicate at all that its result needs to be freed, and yet something like "alpm_fetch_pkgurl" doesn't need freeing (or does it? I don't know and that seems problematic).
So yes, it comes down to documentation and good function naming, which we lack. :)
You could have a simple convention like GTK/GNOME stuff does: any function arguments or returns that are "const" should not be freed by the user (or front end in this case). See "Cleanliness" in http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html k
-Dan
_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev -- K. Piche <kpiche@rogers.com>
On Mon, Nov 19, 2007 at 10:45:53PM -0500, K. Piche wrote:
On Mon, 2007-11-19 at 13:07 -0600, Dan McGee wrote: You could have a simple convention like GTK/GNOME stuff does: any function arguments or returns that are "const" should not be freed by the user (or front end in this case). See "Cleanliness" in http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html
k
I also find the tip for bit fields interesting :) ... GNOME_CANVAS_UPDATE_AFFINE = 1 << 1, GNOME_CANVAS_UPDATE_CLIP = 1 << 2, ...
On Mon, Nov 19, 2007 at 10:45:53PM -0500, "K. Piche" <kpiche@rogers.com> wrote:
And that is my point. We don't have a clear definition of what needs freeing and what does not. The name "alpm_db_whatprovides" doesn't indicate at all that its result needs to be freed, and yet something like "alpm_fetch_pkgurl" doesn't need freeing (or does it? I don't know and that seems problematic).
So yes, it comes down to documentation and good function naming, which we lack. :)
You could have a simple convention like GTK/GNOME stuff does: any function arguments or returns that are "const" should not be freed by the user (or front end in this case). See "Cleanliness" in http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html
or just use __attribute__ ((__warn_unused_result__)) on those functions. obviously those functions' return value must be free'd. - VMiklos
participants (6)
-
Dan McGee
-
K. Piche
-
Miklos Vajna
-
Nagy Gabor
-
Nagy Gabor
-
Xavier