[pacman-dev] trans.c : reworking of transaction interruptions.
My two previous hack related to this part (8038190c7c4786e1c49494eea1b40cdddcbd5136 and b15a5194d1a8485a2769560e49e6ff03e1862533) were caused by the lack of understanding of a feature introduced a while ago : Better control over CTRL-C interruptions -- do not leave the DB in an inconsistent state (54008798efcc9646f622f6b052ecd83281d57cda). Now I have been looking at this commit, and the added feature is indeed interesting. The main problem I had with it is that it does a rather unusual use of alpm_trans_release, which caused a few problems that I tried to fix in a weird way. I think these problems were caused by the fact that there wasn't any difference between an "interrupt transaction" and a "release a transaction which failed" actions from alpm_trans_release POV. So I decided to add a new function instead, alpm_trans_interrupt, which is called on Ctrl+C, and which only sets trans->state to STATE_INTERRUPTED so that remove_commit and add_commit can exit cleanly at a safe moment. This allowed me to revert my two previous hacks as well. Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/add.c | 8 +++----- lib/libalpm/alpm.h | 1 + lib/libalpm/db.c | 6 ++---- lib/libalpm/remove.c | 5 ++--- lib/libalpm/trans.c | 33 ++++++++++++++++++++++----------- src/pacman/pacman.c | 4 ++-- 6 files changed, 32 insertions(+), 25 deletions(-)
On Sat, Sep 08, 2007 at 12:45:14AM +0200, Xavier wrote:
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index f62e588..2db4346 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -208,8 +208,8 @@ static void cleanup(int signum) pm_fprintf(stderr, PM_LOG_ERROR, "Internal pacman error: Segmentation fault.\n" "Please submit a full bug report with --debug if appropriate.\n"); exit(signum); - } else if((signum == SIGINT) && (alpm_trans_release() == -1) - && (pm_errno == PM_ERR_TRANS_COMMITING)) { + } else if((signum == SIGINT)) { + alpm_trans_interrupt(); return; }
hmm, my brain wasn't correctly working when I did this, I guess. I didn't like this part, but instead of rewriting it, I just removed it without trying to understand... The previous code only caught ctrl+C when pacman was commiting a trans, and only in this case it didn't exit directly. In all other cases, it exited. With my patch, it never exits. I actually wonder if that doesn't invalid my whole patch, I am not sure yet and have to think more about it. Dan, please don't pull this yet, and sorry.
On Tue, Sep 11, 2007 at 02:18:40PM +0200, Xavier wrote:
On Sat, Sep 08, 2007 at 12:45:14AM +0200, Xavier wrote:
diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index f62e588..2db4346 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -208,8 +208,8 @@ static void cleanup(int signum) pm_fprintf(stderr, PM_LOG_ERROR, "Internal pacman error: Segmentation fault.\n" "Please submit a full bug report with --debug if appropriate.\n"); exit(signum); - } else if((signum == SIGINT) && (alpm_trans_release() == -1) - && (pm_errno == PM_ERR_TRANS_COMMITING)) { + } else if((signum == SIGINT)) { + alpm_trans_interrupt(); return; }
hmm, my brain wasn't correctly working when I did this, I guess. I didn't like this part, but instead of rewriting it, I just removed it without trying to understand...
The previous code only caught ctrl+C when pacman was commiting a trans, and only in this case it didn't exit directly. In all other cases, it exited. With my patch, it never exits.
I actually wonder if that doesn't invalid my whole patch, I am not sure yet and have to think more about it.
Dan, please don't pull this yet, and sorry.
I see the main problem is to detect if a transaction is being commited or not. If yes, we just return from that cleanup function, and we wait for the transaction to end. If no, we can release the transaction and exit pacman. The code previously used a return code of -1 from alpm_trans_release + pm_errno for detecting that. I thought about using a return code of 0 from alpm_trans_interrupt for replacing it. I don't know if it makes the code more clear or more obscure though...
From a7c9f19e86900d128aa6722094a0abc7978fc29e Mon Sep 17 00:00:00 2001 From: Chantry Xavier <shiningxc@gmail.com> Date: Wed, 12 Sep 2007 19:09:55 +0200 Subject: [PATCH] Handle SIGINT correctly in all cases.
Signed-off-by: Chantry Xavier <shiningxc@gmail.com> --- lib/libalpm/trans.c | 3 ++- src/pacman/pacman.c | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index dbc6038..9eb27c3 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -179,7 +179,8 @@ int SYMEXPORT alpm_trans_interrupt() trans = handle->trans; ASSERT(trans != NULL, RET_ERR(PM_ERR_TRANS_NULL, -1)); - ASSERT(trans->state != STATE_IDLE, RET_ERR(PM_ERR_TRANS_NULL, -1)); + ASSERT(trans->state == STATE_COMMITING || trans->state == STATE_INTERRUPTED, + RET_ERR(PM_ERR_TRANS_TYPE, -1)); trans->state = STATE_INTERRUPTED; diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 2db4346..540025b 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -209,8 +209,13 @@ static void cleanup(int signum) "Please submit a full bug report with --debug if appropriate.\n"); exit(signum); } else if((signum == SIGINT)) { - alpm_trans_interrupt(); - return; + if(alpm_trans_interrupt() == 0) { + /* a transaction is being interrupted, don't exit pacman yet. */ + return; + } else { + /* no commiting transaction, we can release it now and then exit pacman */ + alpm_trans_release(); + } } /* free alpm library resources */ -- 1.5.3
participants (1)
-
Xavier