In order to fix a possible race condition in cTransfer I have centralized the 'active' handling of cThread. Since this is a rather sensitive area of the program, I'd like to invite you all to take a look at the attached patch against VDR version 1.3.28 and let me know if you're running into any trouble with it.
Here's the HISTORY entry I made for this change:
Fixed a race condition in cTransfer (thanks to Klaus ??? for reporting this one). In doing so, the 'active' variables used by the actual derived cThread classes have been replaced by the cThread::Active() function. The previous functionality of cThread::Active() has been moved into the new cThread::Running(). Plugin authors should check their derived cThread classes and replace any 'active' variables the same way as, for instance, done in transfer.c.
Klaus
=================================================================== RCS file: ./RCS/cutter.c retrieving revision 1.8 diff -u -r1.8 ./cutter.c --- ./cutter.c 2005/05/15 14:21:08 1.8 +++ ./cutter.c 2005/08/13 11:49:02 @@ -18,7 +18,6 @@ class cCuttingThread : public cThread { private: const char *error; - bool active; int fromFile, toFile; cFileName *fromFileName, *toFileName; cIndexFile *fromIndex, *toIndex; @@ -35,7 +34,6 @@ :cThread("video cutting") { error = NULL; - active = false; fromFile = toFile = -1; fromFileName = toFileName = NULL; fromIndex = toIndex = NULL; @@ -53,7 +51,6 @@
cCuttingThread::~cCuttingThread() { - active = false; Cancel(3); delete fromFileName; delete toFileName; @@ -67,7 +64,8 @@ if (Mark) { fromFile = fromFileName->Open(); toFile = toFileName->Open(); - active = fromFile >= 0 && toFile >= 0; + if (fromFile < 0 || toFile < 0) + return; int Index = Mark->position; Mark = fromMarks.Next(Mark); int FileSize = 0; @@ -78,7 +76,7 @@ uchar buffer[MAXFRAMESIZE]; bool LastMark = false; bool cutIn = true; - while (active) { + while (Active()) { uchar FileNumber; int FileOffset, Length; uchar PictureType; @@ -215,7 +213,7 @@
void cCutter::Stop(void) { - bool Interrupted = cuttingThread && cuttingThread->Active(); + bool Interrupted = cuttingThread && cuttingThread->Running(); const char *Error = cuttingThread ? cuttingThread->Error() : NULL; delete cuttingThread; cuttingThread = NULL; @@ -232,7 +230,7 @@ bool cCutter::Active(void) { if (cuttingThread) { - if (cuttingThread->Active()) + if (cuttingThread->Running()) return true; error = cuttingThread->Error(); Stop(); =================================================================== RCS file: ./RCS/cutter.h retrieving revision 1.1 diff -u -r1.1 ./cutter.h =================================================================== RCS file: ./RCS/device.c retrieving revision 1.103 diff -u -r1.103 ./device.c --- ./device.c 2005/06/12 13:39:11 1.103 +++ ./device.c 2005/08/13 11:44:06 @@ -156,8 +156,6 @@
SetVideoFormat(Setup.VideoFormat);
- active = false; - mute = false; volume = Setup.CurrentVolume;
@@ -1126,25 +1124,25 @@
void cDevice::Action(void) { - if (active && OpenDvr()) { - for (; active;) { - // Read data from the DVR device: - uchar *b = NULL; - if (GetTSPacket(b)) { - if (b) { - int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; - // Distribute the packet to all attached receivers: - Lock(); - for (int i = 0; i < MAXRECEIVERS; i++) { - if (receiver[i] && receiver[i]->WantsPid(Pid)) - receiver[i]->Receive(b, TS_SIZE); - } - Unlock(); - } - } - else - break; - } + if (Active() && OpenDvr()) { + while (Active()) { + // Read data from the DVR device: + uchar *b = NULL; + if (GetTSPacket(b)) { + if (b) { + int Pid = (((uint16_t)b[1] & PID_MASK_HI) << 8) | b[2]; + // Distribute the packet to all attached receivers: + Lock(); + for (int i = 0; i < MAXRECEIVERS; i++) { + if (receiver[i] && receiver[i]->WantsPid(Pid)) + receiver[i]->Receive(b, TS_SIZE); + } + Unlock(); + } + } + else + break; + } CloseDvr(); } } @@ -1188,10 +1186,8 @@ Receiver->device = this; receiver[i] = Receiver; Unlock(); - if (!active) { - active = true; + if (!Active()) Start(); - } return true; } } @@ -1218,10 +1214,8 @@ else if (receiver[i]) receiversLeft = true; } - if (!receiversLeft) { - active = false; + if (!receiversLeft) Cancel(3); - } }
void cDevice::DetachAll(int Pid) @@ -1246,13 +1240,11 @@ delivered = false; ringBuffer = new cRingBufferLinear(Size, TS_SIZE, true, "TS"); ringBuffer->SetTimeouts(100, 100); - active = true; Start(); }
cTSBuffer::~cTSBuffer() { - active = false; Cancel(3); delete ringBuffer; } @@ -1262,20 +1254,20 @@ if (ringBuffer) { bool firstRead = true; cPoller Poller(f); - for (; active;) { - if (firstRead || Poller.Poll(100)) { - firstRead = false; - int r = ringBuffer->Read(f); - if (r < 0 && FATALERRNO) { - if (errno == EOVERFLOW) - esyslog("ERROR: driver buffer overflow on device %d", cardIndex); - else { - LOG_ERROR; - break; - } - } - } - } + while (Active()) { + if (firstRead || Poller.Poll(100)) { + firstRead = false; + int r = ringBuffer->Read(f); + if (r < 0 && FATALERRNO) { + if (errno == EOVERFLOW) + esyslog("ERROR: driver buffer overflow on device %d", cardIndex); + else { + LOG_ERROR; + break; + } + } + } + } } }
=================================================================== RCS file: ./RCS/device.h retrieving revision 1.60 diff -u -r1.60 ./device.h --- ./device.h 2005/07/30 09:31:53 1.60 +++ ./device.h 2005/08/13 11:44:13 @@ -233,7 +233,6 @@ // PID handle facilities
private: - bool active; virtual void Action(void); protected: enum ePidType { ptAudio, ptVideo, ptPcr, ptTeletext, ptDolby, ptOther }; @@ -518,7 +517,6 @@ private: int f; int cardIndex; - bool active; bool delivered; cRingBufferLinear *ringBuffer; virtual void Action(void); =================================================================== RCS file: ./RCS/dvbdevice.c retrieving revision 1.131 diff -u -r1.131 ./dvbdevice.c --- ./dvbdevice.c 2005/06/19 11:00:43 1.131 +++ ./dvbdevice.c 2005/08/13 11:40:46 @@ -76,7 +76,6 @@ cCiHandler *ciHandler; cChannel channel; const char *diseqcCommands; - bool active; bool useCa; time_t startTime; eTunerStatus tunerStatus; @@ -101,7 +100,6 @@ frontendType = FrontendType; ciHandler = CiHandler; diseqcCommands = NULL; - active = false; useCa = false; tunerStatus = tsIdle; startTime = time(NULL); @@ -113,7 +111,6 @@
cDvbTuner::~cDvbTuner() { - active = false; tunerStatus = tsIdle; newSet.Signal(); Cancel(3); @@ -294,8 +291,7 @@ void cDvbTuner::Action(void) { dvb_frontend_event event; - active = true; - while (active) { + while (Active()) { Lock(); if (tunerStatus == tsSet) { while (GetFrontendEvent(event)) =================================================================== RCS file: ./RCS/dvbplayer.c retrieving revision 1.36 diff -u -r1.36 ./dvbplayer.c --- ./dvbplayer.c 2005/07/30 10:00:24 1.36 +++ ./dvbplayer.c 2005/08/13 12:27:17 @@ -79,7 +79,6 @@ int wanted; int length; bool hasData; - bool active; cCondWait newSet; protected: void Action(void); @@ -98,13 +97,11 @@ buffer = NULL; wanted = length = 0; hasData = false; - active = false; Start(); }
cNonBlockingFileReader::~cNonBlockingFileReader() { - active = false; newSet.Signal(); Cancel(3); free(buffer); @@ -147,8 +144,7 @@
void cNonBlockingFileReader::Action(void) { - active = true; - while (active) { + while (Active()) { Lock(); if (!hasData && f >= 0 && buffer) { int r = safe_read(f, buffer + length, wanted - length); @@ -187,8 +183,6 @@ cIndexFile *index; int replayFile; bool eof; - bool active; - bool running; bool firstPacket; ePlayModes playMode; ePlayDirs playDir; @@ -207,7 +201,7 @@ public: cDvbPlayer(const char *FileName); virtual ~cDvbPlayer(); - bool Active(void) { return active; } + bool Active(void) { return cThread::Active(); } void Pause(void); void Play(void); void Forward(void); @@ -233,8 +227,6 @@ backTrace = NULL; index = NULL; eof = false; - active = true; - running = false; firstPacket = true; playMode = pmPlay; playDir = pdForward; @@ -353,11 +345,8 @@ if (replayFile >= 0) Start(); } - else if (active) { - running = false; + else Cancel(9); - active = false; - } }
void cDvbPlayer::Action(void) @@ -374,8 +363,7 @@ int Length = 0; bool Sleep = false;
- running = true; - while (running && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { + while (Active() && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { if (Sleep) { cCondWait::SleepMs(3); // this keeps the CPU load low Sleep = false; @@ -501,7 +489,6 @@ Sleep = true; } } - active = running = false;
cNonBlockingFileReader *nbfr = nonBlockingFileReader; nonBlockingFileReader = NULL; =================================================================== RCS file: ./RCS/dvbplayer.h retrieving revision 1.2 diff -u -r1.2 ./dvbplayer.h =================================================================== RCS file: ./RCS/recorder.c retrieving revision 1.13 diff -u -r1.13 ./recorder.c --- ./recorder.c 2005/01/16 12:53:17 1.13 +++ ./recorder.c 2005/08/13 11:33:35 @@ -29,7 +29,6 @@ uchar pictureType; int fileSize; int recordFile; - bool active; time_t lastDiskSpaceCheck; bool RunningLowOnDiskSpace(void); bool NextFile(void); @@ -43,7 +42,6 @@ cFileWriter::cFileWriter(const char *FileName, cRemux *Remux) :cThread("file writer") { - active = false; fileName = NULL; remux = Remux; index = NULL; @@ -63,7 +61,6 @@
cFileWriter::~cFileWriter() { - active = false; Cancel(3); delete index; delete fileName; @@ -96,13 +93,11 @@ void cFileWriter::Action(void) { time_t t = time(NULL); - active = true; - while (active) { + while (Active()) { int Count; uchar *p = remux->Get(Count, &pictureType); if (p) { - //XXX+ active??? see old version (Busy) - if (!active && pictureType == I_FRAME) // finish the recording before the next 'I' frame + if (!Active() && pictureType == I_FRAME) // finish the recording before the next 'I' frame break; if (NextFile()) { if (index && pictureType != NO_PICTURE) @@ -124,15 +119,12 @@ t = time(NULL); } } - active = false; }
cRecorder::cRecorder(const char *FileName, int Ca, int Priority, int VPid, const int *APids, const int *DPids, const int *SPids) :cReceiver(Ca, Priority, VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids) ,cThread("recording") { - active = false; - // Make sure the disk is up and running:
SpinUpDisk(FileName); @@ -157,25 +149,22 @@ writer->Start(); Start(); } - else if (active) { - active = false; + else Cancel(3); - } }
void cRecorder::Receive(uchar *Data, int Length) { - if (active) { + if (Active()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Active()) ringBuffer->ReportOverflow(Length - p); } }
void cRecorder::Action(void) { - active = true; - while (active) { + while (Active()) { int r; uchar *b = ringBuffer->Get(r); if (b) { =================================================================== RCS file: ./RCS/recorder.h retrieving revision 1.3 diff -u -r1.3 ./recorder.h --- ./recorder.h 2005/01/15 16:35:53 1.3 +++ ./recorder.h 2005/08/13 11:31:18 @@ -23,7 +23,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; cFileWriter *writer; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length); =================================================================== RCS file: ./RCS/remote.c retrieving revision 1.42 diff -u -r1.42 ./remote.c --- ./remote.c 2005/03/20 13:25:31 1.42 +++ ./remote.c 2005/08/13 11:28:35 @@ -213,7 +213,6 @@ :cRemote("KBD") ,cThread("KBD remote control") { - active = false; tcgetattr(STDIN_FILENO, &savedTm); struct termios tm; if (tcgetattr(STDIN_FILENO, &tm) == 0) { @@ -230,7 +229,6 @@ cKbdRemote::~cKbdRemote() { kbdAvailable = false; - active = false; Cancel(3); tcsetattr(STDIN_FILENO, TCSANOW, &savedTm); } @@ -261,12 +259,11 @@ void cKbdRemote::Action(void) { cPoller Poller(STDIN_FILENO); - active = true; - while (active) { + while (Active()) { if (Poller.Poll(100)) { uint64 Command = 0; uint i = 0; - while (active && i < sizeof(Command)) { + while (Active() && i < sizeof(Command)) { uchar ch; int r = read(STDIN_FILENO, &ch, 1); if (r == 1) { =================================================================== RCS file: ./RCS/remote.h retrieving revision 1.29 diff -u -r1.29 ./remote.h --- ./remote.h 2004/05/28 14:14:02 1.29 +++ ./remote.h 2005/08/13 11:28:10 @@ -81,7 +81,6 @@
class cKbdRemote : public cRemote, private cThread { private: - bool active; static bool kbdAvailable; static bool rawMode; struct termios savedTm; =================================================================== RCS file: ./RCS/sections.c retrieving revision 1.11 diff -u -r1.11 ./sections.c --- ./sections.c 2005/05/29 11:43:17 1.11 +++ ./sections.c 2005/08/13 11:25:04 @@ -44,7 +44,6 @@ { shp = new cSectionHandlerPrivate; device = Device; - active = false; statusCount = 0; on = false; waitForLock = false; @@ -54,7 +53,6 @@
cSectionHandler::~cSectionHandler() { - active = false; Cancel(3); cFilter *fi; while ((fi = filters.First()) != NULL) @@ -166,9 +164,8 @@
void cSectionHandler::Action(void) { - active = true; SetPriority(19); - while (active) { + while (Active()) {
Lock(); if (waitForLock) =================================================================== RCS file: ./RCS/sections.h retrieving revision 1.4 diff -u -r1.4 ./sections.h --- ./sections.h 2004/08/08 13:44:17 1.4 +++ ./sections.h 2005/08/13 11:23:55 @@ -25,7 +25,6 @@ private: cSectionHandlerPrivate *shp; cDevice *device; - bool active; int statusCount; bool on, waitForLock; time_t lastIncompleteSection; =================================================================== RCS file: ./RCS/thread.c retrieving revision 1.43 diff -u -r1.43 ./thread.c --- ./thread.c 2005/05/29 11:40:30 1.43 +++ ./thread.c 2005/08/13 11:22:37 @@ -197,7 +197,7 @@
cThread::cThread(const char *Description) { - running = false; + running = active = false; childTid = 0; description = NULL; SetDescription(Description); @@ -205,6 +205,7 @@
cThread::~cThread() { + Cancel(); // just in case the derived class didn't call it free(description); }
@@ -233,6 +234,7 @@ Thread->Action(); if (Thread->description) dsyslog("%s thread ended (pid=%d, tid=%ld)", Thread->description, getpid(), pthread_self()); + Thread->active = false; Thread->running = false; return NULL; } @@ -240,21 +242,21 @@ bool cThread::Start(void) { if (!running) { - running = true; + running = active = true; if (pthread_create(&childTid, NULL, (void *(*) (void *))&StartThread, (void *)this) == 0) { pthread_detach(childTid); // auto-reap pthread_setschedparam(childTid, SCHED_RR, 0); } else { LOG_ERROR; - running = false; + running = active = false; return false; } } return true; }
-bool cThread::Active(void) +bool cThread::Running(void) { if (running) { // @@ -271,7 +273,7 @@ if (err != ESRCH) LOG_ERROR; childTid = 0; - running = false; + running = active = false; } else return true; @@ -281,10 +283,11 @@
void cThread::Cancel(int WaitSeconds) { + active = false; if (running) { if (WaitSeconds > 0) { for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0; ) { - if (!Active()) + if (!Running()) return; cCondWait::SleepMs(10); } =================================================================== RCS file: ./RCS/thread.h retrieving revision 1.28 diff -u -r1.28 ./thread.h --- ./thread.h 2005/05/29 11:31:24 1.28 +++ ./thread.h 2005/08/13 13:01:33 @@ -76,6 +76,7 @@ friend class cThreadLock; private: bool running; + bool active; pthread_t childTid; cMutex mutex; char *description; @@ -86,13 +87,30 @@ void Lock(void) { mutex.Lock(); } void Unlock(void) { mutex.Unlock(); } virtual void Action(void) = 0; + ///< A derived cThread class must implement the code it wants to + ///< execute as a separate thread in this function. If this is + ///< a loop, it must check Active() repeatedly to see whether + ///< it's time to stop. + bool Active(void) { return active; } + ///< Returns false if a derived cThread object shall leave its Action() + ///< function. void Cancel(int WaitSeconds = 0); + ///< Cancels the thread by first setting 'active' to false, so that + ///< the Action() loop can finish in an orderly fashion and then waiting + ///< up to WaitSeconds seconds for the thread to actually end. If the + ///< thread doesn't end by itself, it is killed. public: cThread(const char *Description = NULL); + ///< Creates a new thread. + ///< If Description is present, a log file entry will be made when + ///< the thread starts and stops. The Start() function must be called + ///< to actually start the thread. virtual ~cThread(); void SetDescription(const char *Description, ...); bool Start(void); - bool Active(void); + ///< Actually starts the thread. + bool Running(void); + ///< Checks whether the thread is actually running. static bool EmergencyExit(bool Request = false); };
=================================================================== RCS file: ./RCS/transfer.c retrieving revision 1.28 diff -u -r1.28 ./transfer.c --- ./transfer.c 2005/02/19 14:38:55 1.28 +++ ./transfer.c 2005/08/13 11:19:46 @@ -21,7 +21,6 @@ ringBuffer = new cRingBufferLinear(TRANSFERBUFSIZE, TS_SIZE * 2, true, "Transfer"); remux = new cRemux(VPid, APids, Setup.UseDolbyDigital ? DPids : NULL, SPids); needsBufferReserve = Setup.UseDolbyDigital && VPid != 0 && DPids && DPids[0] != 0; - active = false; }
cTransfer::~cTransfer() @@ -34,21 +33,17 @@
void cTransfer::Activate(bool On) { - if (On) { - if (!active) - Start(); - } - else if (active) { - active = false; + if (On) + Start(); + else Cancel(3); - } }
void cTransfer::Receive(uchar *Data, int Length) { - if (IsAttached() && active) { + if (IsAttached() && Active()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && active) + if (p != Length && Active()) ringBuffer->ReportOverflow(Length - p); return; } @@ -70,8 +65,7 @@ bool GotBufferReserve = false; int RequiredBufferReserve = KILOBYTE(DvbCardWith4MBofSDRAM ? 288 : 576); #endif - active = true; - while (active) { + while (Active()) { #ifdef FW_NEEDS_BUFFER_RESERVE_FOR_AC3 if (needsBufferReserve && !GotBufferReserve) { //XXX For dolby we've to fill the buffer because the firmware does @@ -145,7 +139,6 @@ } } } - active = false; }
// --- cTransferControl ------------------------------------------------------ =================================================================== RCS file: ./RCS/transfer.h retrieving revision 1.9 diff -u -r1.9 ./transfer.h --- ./transfer.h 2005/01/15 16:39:39 1.9 +++ ./transfer.h 2005/08/13 10:16:02 @@ -21,7 +21,6 @@ cRingBufferLinear *ringBuffer; cRemux *remux; bool needsBufferReserve; - bool active; protected: virtual void Activate(bool On); virtual void Receive(uchar *Data, int Length);