Re: [pacman-dev] Transaction failure handling

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 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 --- 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 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 --- 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