[pacman-dev] Transaction failure handling

[Xavier]

On Fri, Aug 17, 2007 at 03:47:22PM +0200, Xavier wrote:
> On Thu, Aug 09, 2007 at 12:06:06PM +0200, Xavier wrote:
> > 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?
> 
> Since Dan apparently started merging my unregister branch, I removed the two
> above lines, since I think it doesn't make sense, and it's not consistent 
> with the add and remove transactions.
> 
> 
> From c2370ccbd322951d5c11f70096a440782de8c269 Mon Sep 17 00:00:00 2001
> From: Chantry Xavier <shiningxc at gmail.com>
> Date: Fri, 17 Aug 2007 14:07:56 +0200
> Subject: [PATCH] libalpm/sync.c : don't go back on STATE_PREPARED when committing fails.
> 
> In my opinion, a commiting transaction that failed isn't equivalent
> to a prepared transaction.
> Some things could have been done in the meantime.
> 
> Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
> ---
>  lib/libalpm/sync.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 005123d..6c98aa3 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -1027,8 +1027,6 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data)
>  error:
>  	_alpm_trans_free(tr);
>  	tr = NULL;
> -	/* commiting failed, so this is still just a prepared transaction */
> -	trans->state = STATE_PREPARED;
>  	return(-1);
>  }
>  
> -- 
> 1.5.2.5
> 

That causes another problem though...
in _alpm_trans_release, we have :

184         /* during a commit do not interrupt immediately, just after a target */
185         if(trans->state == STATE_COMMITING || trans->state == STATE_INTERRUPTED) {
186                 if(trans->state == STATE_COMMITING) {
187                         trans->state = STATE_INTERRUPTED;
188                 }
189                 pm_errno = PM_ERR_TRANS_COMMITING;
190                 return(-1);
191         }

So since the state now stays on STATE_COMMITING (without going back to STATE_PREPARED)
when a transaction fails, this code is now run, and the function returns here,
without removing the lock.

Now, I'm totally unable to fix this correctly, the thing is, I really don't understand
the purpose of this part, what it's intended to do, in which cases, etc ...

It annoyed me that pacman didn't remove the lock on each error (for example,
conflicting targets), so I had to hack this, but that most likely kills totally what
the original author had in mind, making the code worse than it is.

I guess what I'm trying to say here is that I need someone to look at this,
and fix it correctly.
Maybe I could revert that previous change in sync.c, so that the state goes back to
STATE_PREPARED on failure, and then do the same in add.c / remove.c , but I
still don't like that idea..


>From 7204d3afc160fe02f6d7d8a4be094a8cc0861dad Mon Sep 17 00:00:00 2001
From: Chantry Xavier <shiningxc at gmail.com>
Date: Thu, 30 Aug 2007 00:39:57 +0200
Subject: [PATCH] libalpm/trans.c : remove the lock even on interrupted transactions.

Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
---
 lib/libalpm/trans.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c
index c0e38c7..fe37a1f 100644
--- a/lib/libalpm/trans.c
+++ b/lib/libalpm/trans.c
@@ -171,6 +171,7 @@ int SYMEXPORT alpm_trans_commit(alpm_list_t **data)
 int SYMEXPORT alpm_trans_release()
 {
 	pmtrans_t *trans;
+	int ret = 0;
 
 	ALPM_LOG_FUNC;
 
@@ -187,7 +188,7 @@ int SYMEXPORT alpm_trans_release()
 			trans->state = STATE_INTERRUPTED;
 		}
 		pm_errno = PM_ERR_TRANS_COMMITING;
-		return(-1);
+		ret = -1;
 	}
 
 	_alpm_trans_free(trans);
@@ -205,7 +206,7 @@ int SYMEXPORT alpm_trans_release()
 				alpm_option_get_lockfile());
 	}
 
-	return(0);
+	return(ret);
 }
 
 /** @} */
-- 
1.5.2.5