[pacman-dev] Transaction failure handling

[Andrew Fyfe]

Xavier wrote:
> 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..
> 

I think this is another +1 for a rewrite. It seems for every bug we fix 
another harder to fix one pops up to take its place.

Andrew