[Rpm-maint] RFC: Unblocking of signals within rpm
Panu Matilainen
pmatilai at redhat.com
Thu Jul 19 09:58:29 UTC 2007
Attached patch turns the signal handling within rpmdb upside down: we no
longer run the whole damn thing with termination signals blocked with
occasional checking possibly resulting in exit() from deep within rpmlib,
but instead have an active signal handler that will mop up any open
transactions and iterators and pass on the signal to caller.
In practise it means that ctrl-c is responsive again (yes, in yum as well)
without giving up proper locking.
What the patch does is (or at least, supposed to do ;)
- add a termination function that can clean up open iterators etc
- save caller original signal handlers
- add a signal handler that does rpmdb related cleanup and passes the
signal to caller afterwards
- internally export block/unblockSignals() calls, remove unused db
parameter from them
- specifically protect critical sections with blocked signals (rpmtsRun,
db rebuild)
- remove now useless rpmdbCheckSignals() and its uses
Additionally it adds a kind of related atexit() hook to the termination
function for python on module load so that any open iterators etc will get
cleaned up in case python caller tracebacks. Rpm not cleaning up on
unclean exit from python would appear to be a *huge* cause of stale locks
these days...
I'd appreciate if folks could eyeball this for stupid mistakes like
re-entrancy issues wrt signals on exit from various paths, missed critical
sections that need signal protection etc. The patch is on top of current
rpm.org head but appears to apply to 4.4.2.1 cleanly as well.
- Panu -
-------------- next part --------------
diff -r d8e2ec20c948 lib/query.c
--- a/lib/query.c Wed Jul 18 16:05:56 2007 +0300
+++ b/lib/query.c Thu Jul 19 11:29:30 2007 +0300
@@ -454,8 +454,6 @@ int rpmQueryVerify(QVA_t qva, rpmts ts,
int i;
int provides_checked = 0;
- (void) rpmdbCheckSignals();
-
if (qva->qva_showPackage == NULL)
return 1;
diff -r d8e2ec20c948 lib/transaction.c
--- a/lib/transaction.c Wed Jul 18 16:05:56 2007 +0300
+++ b/lib/transaction.c Thu Jul 19 12:21:40 2007 +0300
@@ -10,6 +10,7 @@
#include "fsm.h"
#include "psm.h"
+#define _RPMDB_INTERNAL
#include "rpmdb.h"
#include "rpmds.h"
@@ -1420,6 +1421,7 @@ int rpmtsRun(rpmts ts, rpmps okProbs, rp
int rollbackOnFailure = 0;
void * lock = NULL;
int xx;
+ sigset_t oldMask;
/* XXX programmer error segfault avoidance. */
if (rpmtsNElements(ts) <= 0)
@@ -1571,6 +1573,14 @@ rpmMessage(RPMMESS_DEBUG, _("sanity chec
totalFileCount += fc;
}
pi = rpmtsiFree(pi);
+
+ /*
+ * Point of no return, block signals.
+ * XXX don't bother on test mode?
+ * XXX should probably block before taking the lock, but signal handler
+ * cleans 'em up now...
+ */
+ blockSignals(&oldMask);
/* Run pre-transaction scripts, but only if there are no known
@@ -1706,8 +1716,6 @@ rpmMessage(RPMMESS_DEBUG, _("computing %
while ((p = rpmtsiNext(pi, 0)) != NULL) {
int fc;
- (void) rpmdbCheckSignals();
-
if ((fi = rpmtsiFi(pi)) == NULL)
continue; /* XXX can't happen */
fc = rpmfiFC(fi);
@@ -1743,8 +1751,6 @@ rpmMessage(RPMMESS_DEBUG, _("computing f
dbiIndexSet * matches;
int knownBad;
int fc;
-
- (void) rpmdbCheckSignals();
if ((fi = rpmtsiFi(pi)) == NULL)
continue; /* XXX can't happen */
@@ -1978,8 +1984,6 @@ rpmMessage(RPMMESS_DEBUG, _("computing f
pi = rpmtsiInit(ts);
while ((p = rpmtsiNext(pi, 0)) != NULL) {
- (void) rpmdbCheckSignals();
-
if ((fi = rpmtsiFi(pi)) == NULL)
continue; /* XXX can't happen */
switch (rpmteType(p)) {
@@ -2031,8 +2035,6 @@ assert(psm != NULL);
while ((p = rpmtsiNext(pi, 0)) != NULL) {
alKey pkgKey;
int gotfd;
-
- (void) rpmdbCheckSignals();
gotfd = 0;
if ((fi = rpmtsiFi(pi)) == NULL)
@@ -2386,6 +2388,8 @@ assert(psm != NULL);
}
rpmtsFreeLock(lock);
+ /* Out of critical section, unblock... */
+ unblockSignals(&oldMask);
/*@-nullstate@*/ /* FIX: ts->flList may be NULL */
if (ourrc)
diff -r d8e2ec20c948 python/rpmmodule.c
--- a/python/rpmmodule.c Wed Jul 18 16:05:56 2007 +0300
+++ b/python/rpmmodule.c Thu Jul 19 12:04:59 2007 +0300
@@ -214,6 +214,11 @@ void init_rpm(void)
m = Py_InitModule3("_rpm", rpmModuleMethods, rpm__doc__);
if (m == NULL)
return;
+
+ /* Mop up any leftover iterators at exit (traceback or otherwise) */
+ if (Py_AtExit(rpmdbTerminate) == -1) {
+ return;
+ }
rpmReadConfigFiles(NULL, NULL);
diff -r d8e2ec20c948 rpmdb/rpmdb.c
--- a/rpmdb/rpmdb.c Wed Jul 18 16:05:56 2007 +0300
+++ b/rpmdb/rpmdb.c Thu Jul 19 12:20:26 2007 +0300
@@ -32,6 +32,7 @@ extern void regfree (/*@only@*/ regex_t
#include <rpmmacro.h>
#include <rpmsq.h>
+#define _RPMDB_INTERNAL
#include "rpmdb.h"
#include "fprint.h"
#include "legacy.h"
@@ -700,32 +701,22 @@ static rpmdb rpmdbRock;
/*@unchecked@*/ /*@exposed@*/ /*@null@*/
static rpmdbMatchIterator rpmmiRock;
-int rpmdbCheckSignals(void)
- /*@globals rpmdbRock, rpmmiRock @*/
- /*@modifies rpmdbRock, rpmmiRock @*/
+static volatile sig_atomic_t terminating = 0;
+
+void rpmdbTerminate(void)
{
sigset_t newMask, oldMask;
- static int terminate = 0;
-
- if (terminate) return 0;
+
+ if (terminating) return;
(void) sigfillset(&newMask); /* block all signals */
(void) sigprocmask(SIG_BLOCK, &newMask, &oldMask);
- if (sigismember(&rpmsqCaught, SIGINT)
- || sigismember(&rpmsqCaught, SIGQUIT)
- || sigismember(&rpmsqCaught, SIGHUP)
- || sigismember(&rpmsqCaught, SIGTERM)
- || sigismember(&rpmsqCaught, SIGPIPE))
- terminate = 1;
-
- if (terminate) {
+ terminating = 1;
+
+ if (terminating) {
rpmdb db;
rpmdbMatchIterator mi;
-
-/*@-abstract@*/ /* sigset_t is abstract type */
- rpmMessage(RPMMESS_DEBUG, "Exiting on signal(0x%lx) ...\n", *((unsigned long *)&rpmsqCaught));
-/*@=abstract@*/
/*@-branchstate@*/
while ((mi = rpmmiRock) != NULL) {
@@ -742,17 +733,29 @@ int rpmdbCheckSignals(void)
(void) rpmdbClose(db);
}
/*@=newreftrans@*/
- exit(EXIT_FAILURE);
- }
- return sigprocmask(SIG_SETMASK, &oldMask, NULL);
+ }
+ sigprocmask(SIG_SETMASK, &oldMask, NULL);
+ return;
+}
+
+static void rpmdbSigHandler(int signum, void *info, void *context)
+{
+ if (terminating) {
+ raise(signum);
+ }
+ rpmdbTerminate();
+ /* restore original signal handler */
+ rpmsqEnable(-signum, NULL);
+ raise(signum);
+
+ return;
}
/**
* Block all signals, returning previous signal mask.
*/
-static int blockSignals(/*@unused@*/ rpmdb db, /*@out@*/ sigset_t * oldMask)
- /*@globals fileSystem @*/
- /*@modifies *oldMask, fileSystem @*/
+int blockSignals(sigset_t * oldMask)
+ /*@modifies *oldMask @*/
{
sigset_t newMask;
@@ -769,12 +772,8 @@ static int blockSignals(/*@unused@*/ rpm
/**
* Restore signal mask.
*/
-/*@mayexit@*/
-static int unblockSignals(/*@unused@*/ rpmdb db, sigset_t * oldMask)
- /*@globals rpmdbRock, fileSystem, internalState @*/
- /*@modifies rpmdbRock, fileSystem, internalState @*/
-{
- (void) rpmdbCheckSignals();
+int unblockSignals(sigset_t * oldMask)
+{
return sigprocmask(SIG_SETMASK, oldMask, NULL);
}
@@ -1042,11 +1041,11 @@ static int openDatabase(/*@null@*/ const
if (db == NULL)
return 1;
- (void) rpmsqEnable(SIGHUP, NULL);
- (void) rpmsqEnable(SIGINT, NULL);
- (void) rpmsqEnable(SIGTERM,NULL);
- (void) rpmsqEnable(SIGQUIT,NULL);
- (void) rpmsqEnable(SIGPIPE,NULL);
+ (void) rpmsqEnable(SIGHUP, rpmdbSigHandler);
+ (void) rpmsqEnable(SIGINT, rpmdbSigHandler);
+ (void) rpmsqEnable(SIGTERM, rpmdbSigHandler);
+ (void) rpmsqEnable(SIGQUIT, rpmdbSigHandler);
+ (void) rpmsqEnable(SIGPIPE, rpmdbSigHandler);
db->db_api = _dbapi;
@@ -1645,7 +1644,7 @@ static int miFreeHeader(rpmdbMatchIterat
}
if (data->data != NULL && rpmrc != RPMRC_FAIL) {
- (void) blockSignals(dbi->dbi_rpmdb, &signalMask);
+ (void) blockSignals(&signalMask);
rc = dbiPut(dbi, mi->mi_dbc, key, data, DB_KEYLAST);
if (rc) {
rpmError(RPMERR_DBPUTINDEX,
@@ -1653,7 +1652,7 @@ static int miFreeHeader(rpmdbMatchIterat
rc, mi->mi_prevoffset, tagName(dbi->dbi_rpmtag));
}
xx = dbiSync(dbi, 0);
- (void) unblockSignals(dbi->dbi_rpmdb, &signalMask);
+ (void) unblockSignals(&signalMask);
}
data->data = _free(data->data);
data->size = 0;
@@ -1714,8 +1713,6 @@ rpmdbMatchIterator rpmdbFreeIterator(rpm
mi->mi_db = rpmdbUnlink(mi->mi_db, "matchIterator");
mi = _free(mi);
-
- (void) rpmdbCheckSignals();
return mi;
}
@@ -2484,8 +2481,6 @@ rpmdbMatchIterator rpmdbInitIterator(rpm
if (db == NULL)
return NULL;
- (void) rpmdbCheckSignals();
-
/* XXX HACK to remove rpmdbFindByLabel/findMatches from the API */
if (rpmtag == RPMDBI_LABEL) {
rpmtag = RPMTAG_NAME;
@@ -2658,7 +2653,7 @@ memset(data, 0, sizeof(*data));
rpmMessage(RPMMESS_DEBUG, " --- h#%8u %s-%s-%s\n", hdrNum, n, v, r);
}
- (void) blockSignals(db, &signalMask);
+ (void) blockSignals(&signalMask);
/*@-nullpass -nullptrarith -nullderef @*/ /* FIX: rpmvals heartburn */
{ int dbix;
@@ -2908,7 +2903,7 @@ if (key->size == 0) key->size++; /* XXX
}
/*@=nullpass =nullptrarith =nullderef @*/
- (void) unblockSignals(db, &signalMask);
+ (void) unblockSignals(&signalMask);
h = headerFree(h);
@@ -2972,7 +2967,7 @@ memset(data, 0, sizeof(*data));
if (_noDirTokens)
expandFilelist(h);
- (void) blockSignals(db, &signalMask);
+ (void) blockSignals(&signalMask);
{
unsigned int firstkey = 0;
@@ -3355,7 +3350,7 @@ if (key->size == 0) key->size++; /* XXX
}
exit:
- (void) unblockSignals(db, &signalMask);
+ (void) unblockSignals(&signalMask);
return ret;
}
@@ -3742,6 +3737,7 @@ int rpmdbRebuild(const char * prefix, rp
int rc = 0, xx;
int _dbapi;
int _dbapi_rebuild;
+ sigset_t oldMask;
/*@-branchstate@*/
if (prefix == NULL) prefix = "/";
@@ -3834,6 +3830,8 @@ int rpmdbRebuild(const char * prefix, rp
_rebuildinprogress = 0;
_dbapi_rebuild = newdb->db_api;
+
+ blockSignals(&oldMask);
{ Header h = NULL;
rpmdbMatchIterator mi;
@@ -3905,6 +3903,8 @@ int rpmdbRebuild(const char * prefix, rp
xx = rpmdbClose(olddb);
xx = rpmdbClose(newdb);
+ unblockSignals(&oldMask);
+
if (failed) {
rpmMessage(RPMMESS_NORMAL, _("failed to rebuild database: original database "
"remains in place\n"));
diff -r d8e2ec20c948 rpmdb/rpmdb.h
--- a/rpmdb/rpmdb.h Wed Jul 18 16:05:56 2007 +0300
+++ b/rpmdb/rpmdb.h Thu Jul 19 11:52:29 2007 +0300
@@ -1038,6 +1038,12 @@ Header rpmdbNextIterator(/*@null@*/ rpmd
/*@globals rpmGlobalMacroContext, h_errno, fileSystem, internalState @*/
/*@modifies mi, rpmGlobalMacroContext, fileSystem, internalState @*/;
+
+/** \ingroup rpmdb
+ * Clean up any open db iterators and transactions
+ */
+void rpmdbTerminate(void);
+
/** \ingroup rpmdb
* Check rpmdb signal handler for trapped signal exit.
*/
@@ -1096,6 +1102,25 @@ int rpmdbRebuild(/*@null@*/ const char *
/*@null@*/ rpmRC (*hdrchk) (rpmts ts, const void *uh, size_t uc, const char ** msg))
/*@globals rpmGlobalMacroContext, h_errno, fileSystem, internalState @*/
/*@modifies rpmGlobalMacroContext, fileSystem, internalState @*/;
+
+#ifdef _RPMDB_INTERNAL
+
+/* XXX these don't really have anything to do with rpmdb itself... */
+
+/** \ingroup rpmdb
+ * Block all signals, return previous mask through oldMask
+ * @retval oldMask saved signal mask
+ * @return return code of sigprocmask()
+ */
+int blockSignals(sigset_t * oldMask);
+
+/** \ingroup rpmdb
+ * Unblock all signals, return previous mask through oldMask
+ * @param oldMask signal set to restore
+ * @return return code of sigprocmask()
+ */
+int unblockSignals(sigset_t * oldMask);
+#endif
#ifndef __APPLE__
/**
diff -r d8e2ec20c948 rpmio/rpmsq.c
--- a/rpmio/rpmsq.c Wed Jul 18 16:05:56 2007 +0300
+++ b/rpmio/rpmsq.c Thu Jul 19 12:34:28 2007 +0300
@@ -270,6 +270,7 @@ static struct rpmsig_s {
static struct rpmsig_s {
int signum;
void (*handler) (int signum, void * info, void * context);
+ void (*saved_handler);
int active;
struct sigaction oact;
} rpmsigTbl[] = {
@@ -367,6 +368,12 @@ int rpmsqEnable(int signum, /*@null@*/ r
(void) sigaction(tbl->signum, NULL, &tbl->oact);
if (tbl->oact.sa_handler == SIG_IGN)
continue;
+
+ /*
+ * Various callers expect NULL to mean rpmsqAction, so
+ * unless a handler was specified use it as "saved" handler
+ */
+ tbl->saved_handler = (void*)(handler != NULL ? handler : rpmsqAction);
(void) sigemptyset (&sa.sa_mask);
sa.sa_flags = SA_SIGINFO;
@@ -388,7 +395,7 @@ int rpmsqEnable(int signum, /*@null@*/ r
if (sigaction(tbl->signum, &tbl->oact, NULL) < 0)
break;
tbl->active = 0; /* XXX just in case */
- tbl->handler = (handler != NULL ? handler : rpmsqAction);
+ tbl->handler = (handler != NULL ? handler : tbl->saved_handler);
}
}
ret = tbl->active;
More information about the Rpm-maint
mailing list