zeroconf: clean up destruction/startup

* fixed race condition in destruction (hopefully ;)
* cleaned startup status testing in MainConnection
* faster shutdown/startup, less blocking in embedded lib getProperty

Change-Id: I3e9062fa59465523feb47ba195cf4ef465d2c16c
Reviewed-by: Daniel Molkentin <daniel.molkentin@nokia.com>

src/libs/zeroconf/embed/dnssd_clientstub.c

@@ -215,10 +215,29 @@ static int read_all(dnssd_sock_t sd, char *buf, int len)
         {
         // Don't use "MSG_WAITALL"; it returns "Invalid argument" on some Linux versions; use an explicit while () loop instead.
         //if (recv(sd, buf, len, MSG_WAITALL) != len) return -1;
+        int nErr = 0;
 
         while (len)
                 {
-                ssize_t num_read = recv(sd, buf, len, 0);
+                timeval timeout;
+                timeout.tv_sec = 0;
+                timeout.tv_usec = 100000;
+                fd_set readFds, writeFds, exceptFds;
+                memset(&readFds,0,sizeof(readFds));
+                memset(&writeFds,0,sizeof(writeFds));
+                memset(&exceptFds,0,sizeof(exceptFds));
+                FD_SET(sd, &readFds);
+                FD_SET(sd, &writeFds);
+                FD_SET(sd, &exceptFds);
+                ssize_t num_read = 0;
+                int nVal=select(sd+1, &readFds, &writeFds, &exceptFds, &timeout);
+                if (nVal < 1 || !FD_ISSET(sd, &readFds)) {
+                    ++nErr;
+                    if (nErr < 5) // wait max 0.5s without reading
+                        continue;
+                } else {
+                    num_read = recv(sd, buf, len, 0);
+                }
                 // It is valid to get an interrupted system call error e.g., somebody attaching
                 // in a debugger, retry without failing
                 if ((num_read < 0) && (errno == EINTR)) { syslog(LOG_INFO, "dnssd_clientstub read_all: EINTR continue"); continue; }

src/libs/zeroconf/servicebrowser.cpp

@@ -752,12 +752,16 @@ void ServiceGatherer::stopResolve(ZK_IP_Protocol protocol)
 {
     if ((protocol == ZK_PROTO_IPv4_OR_IPv6 || protocol == ZK_PROTO_IPv4)
             && (status & ResolveConnectionActive) != 0) {
+        QMutexLocker l(serviceBrowser->mainConnection->lock());
+        if (serviceBrowser->mainConnection->status() < MainConnection::Stopping)
             lib()->refDeallocate(resolveConnection);
         status &= ~ResolveConnectionActive;
         serviceBrowser->updateFlowStatusForCancel();
     }
     if ((protocol == ZK_PROTO_IPv4_OR_IPv6 || protocol == ZK_PROTO_IPv6)
             && (status & ResolveConnectionV6Active) != 0) {
+        QMutexLocker l(serviceBrowser->mainConnection->lock());
+        if (serviceBrowser->mainConnection->status() < MainConnection::Stopping)
             lib()->refDeallocate(resolveConnectionV6);
         status &= ~ResolveConnectionV6Active;
         serviceBrowser->updateFlowStatusForCancel();
@@ -837,6 +841,8 @@ void ServiceGatherer::restartResolve(ZK_IP_Protocol protocol)
 void ServiceGatherer::stopTxt()
 {
     if ((status & TxtConnectionActive) == 0) return;
+    QMutexLocker l(serviceBrowser->mainConnection->lock());
+    if (serviceBrowser->mainConnection->status() < MainConnection::Stopping)
         lib()->refDeallocate(txtConnection);
     status &= ~TxtConnectionActive;
     serviceBrowser->updateFlowStatusForCancel();
@@ -863,6 +869,8 @@ void ServiceGatherer::restartTxt()
 void ServiceGatherer::stopHostResolution()
 {
     if ((status & AddrConnectionActive) == 0) return;
+    QMutexLocker l(serviceBrowser->mainConnection->lock());
+    if (serviceBrowser->mainConnection->status() < MainConnection::Stopping)
         lib()->refDeallocate(addrConnection);
     status &= ~AddrConnectionActive;
     serviceBrowser->updateFlowStatusForCancel();
@@ -1552,14 +1560,21 @@ void ServiceBrowserPrivate::startedBrowsing()
 
 void MainConnection::stop(bool wait)
 {
-#if QT_VERSION >= 0x050000
+    {
-    if (m_status.load() < Stopping)
+        if (QThread::currentThread() == m_thread)
-#else
+            qCritical() << "ERROR ZerocConf::MainConnection::stop called from m_thread";
-    if (m_status < Stopping)
+        // This will most likely lock if called from the connection thread (as mainThreadLock is non recusive)
-#endif
+        // Making it recursive would open a hole during the startup of the thread.
+        // As of now this should always be called during the destruction of a browser,
+        // so from another thread.
         increaseStatusTo(Stopping);
-    if (m_mainRef)
+        QMutexLocker l(mainThreadLock());
+        QMutexLocker l2(lock());
+    }
+    if (m_mainRef) {
         lib->stopConnection(m_mainRef);
+        m_mainRef = 0;
+    }
     if (!m_thread)
         increaseStatusTo(Stopped);
     else if (wait && QThread::currentThread() != m_thread)
@@ -1569,38 +1584,30 @@ void MainConnection::stop(bool wait)
 MainConnection::MainConnection():
     flowStatus(NormalRFS),
     lib(zeroConfLibInstance()->defaultLib()), m_lock(QMutex::Recursive),
-    m_mainThreadLock(QMutex::Recursive), m_mainRef(0), m_failed(false), m_status(Starting), m_nErrs(0)
+    m_mainThreadLock(QMutex::NonRecursive), m_mainRef(0), m_failed(false), m_status(Starting), m_nErrs(0)
 {
     if (lib.isNull()){
         qDebug() << "could not load a valid library for ZeroConf::MainConnection, failing";
     } else {
         m_thread = new ConnectionThread(*this);
+        m_mainThreadLock.lock();
         m_thread->start(); // delay startup??
     }
 }
 
 MainConnection::~MainConnection()
 {
-    stop();
+    stop(true);
     delete m_thread;
-    // to do
 }
 
 bool MainConnection::increaseStatusTo(int s)
 {
-#if QT_VERSION >= 0x050000
+    int sAtt = status();
-    int sAtt = m_status.load();
-#else
-    int sAtt = m_status;
-#endif
     while (sAtt < s){
         if (m_status.testAndSetRelaxed(sAtt, s))
             return true;
-#if QT_VERSION >= 0x050000
+        sAtt = status();
-        sAtt = m_status.load();
-#else
-        sAtt = m_status;
-#endif
     }
     return false;
 }
@@ -1621,11 +1628,7 @@ void MainConnection::waitStartup()
     while (true){
         {
             QMutexLocker l(lock());
-#if QT_VERSION >= 0x050000
+            sAtt = status();
-            sAtt = m_status.load();
-#else
-            sAtt = m_status;
-#endif
             if (sAtt >= Running)
                 return;
         }
@@ -1639,11 +1642,7 @@ void MainConnection::addBrowser(ServiceBrowserPrivate *browser)
     QList<ErrorMessage> errs;
     {
         QMutexLocker l(lock());
-#if QT_VERSION >= 0x050000
+        actualStatus = status();
-        actualStatus = m_status.load();
-#else
-        actualStatus = m_status;
-#endif
         m_browsers.append(browser);
         errs = m_errors;
     }
@@ -1716,11 +1715,7 @@ void MainConnection::abortLib(){
 void MainConnection::createConnection()
 {
     gotoValidLib();
-#if QT_VERSION >= 0x050000
+    while (status() <= Running) {
-    while (m_status.load() <= Running) {
-#else
-    while (m_status <= Running) {
-#endif
         if (!lib) {
             increaseStatusTo(Stopped);
             break;
@@ -1812,6 +1807,7 @@ void MainConnection::destroyConnection()
 
 void  MainConnection::handleEvents()
 {
+    try {
         if (!m_status.testAndSetAcquire(Starting, Started)){
             appendError(ErrorMessage::WarningLevel, tr("MainConnection::handleEvents called with m_status != Starting, aborting"));
             increaseStatusTo(Stopped);
@@ -1819,12 +1815,13 @@ void  MainConnection::handleEvents()
         }
         m_nErrs = 0;
         createConnection();
+    } catch(...) {
+        mainThreadLock()->unlock();
+        throw;
+    }
+    mainThreadLock()->unlock();
     increaseStatusTo(Running);
-#if QT_VERSION >= 0x050000
+    while (status() < Stopping) {
-    while (m_status.load() < Stopping) {
-#else
-    while (m_status < Stopping) {
-#endif
         QMutexLocker l(mainThreadLock());
         if (m_nErrs > 10)
             increaseStatusTo(Stopping);
@@ -1861,16 +1858,22 @@ void  MainConnection::handleEvents()
 
 ZConfLib::ConnectionRef MainConnection::mainRef()
 {
-#if QT_VERSION >= 0x050000
+    while (status() < Running){
-    while (m_status.load() < Running){
-#else
-    while (m_status < Running){
-#endif
         QThread::yieldCurrentThread();
     }
     return m_mainRef;
 }
 
+MainConnection::Status MainConnection::status()
+{
+#if QT_VERSION >= 0x050000
+    return static_cast<MainConnection::Status>(m_status.load());
+#else
+    int val = m_status;
+    return static_cast<MainConnection::Status>(val);
+#endif
+}
+
 QList<ErrorMessage> MainConnection::errors()
 {
     QMutexLocker l(lock());

src/libs/zeroconf/servicebrowser_p.h

@@ -253,6 +253,7 @@ public:
     bool increaseStatusTo(int s);
     void handleEvents();
     ZConfLib::ConnectionRef mainRef();
+    Status status();
 
     QList<ErrorMessage> errors();
     bool isOk();