QDBusConnectionManager: Move all locking inside the class

Make `mutex` member private and make all former users call
new public member functions that perform locking. Make
old non-locking member functions private.

Change-Id: I29092d1bd785aa6b830183c1c1fe125b16e0d633
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

5 files changed, 59 insertions, 31 deletions

diff --git a/src/dbus/qdbusconnection.cpp b/src/dbus/qdbusconnection.cpp
index f0e38dcbd4..bd9df89aaa 100644
--- a/src/dbus/qdbusconnection.cpp
+++ b/src/dbus/qdbusconnection.cpp
@@ -7,7 +7,6 @@
 
 #include <qdebug.h>
 #include <qstringlist.h>
-#include <QtCore/private/qlocking_p.h>
 
 #include "qdbusconnectioninterface.h"
 #include "qdbuserror.h"
@@ -161,10 +160,7 @@ QDBusConnection::QDBusConnection(const QString &name)
     if (!manager) {
         d = nullptr;
     } else {
-        const auto locker = qt_scoped_lock(manager->mutex);
-        d = manager->connection(name);
-        if (d)
-            d->ref.ref();
+        d = manager->existingConnection(name);
     }
 }
 
@@ -276,14 +272,10 @@ QDBusConnection QDBusConnection::connectToPeer(const QString &address,
 void QDBusConnection::disconnectFromBus(const QString &name)
 {
     auto *manager = QDBusConnectionManager::instance();
+    if (!manager)
+        return;
 
-    if (manager) {
-        const auto locker = qt_scoped_lock(manager->mutex);
-        QDBusConnectionPrivate *d = manager->connection(name);
-        if (d && d->mode != QDBusConnectionPrivate::ClientMode)
-            return;
-        manager->removeConnection(name);
-    }
+    manager->disconnectFrom(name, QDBusConnectionPrivate::ClientMode);
 }
 
 /*!
@@ -299,14 +291,10 @@ void QDBusConnection::disconnectFromBus(const QString &name)
 void QDBusConnection::disconnectFromPeer(const QString &name)
 {
     auto *manager = QDBusConnectionManager::instance();
+    if (!manager)
+        return;
 
-    if (manager) {
-        const auto locker = qt_scoped_lock(manager->mutex);
-        QDBusConnectionPrivate *d = manager->connection(name);
-        if (d && d->mode != QDBusConnectionPrivate::PeerMode)
-            return;
-        manager->removeConnection(name);
-    }
+    manager->disconnectFrom(name, QDBusConnectionPrivate::PeerMode);
 }
 
 /*!
diff --git a/src/dbus/qdbusconnectionmanager.cpp b/src/dbus/qdbusconnectionmanager.cpp
index 1107841f38..73b993d566 100644
--- a/src/dbus/qdbusconnectionmanager.cpp
+++ b/src/dbus/qdbusconnectionmanager.cpp
@@ -6,6 +6,7 @@
 
 #include <qcoreapplication.h>
 #include <qthread.h>
+#include <qstringlist.h>
 #include <QtCore/private/qlocking_p.h>
 
 #include "qdbuserror.h"
@@ -49,6 +50,15 @@ QDBusConnectionPrivate *QDBusConnectionManager::connection(const QString &name)
     return connectionHash.value(name, nullptr);
 }
 
+QDBusConnectionPrivate *QDBusConnectionManager::existingConnection(const QString &name) const
+{
+    const auto locker = qt_scoped_lock(mutex);
+    auto *conn = connection(name);
+    if (conn)
+        conn->ref.ref();
+    return conn;
+}
+
 void QDBusConnectionManager::removeConnection(const QString &name)
 {
     QDBusConnectionPrivate *d = nullptr;
@@ -63,6 +73,25 @@ void QDBusConnectionManager::removeConnection(const QString &name)
     // ### Output a warning if connections are being used after they have been removed.
 }
 
+void QDBusConnectionManager::removeConnections(const QStringList &names)
+{
+    const auto locker = qt_scoped_lock(mutex);
+
+    for (const auto &name : names)
+        removeConnection(name);
+}
+
+void QDBusConnectionManager::disconnectFrom(const QString &name,
+                                            QDBusConnectionPrivate::ConnectionMode mode)
+{
+    const auto locker = qt_scoped_lock(mutex);
+
+    QDBusConnectionPrivate *d = connection(name);
+    if (d && d->mode != mode)
+        return;
+    removeConnection(name);
+}
+
 QDBusConnectionManager::QDBusConnectionManager()
 {
     // Ensure that the custom metatype registry is created before the instance
@@ -104,6 +133,12 @@ void QDBusConnectionManager::setConnection(const QString &name, QDBusConnectionP
     c->name = name;
 }
 
+void QDBusConnectionManager::addConnection(const QString &name, QDBusConnectionPrivate *c)
+{
+    const auto locker = qt_scoped_lock(mutex);
+    setConnection(name, c);
+}
+
 void QDBusConnectionManager::run()
 {
     exec();
diff --git a/src/dbus/qdbusconnectionmanager_p.h b/src/dbus/qdbusconnectionmanager_p.h
index a23a211838..ac67aa38a4 100644
--- a/src/dbus/qdbusconnectionmanager_p.h
+++ b/src/dbus/qdbusconnectionmanager_p.h
@@ -36,17 +36,18 @@ public:
     static QDBusConnectionManager* instance();
 
     QDBusConnectionPrivate *busConnection(QDBusConnection::BusType type);
-    QDBusConnectionPrivate *connection(const QString &name) const;
-    void removeConnection(const QString &name);
-    void setConnection(const QString &name, QDBusConnectionPrivate *c);
+    QDBusConnectionPrivate *existingConnection(const QString &name) const;
+
+    void removeConnections(const QStringList &names);
+    void disconnectFrom(const QString &name, QDBusConnectionPrivate::ConnectionMode mode);
+    void addConnection(const QString &name, QDBusConnectionPrivate *c);
+
     QDBusConnectionPrivate *connectToBus(QDBusConnection::BusType type, const QString &name, bool suspendedDelivery);
     QDBusConnectionPrivate *connectToBus(const QString &address, const QString &name);
     QDBusConnectionPrivate *connectToPeer(const QString &address, const QString &name);
 
     void createServer(const QString &address, QDBusServer *server);
 
-    mutable QMutex mutex;
-
 protected:
     void run() override;
 
@@ -56,7 +57,11 @@ private:
     QDBusConnectionPrivate *doConnectToBus(const QString &address, const QString &name);
     QDBusConnectionPrivate *doConnectToPeer(const QString &address, const QString &name);
 
+    mutable QMutex mutex;
     QHash<QString, QDBusConnectionPrivate *> connectionHash;
+    QDBusConnectionPrivate *connection(const QString &name) const;
+    void removeConnection(const QString &name);
+    void setConnection(const QString &name, QDBusConnectionPrivate *c);
 
     QMutex defaultBusMutex;
     QDBusConnectionPrivate *defaultBuses[2];
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index a25ade92ce..1bc827daf5 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -291,7 +291,8 @@ static void qDBusNewConnection(DBusServer *server, DBusConnection *connection, v
     Q_ASSERT(connection);
     Q_ASSERT(data);
 
-    if (!QDBusConnectionManager::instance())
+    auto *manager = QDBusConnectionManager::instance();
+    if (!manager)
         return;
 
     // keep the connection alive
@@ -303,8 +304,10 @@ static void qDBusNewConnection(DBusServer *server, DBusConnection *connection, v
         q_dbus_connection_set_allow_anonymous(connection, true);
 
     QDBusConnectionPrivate *newConnection = new QDBusConnectionPrivate(serverConnection->parent());
-    const auto locker = qt_scoped_lock(QDBusConnectionManager::instance()->mutex);
-    QDBusConnectionManager::instance()->setConnection("QDBusServer-"_L1 + QString::number(reinterpret_cast<qulonglong>(newConnection), 16), newConnection);
+
+    manager->addConnection(
+            "QDBusServer-"_L1 + QString::number(reinterpret_cast<qulonglong>(newConnection), 16),
+            newConnection);
     serverConnection->serverConnectionNames << newConnection->name;
 
     // setPeer does the error handling for us
diff --git a/src/dbus/qdbusserver.cpp b/src/dbus/qdbusserver.cpp
index e298b14bf1..cb3cee3d6b 100644
--- a/src/dbus/qdbusserver.cpp
+++ b/src/dbus/qdbusserver.cpp
@@ -86,12 +86,9 @@ QDBusServer::~QDBusServer()
     if (!manager)
         return;
 
-    QMutexLocker locker(&manager->mutex);
     QWriteLocker writeLocker(&d->lock);
-    for (const QString &name : std::as_const(d->serverConnectionNames))
-        manager->removeConnection(name);
+    manager->removeConnections(d->serverConnectionNames);
     d->serverConnectionNames.clear();
-    locker.unlock();
 
     d->serverObject = nullptr;
     d->ref.storeRelaxed(0);