[pacman-dev] logging output crazy

[Xavier]

On Sat, Jul 07, 2007 at 09:44:47PM +0200, Xavier wrote:
> On Sat, Jul 07, 2007 at 09:27:33PM +0200, Xavier wrote:
> > And the alpm_release function in libalpm/alpm.c :
> > 
> > 77   while((dbs_left = alpm_list_count(handle->dbs_sync)) > 0) {
> > 78     pmdb_t *db = (pmdb_t *)handle->dbs_sync->data;
> > 79     _alpm_log(PM_LOG_DEBUG, _("removing DB %s, %d remaining..."), db->treename, dbs_left);
> > 80     alpm_db_unregister(db);
> > 81     db = NULL;
> > 82   }
> > 
> > There is an infinite loop there when alpm_db_unregister (db.c) fails,
> 
> oops the _alpm_log line was cut during pasting, fixed it :)
> 
> Anyway, I forgot to mention I found where this code was introduced :
> http://projects.archlinux.org/git/?p=pacman.git;a=commitdiff;h=55f178c629ada663d2a8e5bbed029ec9482f00ea
> 
> So it looks like what the previous code did was even worse :)
> While I'm not very happy about how it was fixed, 
> I can't find a better way to fix it..
> I still think it's not very nice to have a code like that which can loop
> infinitely so easily.

Erm, what can make alpm_db_unregister(db) fail isn't even specific to the db,
so the same checks are done multiple times.
Couldn't we have these checks done in alpm_release() instead, and have a
private _alpm_db_unregister(db) that wouldn't fail, instead of a public one?
The only thing that alpm_release do is unregistering databases anyway, so the
public alpm_db_unregister(db) isn't even used by pacman.

That would make the code cleaner and safer without losing any functionality imo.
The only thing that is worrying me is that currently, there are symetrical
pmdb_t *alpm_db_register() and alpm_db_unregister(pmdb_t*) functions, which
makes sense, but the following patch removes that (from the client pov).

>From 642a6509094b130ca7edaf312cf5e1a75eafff1e Mon Sep 17 00:00:00 2001
From: Chantry Xavier <shiningxc at gmail.com>
Date: Mon, 9 Jul 2007 17:02:29 +0200
Subject: [PATCH] makes alpm_db_unregister(pmdb_t *) private.

This allows to clean the alpm_db_unregister and alpm_release functions a bit,
and prevent alpm_release from looping infinitely.

Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
---
 lib/libalpm/alpm.c |   26 +++++++++++++-------------
 lib/libalpm/db.c   |   25 +------------------------
 lib/libalpm/db.h   |    1 +
 3 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index 11d00d4..a1abebc 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -62,23 +62,23 @@ int SYMEXPORT alpm_initialize(void)
  */
 int SYMEXPORT alpm_release(void)
 {
-	int dbs_left = 0;
-
 	ALPM_LOG_FUNC;
 
+	alpm_list_t *i;
+
+	/* Sanity checks */
 	ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
+	/* Do not unregister a database if a transaction is on-going */
+	ASSERT(handle->trans == NULL || handle->trans->state == STATE_INTERRUPTED, 
+			RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
 
-	/* close local database */
-	if(handle->db_local) {
-		alpm_db_unregister(handle->db_local);
-		handle->db_local = NULL;
-	}
-	/* and also sync ones */
-	while((dbs_left = alpm_list_count(handle->dbs_sync)) > 0) {
-		pmdb_t *db = (pmdb_t *)handle->dbs_sync->data;
-		_alpm_log(PM_LOG_DEBUG, _("removing DB %s, %d remaining..."), db->treename, dbs_left);
-		alpm_db_unregister(db);
-		db = NULL;
+	_alpm_db_unregister(handle->db_local);
+	handle->db_local = NULL;
+
+	for(i = handle->dbs_sync; i; i = i->next) {
+		pmdb_t *db = i->data;
+		_alpm_db_unregister(db);
+		i->data = NULL;
 	}
 
 	_alpm_handle_free(handle);
diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
index 7cc9390..5177aeb 100644
--- a/lib/libalpm/db.c
+++ b/lib/libalpm/db.c
@@ -74,33 +74,10 @@ pmdb_t SYMEXPORT *alpm_db_register(const char *treename)
  * @param db pointer to the package database to unregister
  * @return 0 on success, -1 on error (pm_errno is set accordingly)
  */
-int SYMEXPORT alpm_db_unregister(pmdb_t *db)
+int _alpm_db_unregister(pmdb_t *db)
 {
-	int found = 0;
-
-	ALPM_LOG_FUNC;
-
 	/* Sanity checks */
-	ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
 	ASSERT(db != NULL, RET_ERR(PM_ERR_WRONG_ARGS, -1));
-	/* Do not unregister a database if a transaction is on-going */
-	ASSERT(handle->trans == NULL || handle->trans->state == STATE_INTERRUPTED, 
-			RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
-
-	if(db == handle->db_local) {
-		handle->db_local = NULL;
-		found = 1;
-	} else {
-		void *data;
-		handle->dbs_sync = alpm_list_remove(handle->dbs_sync, db, _alpm_db_cmp, &data);
-		if(data) {
-			found = 1;
-		}
-	}
-
-	if(!found) {
-		RET_ERR(PM_ERR_DB_NOT_FOUND, -1);
-	}
 
 	_alpm_log(PM_LOG_DEBUG, _("unregistering database '%s'"), db->treename);
 
diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h
index 2597c8c..5aaca69 100644
--- a/lib/libalpm/db.h
+++ b/lib/libalpm/db.h
@@ -53,6 +53,7 @@ void _alpm_db_free(pmdb_t *db);
 int _alpm_db_cmp(const void *db1, const void *db2);
 alpm_list_t *_alpm_db_search(pmdb_t *db, const alpm_list_t *needles);
 pmdb_t *_alpm_db_register(const char *treename);
+int _alpm_db_unregister(pmdb_t *db);
 
 /* be.c, backend specific calls */
 int _alpm_db_install(pmdb_t *db, const char *dbfile);
-- 
1.5.2.2