Ticket #603: mythmedia_locking.diff

libs/libmythui/mythmainwindow.cpp

@@ -1126 +1126 @@
                 iscatched = activedialog->onMediaEvent(pDev);
             if (!iscatched)
             {
-                while (itr != d->mediaHandlerMap.end())
+                MediaMonitor *mon = MediaMonitor::GetMediaMonitor();
+                if (mon && mon->ValidateAndLock(pDev))
                 {
-                   if ((itr.data().MediaType & (int)pDev->getMediaType()))
-                   {
-                       VERBOSE(VB_IMPORTANT, "Found a handler");
-                       d->exitingtomain = true;
-                       d->exitmenumediadevicecallback = itr.data().callback;
-                       d->mediadeviceforcallback = pDev;
-                       QApplication::postEvent(this, new ExitToMainMenuEvent());
-                       break;
-                   }
-                   itr++;
+                    while (itr != d->mediaHandlerMap.end())
+                    {
+                       if ((itr.data().MediaType & (int)pDev->getMediaType()))
+                       {
+                           VERBOSE(VB_IMPORTANT, "Found a handler");
+                           d->exitingtomain = true;
+                           d->exitmenumediadevicecallback = itr.data().callback;
+                           d->mediadeviceforcallback = pDev;
+                           QApplication::postEvent(this, new ExitToMainMenuEvent());
+                           mon->Unlock(pDev);
+                           break;
+                       }
+                       itr++;
+                    }
+                    mon->Unlock(pDev);
                 }
             }
         }

libs/libmyth/mythmediamonitor.h

@@ -43 +43 @@
     unsigned long m_Interval;
 };
 
-class MediaMonitor;
-
 class MediaMonitor : public QObject
 {
     Q_OBJECT
@@ -64 +62 @@
 
     static MediaMonitor *GetMediaMonitor(void);
 
-    // this is not safe.. device could get deleted...
-    QValueList <MythMediaDevice*> GetMedias(MediaType mediatype);
+    bool ValidateAndLock(MythMediaDevice *pMedia);
+    void Unlock(MythMediaDevice *pMedia);
 
+    // To safely dereference the pointers returned by this function
+    // first validate the pointer with ValidateAndLock(), if true is returned
+    // it is safe to dereference the pointer. When finished call Unlock()
+    QValueList<MythMediaDevice*> GetMedias(MediaType mediatype);
+
   public slots:
     void mediaStatusChanged(MediaStatus oldStatus, MythMediaDevice* pMedia);
 
@@ -87 +90 @@
     static QStringList GetCDROMBlockDevices(void);
 
   protected:
+    QMutex                       m_DevicesLock;
     QValueList<MythMediaDevice*> m_Devices;
+    QValueList<MythMediaDevice*> m_RemovedDevices;
+    QMap<MythMediaDevice*, int>  m_UseCount;
+
     bool                         m_Active;
     MonitorThread                m_Thread;
     bool                         m_AllowEject;

libs/libmyth/mythmediamonitor.cpp

@@ -107 +107 @@
 {
     MythMediaDevice *selected = NULL;
 
+    QMutexLocker locker(&m_DevicesLock);
+
     if (m_Devices.count() == 1)
     {
         if (m_Devices.first()->getAllowEject())
@@ -194 +196 @@
 
 MediaMonitor::MediaMonitor(QObject* par, unsigned long interval, 
                            bool allowEject) 
-    : QObject(par), m_Active(false), m_Thread(NULL, interval),
+    : QObject(par), m_Active(false), m_Thread(this, interval),
       m_AllowEject(allowEject), m_fifo(-1)
 {
 }
@@ -348 +350 @@
 // Given a media deivce add it to our collection.
 void MediaMonitor::AddDevice(MythMediaDevice* pDevice)
 {
+    QMutexLocker locker(&m_DevicesLock);
+
     connect(pDevice, SIGNAL(statusChanged(MediaStatus, MythMediaDevice*)), 
             this, SLOT(mediaStatusChanged(MediaStatus, MythMediaDevice*)));
     m_Devices.push_back( pDevice );
+    m_UseCount[pDevice] = 0;
 }
 
 // Given a fstab entry to a media device determine what type of device it is 
@@ -575 +580 @@
  */
 bool MediaMonitor::RemoveDevice(const QString &dev)
 {
+    QMutexLocker locker(&m_DevicesLock);
+
     QValueList<MythMediaDevice*>::iterator it;
     for (it = m_Devices.begin(); it != m_Devices.end(); it++)
     {
         if ((*it)->getDevicePath() == dev)
         {
-            m_Devices.remove(it);
+            if (m_UseCount[*it] == 0)
+            {
+                delete *it;
+                m_Devices.remove(it);
+                m_UseCount.remove(*it);
+            }
+            else
+            {
+                // Other threads are still using this device
+                // postpone actual delete until they finish.
+                disconnect(*it);
+                m_RemovedDevices.append(*it);
+                m_Devices.remove(it);
+            }
+
             return true;
         }
     }
+
     return false;
 }
 
@@ -681 +703 @@
     m_Thread.wait();
 }
 
-// Ask for available media
-// this is not safe.. device could get deleted...
-QValueList <MythMediaDevice*> MediaMonitor::GetMedias(MediaType mediatype)
+/** \fn MediaMonitor::ValidateAndLock(MythMediaDevice *pMedia)
+ *  \brief Validates the MythMediaDevice and increments its reference count
+ *
+ *  Returns true if pMedia device is valid, otherwise returns false.
+ *  If this function returns false the caller should gracefully recover.
+ *
+ *  NOTE: This function can block.
+ *
+ *  \sa Unlock(MythMediaDevice *pMedia), GetMedias(MediaType mediatype)
+ */
+bool MediaMonitor::ValidateAndLock(MythMediaDevice *pMedia)
 {
-    QValueList <MythMediaDevice*> medias;
+    QMutexLocker locker(&m_DevicesLock);
 
-    QValueList <MythMediaDevice*>::iterator it = m_Devices.begin();
+    if (!m_Devices.contains(pMedia))
+        return false;
+
+    m_UseCount[pMedia]++;
+
+    return true;
+}
+
+/** \fn MediaMonitor::Unlock(MythMediaDevice *pMedia)
+ *  \brief decrements the MythMediaDevices reference count
+ *
+ *  \sa ValidateAndLock(MythMediaDevice *pMedia), GetMedias(MediaType mediatype)
+ */
+void MediaMonitor::Unlock(MythMediaDevice *pMedia)
+{
+    QMutexLocker locker(&m_DevicesLock);
+
+    if (!m_UseCount.contains(pMedia))
+        return;
+
+    m_UseCount[pMedia]--;
+
+    if (m_UseCount[pMedia] == 0 && m_RemovedDevices.contains(pMedia))
+    {
+        delete pMedia;
+        m_RemovedDevices.remove(pMedia);
+        m_UseCount.remove(pMedia);
+    }
+}
+
+/** \fn MediaMonitor::GetMedias(MediaType mediatype)
+ *  \brief Ask for available media
+ *
+ *  GetMedias returns a list of MythMediaDevice pointers which match the given mediatype.
+ *
+ *  It is potentially unsafe to use the pointers returned by this function. The devices
+ *  may be removed and their associated MythMediaDevice objects destroyed. It is the
+ *  responsibility of the caller to ensure that the pointers are validated and
+ *  and the reference count is incremented by calling MediaMonitor::ValidateAndLock()
+ *  before the the returned pointer is dereferenced and MediaMonitor::Unlock() when
+ *  done.
+ *
+ * \sa ValidateAndLock(MythMediaDevice *pMedia), Unlock(MythMediaDevice *pMedia)
+ */
+QValueList<MythMediaDevice*> MediaMonitor::GetMedias(MediaType mediatype)
+{
+    QMutexLocker locker(&m_DevicesLock);
+
+    QValueList<MythMediaDevice*> medias;
+
+    QValueList<MythMediaDevice*>::iterator it = m_Devices.begin();
     for (;it != m_Devices.end(); it++)
     {
         if (((*it)->getMediaType() == mediatype) &&
@@ -701 +781 @@
 
     return medias;
 }
+
 // Signal handler.
 void MediaMonitor::mediaStatusChanged(MediaStatus oldStatus, 
                                       MythMediaDevice* pMedia)