>From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001
From: Nagy Gabor <ngaba@bibl.u-szeged.hu>
Date: Tue, 15 Jul 2008 13:30:34 +0200
Subject: [PATCH] Fix some memleaks in alpm/add.c

In case of error some allocated memory wasn't freed in commit_single_pkg.
Note: The return value of this function is not used.

Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu>
---
lib/libalpm/add.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
index 5bf3fcd..3b60fb3 100644
--- a/lib/libalpm/add.c
+++ b/lib/libalpm/add.c
@@ -689,7 +689,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
/* set up fake remove transaction */
int ret = upgrade_remove(oldpkg, newpkg, trans, db);
if(ret != 0) {
- return(ret);
+ goto cleanup;
}
}

@@ -701,7 +701,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
_alpm_log(PM_LOG_DEBUG, "extracting files
");

if ((archive = archive_read_new()) == NULL) {
- RET_ERR(PM_ERR_LIBARCHIVE, -1);
+ pm_errno = PM_ERR_LIBARCHIVE;
+ ret = -1;
+ goto cleanup;
}

archive_read_support_compression_all(archive);
@@ -710,7 +712,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
_alpm_log(PM_LOG_DEBUG, "archive: %s
", newpkg->origin_data.file);
if(archive_read_open_filename(archive, newpkg->origin_data.file,
ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) {
- RET_ERR(PM_ERR_PKG_OPEN, -1);
+ pm_errno = PM_ERR_PKG_OPEN;
+ ret = -1;
+ goto cleanup;
}

/* save the cwd so we can restore it later */
@@ -772,7 +776,7 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
}

if(errors) {
- ret = 1;
+ ret = -1;
if(is_upgrade) {
_alpm_log(PM_LOG_ERROR, _("problem occurred while upgrading %s
"),
newpkg->name);
@@ -798,7 +802,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
alpm_pkg_get_name(newpkg), alpm_pkg_get_version(newpkg));
alpm_logaction("error: could not update database entry %s-%s
",
alpm_pkg_get_name(newpkg), alpm_pkg_get_version(newpkg));
- RET_ERR(PM_ERR_DB_WRITE, -1);
+ pm_errno = PM_ERR_DB_WRITE;
+ ret = -1;
+ goto cleanup;
}

if(_alpm_db_add_pkgincache(db, newpkg) == -1) {
@@ -833,9 +839,9 @@ static int commit_single_pkg(pmpkg_t *newpkg, int pkg_current, int pkg_count,
EVENT(trans, PM_TRANS_EVT_ADD_DONE, newpkg, oldpkg);
}

+cleanup:
_alpm_pkg_free(oldpkg);
-
- return(0);
+ return(ret);
}

int _alpm_add_commit(pmtrans_t *trans, pmdb_t *db)
--
1.5.6.2


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

On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
> >From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001
> From: Nagy Gabor <ngaba@bibl.u-szeged.hu>
> Date: Tue, 15 Jul 2008 13:30:34 +0200
> Subject: [PATCH] Fix some memleaks in alpm/add.c
>
> In case of error some allocated memory wasn't freed in commit_single_pkg.
> Note: The return value of this function is not used.
>
> Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu>
> ---

Thanks. Perhaps we should use the return value?

-Dan

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

Idézés Dan McGee <dpmcgee@gmail.com>:

> On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
> > >From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001
> > From: Nagy Gabor <ngaba@bibl.u-szeged.hu>
> > Date: Tue, 15 Jul 2008 13:30:34 +0200
> > Subject: [PATCH] Fix some memleaks in alpm/add.c
> >
> > In case of error some allocated memory wasn't freed in commit_single_pkg.
> > Note: The return value of this function is not used.
> >
> > Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu>
> > ---
>
> Thanks. Perhaps we should use the return value?
>

Well, now our add code won't stop in case of error just go ahead;-)
As I guessed this behaviour change was introduced by commit
591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge add_commit
function. So to answer to your question, I think we should. Others?

Bye


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/


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

2008/7/16 Nagy Gabor <ngaba@bibl.u-szeged.hu>:
> Idézés Dan McGee <dpmcgee@gmail.com>:
>
>> On Tue, Jul 15, 2008 at 6:36 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
>> > >From b9673a106b1f985cb81f620f394620e6eca88516 Mon Sep 17 00:00:00 2001
>> > From: Nagy Gabor <ngaba@bibl.u-szeged.hu>
>> > Date: Tue, 15 Jul 2008 13:30:34 +0200
>> > Subject: [PATCH] Fix some memleaks in alpm/add.c
>> >
>> > In case of error some allocated memory wasn't freed in commit_single_pkg.
>> > Note: The return value of this function is not used.
>> >
>> > Signed-off-by: Nagy Gabor <ngaba@bibl.u-szeged.hu>
>> > ---
>>
>> Thanks. Perhaps we should use the return value?
>>
>
> Well, now our add code won't stop in case of error just go ahead;-)
> As I guessed this behaviour change was introduced by commit
> 591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge add_commit
> function. So to answer to your question, I think we should. Others?
>

There is no point discussing whether we should use it or not, what is
important is to discuss *how* to use it.
There are two main ways :
1) just printing a message : error, warning or just debug ?
2) stops here, and probably returning -1 . Should we run ldconfig
before returning?

The ldconfig question in case 2 also applies to the case where we
cancel a transaction with ctrl+c (sate_interrupted).

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

> The ldconfig question in case 2 also applies to the case where we
> cancel a transaction with ctrl+c (sate_interrupted).
>
Personally I would put ldconfig to trans.c (alpm_trans_commit?).

Bye

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

> > Well, now our add code won't stop in case of error just go ahead;-)
> > As I guessed this behaviour change was introduced by commit
> > 591bfabbd38bf4f8f209977f416a4e5fd3cc2baf, where we split the huge
> > add_commit function. So to answer to your question, I think we
> > should. Others?
> >
>
> There is no point discussing whether we should use it or not, what is
> important is to discuss *how* to use it.
> There are two main ways :
> 1) just printing a message : error, warning or just debug ?

Most of them is printed via alpm_log in the split function.

> 2) stops here, and probably returning -1 . Should we run ldconfig
> before returning?

Before the mentioned commit, we followed 2) without ldconfig. Now we
don't stop (and thus run ldconfig always).

>
> The ldconfig question in case 2 also applies to the case where we
> cancel a transaction with ctrl+c (sate_interrupted).
>

I can cancel the transaction in the middle of a commit? (I mean
"between" packages...) That's not good. If pacman upgraded only the half
of the packages, inconsistent database is predicted. (Maybe I'm too
strict here.)

This is true for the original question, the ability to rollback the
transaction would be the best there. Without rollback I have no clue...
Maybe the current one is better. Others?

Bye

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