[pacman-dev] pacman loop

[Xavier]

On Sat, Jul 07, 2007 at 09:27:33PM +0200, Xavier wrote:
> On Fri, Jul 06, 2007 at 11:29:55PM +0200, Xavier wrote:
> > There is a second problem (still unusual case) that I need to figure out :
> > when first installing a package as root, then installing a package as user,
> > I get the error and behavior as described here :
> > http://www.archlinux.org/pipermail/pacman-dev/2007-April/008008.html
> > ie pacman freezes and can't even be cancelled with ctrl+C.
> > 
> > It would be nicer if it exited cleanly instead :)
> 
> The problem is that when a transaction fails, trans->state isn't set
> to STATE_COMMITED, but stays on STATE_COMMITING
> (see _alpm_trans_commit function in libalm/trans.c).
> Then when _alpm_trans_release is called, it does the following :
> 
> 195   /* during a commit do not interrupt immediately, just after a target */
> 196   if(trans->state == STATE_COMMITING || trans->state == STATE_INTERRUPTED) {
> 197     if(trans->state == STATE_COMMITING) {
> 198       trans->state = STATE_INTERRUPTED;
> 199     }
> 200     pm_errno = PM_ERR_TRANS_COMMITING;
> 201     return(-1);
> 202   }
> 
> Does anyone understand the comment above?
> Anyway, now the state is set to INTERRUPTED.
> 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,
> 80     alpm_db_unregister(db);
> 81     db = NULL;
> 82   }
> 
> There is an infinite loop there when alpm_db_unregister (db.c) fails,
> and it fails when handle->trans isn't NULL :
> 86   /* Do not unregister a database if a transaction is on-going */
> 87   ASSERT(handle->trans == NULL, RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
> 
> Could we consider that when trans is not NULL, but trans->state
> is set to STATE_INTERRUPTED, then it isn't a on-going transaction anymore,
> and so we can unregister the database safely ?
> 
> 
> From b130e08ed216e3a9caa0026bec213d9a0a5fa094 Mon Sep 17 00:00:00 2001
> From: Chantry Xavier <shiningxc at gmail.com>
> Date: Sat, 7 Jul 2007 21:24:30 +0200
> Subject: [PATCH] libalpm/db.c : allow unregistering db for interrupted transaction.
> 
> This prevents alpm_release to loop infinitely in case of
> an interrupted transaction, where the database wasn't
> unregistered.
> alpm_release should probably also be fixed, as it can
> still loop if db_unregister fails for another reason.
> 
> Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
> ---
>  lib/libalpm/db.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
> index 95a1ecb..7cc9390 100644
> --- a/lib/libalpm/db.c
> +++ b/lib/libalpm/db.c
> @@ -84,7 +84,8 @@ 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, RET_ERR(PM_ERR_TRANS_NOT_NULL, -1));
> +	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;
> -- 
> 1.5.2.2
> 

While looking at FS#7652, I found something new :
this problem doesn't happen on -S operations, only on -U, because of the
following code in sync.c , _alpm_sync_commit function :

1027 error:
1028   _alpm_trans_free(tr);
1029   tr = NULL;
1030   /* commiting failed, so this is still just a prepared transaction */
1031   trans->state = STATE_PREPARED;
1032   return(-1);

So if an error happens in sync code, the state goes back to STATE_PREPARED.
That looks even more hackish than my patch above, but if it's better, then the
same should be done in _alpm_add_commit and _alpm_remove_commit.
Otherwise, it should be removed and handled somewhere else, like with my
patch.
Which way should we take?