On Mon, Jul 09, 2007 at 05:06:05PM +0200, Xavier wrote:
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=55f178c629ada...
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).
Maybe it was not ok to remove that alpm_db_unregister public function, so I kept it, and added a warning in a comment instead. But now I provide an additional alpm_db_unregister_all() function, so this should fix all problems. So this might be a better patch :
From c31156cf14201fa7533057d75be82f330ca3dad4 Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.com> Date: Mon, 9 Jul 2007 17:02:29 +0200 Subject: [PATCH] libalpm/db.c : add alpm_db_unregister_all.
This basically moves the code from alpm_release, which was mostly about unregistering all databases, to a safer alpm_db_unregister_all. This allows to avoid modifying the dbs_sync list while iterating over it, and and also prevent alpm_release from looping infinitely when a database can't be unregistered. Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/alpm.c | 16 +----------- lib/libalpm/alpm.h | 1 + lib/libalpm/db.c | 63 ++++++++++++++++++++++++++++++++++++++++++--------- lib/libalpm/db.h | 1 + 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index e3ad211..20d4a74 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -62,24 +62,12 @@ int SYMEXPORT alpm_initialize(void) */ int SYMEXPORT alpm_release(void) { - int dbs_left = 0; - ALPM_LOG_FUNC; ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_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; + if(alpm_db_unregister_all() == -1) { + return(-1); } _alpm_handle_free(handle); diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 9e641f3..98028de 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -145,6 +145,7 @@ alpm_list_t *alpm_option_get_syncdbs(); pmdb_t *alpm_db_register(const char *treename); int alpm_db_unregister(pmdb_t *db); +int alpm_db_unregister_all(); const char *alpm_db_get_name(const pmdb_t *db); const char *alpm_db_get_url(const pmdb_t *db); diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index 306d0b9..aa02352 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -70,6 +70,35 @@ pmdb_t SYMEXPORT *alpm_db_register(const char *treename) return(_alpm_db_register(treename)); } +/** Unregister all package databases + * @return 0 on success, -1 on error (pm_errno is set accordingly) + */ +int SYMEXPORT alpm_db_unregister_all() +{ + alpm_list_t *i; + + ALPM_LOG_FUNC; + + /* 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 */ + _alpm_db_unregister(handle->db_local); + handle->db_local = NULL; + + /* and also sync ones */ + for(i = handle->dbs_sync; i; i = i->next) { + pmdb_t *db = i->data; + _alpm_db_unregister(db); + i->data = NULL; + } + FREELIST(handle->dbs_sync); + return(0); +} + /** Unregister a package database * @param db pointer to the package database to unregister * @return 0 on success, -1 on error (pm_errno is set accordingly) @@ -84,13 +113,17 @@ int SYMEXPORT alpm_db_unregister(pmdb_t *db) 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, + 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 { + /* Warning : this function shouldn't be used to unregister all sync databases + * by walking through the list returned by alpm_option_get_syncdbs, + * because the db is removed from that list here. + */ void *data; handle->dbs_sync = alpm_list_remove(handle->dbs_sync, db, _alpm_db_cmp, &data); if(data) { @@ -102,16 +135,7 @@ int SYMEXPORT alpm_db_unregister(pmdb_t *db) RET_ERR(PM_ERR_DB_NOT_FOUND, -1); } - _alpm_log(PM_LOG_DEBUG, "unregistering database '%s'", db->treename); - - /* Cleanup */ - _alpm_db_free_pkgcache(db); - - _alpm_log(PM_LOG_DEBUG, "closing database '%s'", db->treename); - _alpm_db_close(db); - - _alpm_db_free(db); - + _alpm_db_unregister(db); return(0); } @@ -542,6 +566,23 @@ error: /** @} */ +/* Helper function for alpm_db_unregister{_all} */ +void _alpm_db_unregister(pmdb_t *db) +{ + if(db == NULL) { + return; + } + _alpm_log(PM_LOG_DEBUG, "unregistering database '%s'", db->treename); + + /* Cleanup */ + _alpm_db_free_pkgcache(db); + + _alpm_log(PM_LOG_DEBUG, "closing database '%s'", db->treename); + _alpm_db_close(db); + + _alpm_db_free(db); +} + pmdb_t *_alpm_db_new(const char *dbpath, const char *treename) { pmdb_t *db; diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index 2597c8c..d31bf60 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); +void _alpm_db_unregister(pmdb_t *db); /* be.c, backend specific calls */ int _alpm_db_install(pmdb_t *db, const char *dbfile); -- 1.5.2.4