summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRainer Gerhards <rgerhards@adiscon.com>2007-12-20 08:54:51 +0000
committerRainer Gerhards <rgerhards@adiscon.com>2007-12-20 08:54:51 +0000
commit2eb19201b3f4be01057c1b958a077b5fb2fce107 (patch)
tree067b64859e42ff5ad903f20ec9c22bd3600eabe6
parent02a46ee6b0b7f326dc0affd2caae961e09bf9a3b (diff)
downloadrsyslog-2eb19201b3f4be01057c1b958a077b5fb2fce107.tar.gz
rsyslog-2eb19201b3f4be01057c1b958a077b5fb2fce107.tar.xz
rsyslog-2eb19201b3f4be01057c1b958a077b5fb2fce107.zip
- working on a potential race condition on the new input module interface.
See newsgroup posting for details on the issue: http://groups.google.com/group/comp.programming.threads/msg/330b9675f17 a1ad6 I tried some mutex operations but came to the conclusion that this does not really help. So I have now switched to plain thread cancellation, which so far seems to be OK. Need more practical experience with other input modules to make a final decision. Thus I leave all code in and have just disabled the problematic code.
-rw-r--r--plugins/imklog/imklog.c8
-rw-r--r--plugins/immark/immark.c1
-rw-r--r--threads.c33
-rw-r--r--threads.h10
4 files changed, 43 insertions, 9 deletions
diff --git a/plugins/imklog/imklog.c b/plugins/imklog/imklog.c
index 157dee73..1c3c79c3 100644
--- a/plugins/imklog/imklog.c
+++ b/plugins/imklog/imklog.c
@@ -46,11 +46,14 @@ MODULE_TYPE_INPUT
TERM_SYNC_TYPE(eTermSync_SIGNAL)
/* Module static data */
-int dbgPrintSymbols = 0;
DEF_IMOD_STATIC_DATA
typedef struct _instanceData {
} instanceData;
+/* configuration settings TODO: move to instance data? */
+int dbgPrintSymbols = 0;
+int symbols_twice = 0;
+
/* Includes. */
#include <unistd.h>
@@ -87,12 +90,11 @@ static int kmsg,
static int use_syscall = 0,
symbol_lookup = 1;
-static char *symfile = (char *) 0,
+static char *symfile = NULL,
log_buffer[LOG_BUFFER_SIZE];
static enum LOGSRC {none, proc, kernel} logsrc;
-int symbols_twice = 0;
/* Function prototypes. */
diff --git a/plugins/immark/immark.c b/plugins/immark/immark.c
index b7b816b6..973821ad 100644
--- a/plugins/immark/immark.c
+++ b/plugins/immark/immark.c
@@ -68,6 +68,7 @@ typedef struct _instanceData {
*/
BEGINrunInput
CODESTARTrunInput
+ thrdBlockTermination(pThrd);
/* this is an endless loop - it is terminated when the thread is
* signalled to do so. This, however, is handled by the framework,
* right into the sleep below.
diff --git a/threads.c b/threads.c
index 88b4ce8f..04eea8f1 100644
--- a/threads.c
+++ b/threads.c
@@ -1,3 +1,7 @@
+/* TODO: we should guard the individual thread actions with a mutex. Else, we may
+ * run into race conditions on thread termination.
+ */
+
/* threads.c
*
* This file implements threading support helpers (and maybe the thread object)
@@ -25,7 +29,6 @@
* A copy of the GPL can be found in the file "COPYING" in this distribution.
*/
#include "config.h"
-#include "rsyslog.h"
#include <stdlib.h>
#include <string.h>
@@ -33,6 +36,7 @@
#include <pthread.h>
#include <assert.h>
+#include "rsyslog.h"
#include "syslogd.h"
#include "linkedlist.h"
#include "threads.h"
@@ -48,18 +52,22 @@ static linkedList_t llThrds;
/* Construct a new thread object
*/
-static rsRetVal thrdConstruct(thrdInfo_t **pThis)
+static rsRetVal thrdConstruct(thrdInfo_t **ppThis)
{
- thrdInfo_t *pNew;
+ thrdInfo_t *pThis;
+
+ assert(ppThis != NULL);
- if((pNew = calloc(1, sizeof(thrdInfo_t))) == NULL)
+ if((pThis = calloc(1, sizeof(thrdInfo_t))) == NULL)
return RS_RET_OUT_OF_MEMORY;
/* OK, we got the element, now initialize members that should
* not be zero-filled.
*/
+ pThis->mutTermOK = (pthread_mutex_t *) malloc (sizeof (pthread_mutex_t));
+ pthread_mutex_init (pThis->mutTermOK, NULL);
- *pThis = pNew;
+ *ppThis = pThis;
return RS_RET_OK;
}
@@ -76,6 +84,7 @@ dbgprintf("thrdDestruct, pThis: %lx\n", pThis);
if(pThis->bIsActive == 1) {
thrdTerminate(pThis);
}
+ free(pThis->mutTermOK);
free(pThis);
return RS_RET_OK;
@@ -91,9 +100,15 @@ rsRetVal thrdTerminate(thrdInfo_t *pThis)
dbgprintf("Terminate thread %lx via method %d\n", pThis->thrdID, pThis->eTermTool);
if(pThis->eTermTool == eTermSync_SIGNAL) {
+ /* we first wait for the thread to reach a point in execution where it
+ * is safe to terminate it.
+ * TODO: TIMEOUT!
+ */
+ pthread_mutex_lock(pThis->mutTermOK);
pThis->bShallStop = 1; /* request termination */
pthread_kill(pThis->thrdID, SIGUSR2); /* get thread out ouf blocking calls */
pthread_join(pThis->thrdID, NULL);
+ pthread_mutex_unlock(pThis->mutTermOK); /* cleanup... */
/* TODO: TIMEOUT! */
} else if(pThis->eTermTool == eTermSync_NONE) {
pthread_cancel(pThis->thrdID);
@@ -162,7 +177,7 @@ rsRetVal thrdCreate(rsRetVal (*thrdMain)(thrdInfo_t*), eTermSyncType_t eTermSync
assert(thrdMain != NULL);
CHKiRet(thrdConstruct(&pThis));
- pThis->eTermTool = eTermSyncType;
+ pThis->eTermTool = eTermSync_NONE; // eTermSyncType; TODO: review
pThis->bIsActive = 1;
pThis->pUsrThrdMain = thrdMain;
pThis->pAfterRun = afterRun;
@@ -233,9 +248,15 @@ thrdSleep(thrdInfo_t *pThis, int iSeconds, int iuSeconds)
assert(pThis != NULL);
tvSelectTimeout.tv_sec = iSeconds;
tvSelectTimeout.tv_usec = iuSeconds; /* micro seconds */
+ thrdUnblockTermination(pThis);
+ /* there may be a race condition if pthread_kill() is called after unblock but
+ * before the select() is setup. TODO: check and re-eval -- rgerhards, 2007-12-20
+ */
select(0, NULL, NULL, NULL, &tvSelectTimeout);
if(pThis->bShallStop)
iRet = RS_RET_TERMINATE_NOW;
+ else
+ thrdBlockTermination(pThis);
return iRet;
}
diff --git a/threads.h b/threads.h
index 51ae52cf..6bf1e3b1 100644
--- a/threads.h
+++ b/threads.h
@@ -33,6 +33,7 @@ typedef enum eTermSyncType {
/* the thread object */
typedef struct thrdInfo {
eTermSyncType_t eTermTool;
+ pthread_mutex_t *mutTermOK; /* Is it ok to terminate that thread now? */
int bIsActive; /* Is thread running? */
int bShallStop; /* set to 1 if the thread should be stopped ? */
rsRetVal (*pUsrThrdMain)(struct thrdInfo*); /* user thread main to be called in new thread */
@@ -67,4 +68,13 @@ void queueDel (msgQueue *q, void **out);
extern int iMainMsgQueueSize;
extern msgQueue *pMsgQueue;
+
+/* macros (replace inline functions) */
+/*TODO: remove these macros once we now we can live without -- rgerhards, 2007-12-20
+ * #define thrdBlockTermination(pThis) {dbgprintf("lock mutex\n"); pthread_mutex_lock((pThis)->mutTermOK) ;}
+ * #define thrdUnblockTermination(pThis) {dbgprintf("unlock mutex\n"); pthread_mutex_unlock((pThis)->mutTermOK) ;}
+ */
+#define thrdBlockTermination(pThis)
+#define thrdUnblockTermination(pThis)
+
#endif /* #ifndef THREADS_H_INCLUDED */