Merge lp:~unity-api-team/indicator-network/lp1411714-15.04 into lp:indicator-network/15.04

[Antti Kaijanmäki]

* Guard access to ofono::Modem properties that have a
  shared_ptr inside them with a mutex.
* simplify GMainLoopDispatcher

details from:

https://code.launchpad.net/~unity-api-team/indicator-network/lp1411714-14.09/+merge/247407

[Antti Kaijanmäki (kaijanmaki)]

14.09 approved

review: Approve

=== modified file 'src/dbus-cpp/services/ofono.h'
--- src/dbus-cpp/services/ofono.h	2014-10-23 21:46:41 +0000
+++ src/dbus-cpp/services/ofono.h	2015-01-30 12:31:15 +0000
@@ -1034,8 +1034,9 @@
                         break;
                     case Property::Interfaces::Type::NetworkRegistration:
                     {
+                        std::unique_lock<std::mutex> lock(_lock);
                         if (std::find(newInterfaces.begin(), newInterfaces.end(), known) == newInterfaces.end() &&
-                            networkRegistration->get()) {
+                            networkRegistration.get()) {
                             networkRegistration.set(std::shared_ptr<NetworkRegistration>());
                         }
                         break;
@@ -1046,8 +1047,9 @@
                         break;
                     case Property::Interfaces::Type::SimManager:
                     {
+                        std::unique_lock<std::mutex> lock(_lock);
                         if (std::find(newInterfaces.begin(), newInterfaces.end(), known) == newInterfaces.end() &&
-                            simManager->get()) {
+                            simManager.get()) {
                             simManager.set(std::shared_ptr<SimManager>());
                         }
                         break;
@@ -1080,7 +1082,8 @@
                         break;
                     case Property::Interfaces::Type::NetworkRegistration:
                     {
-                        if (!networkRegistration->get()) {
+                        std::unique_lock<std::mutex> lock(_lock);
+                        if (!networkRegistration.get()) {
                             networkRegistration.set(std::make_shared<NetworkRegistration>(this->object));
                         }
                         break;
@@ -1091,7 +1094,8 @@
                         break;
                     case Property::Interfaces::Type::SimManager:
                     {
-                        if (!simManager->get()) {
+                        std::unique_lock<std::mutex> lock(_lock);
+                        if (!simManager.get()) {
                             simManager.set(std::make_shared<SimManager>(this->object));
                         }
                         break;
@@ -1174,6 +1178,8 @@
 
         std::shared_ptr<core::dbus::Signal<Signal::PropertyChanged, Signal::PropertyChanged::ArgumentType>> propertyChanged;
 
+        // this lock must be acquired for any access to networkRegistration or simManager
+        std::mutex _lock;
         core::Property<NetworkRegistration::Ptr> networkRegistration;
         core::Property<SimManager::Ptr>          simManager;
 

=== modified file 'src/indicator/modem.cpp'
--- src/indicator/modem.cpp	2014-12-09 10:58:21 +0000
+++ src/indicator/modem.cpp	2015-01-30 12:31:15 +0000
@@ -65,20 +65,21 @@
         });
     });
 
-    simManagerChanged(m_ofonoModem->simManager.get());
+    std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
+    auto simmgr = m_ofonoModem->simManager.get();
+    auto netreg = m_ofonoModem->networkRegistration.get();
+    lock.unlock();
+
+    simManagerChanged(simmgr);
     m_ofonoModem->simManager.changed().connect([that](org::ofono::Interface::SimManager::Ptr simmgr)
     {
-        GMainLoopDispatch([that, simmgr](){
-            that->simManagerChanged(simmgr);
-        });
+        that->simManagerChanged(simmgr);
     });
 
-    networkRegistrationChanged(m_ofonoModem->networkRegistration.get());
+    networkRegistrationChanged(netreg);
     m_ofonoModem->networkRegistration.changed().connect([that](org::ofono::Interface::NetworkRegistration::Ptr netreg)
     {
-        GMainLoopDispatch([that, netreg](){
-            that->networkRegistrationChanged(netreg);
-        });
+        that->networkRegistrationChanged(netreg);
     });
 
     /// @todo hook up with system-settings to allow changing the identifier.
@@ -93,14 +94,17 @@
     } else {
         m_simIdentifier.set(path);
     }
-
 }
 
 
 void
 Modem::Private::update()
 {
+    std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
     auto simmgr = m_ofonoModem->simManager.get();
+    auto netreg = m_ofonoModem->networkRegistration.get();
+    lock.unlock();
+
     if (simmgr) {
         // update requiredPin
         switch(simmgr->pinRequired.get())
@@ -155,7 +159,6 @@
         m_simStatus.set(SimStatus::not_available);
     }
 
-    auto netreg = m_ofonoModem->networkRegistration.get();
     if (netreg) {
         m_operatorName.set(netreg->operatorName.get());
         m_status.set(netreg->status.get());
@@ -172,75 +175,55 @@
 void
 Modem::Private::networkRegistrationChanged(org::ofono::Interface::NetworkRegistration::Ptr netreg)
 {
+    auto that = shared_from_this();
     if (netreg) {
-        auto that = shared_from_this();
         netreg->operatorName.changed().connect([that](const std::string &)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
         netreg->status.changed().connect([that](org::ofono::Interface::NetworkRegistration::Status)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         netreg->strength.changed().connect([that](std::int8_t)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         netreg->technology.changed().connect([that](org::ofono::Interface::NetworkRegistration::Technology)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
     }
-    update();
+
+    GMainLoopDispatch([that]() { that->update(); });
 }
 
 
 void
 Modem::Private::simManagerChanged(org::ofono::Interface::SimManager::Ptr simmgr)
 {
+    auto that = shared_from_this();
     if (simmgr) {
-        auto that = shared_from_this();
         simmgr->present.changed().connect([that](bool)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         simmgr->pinRequired.changed().connect([that](org::ofono::Interface::SimManager::PinType)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
         simmgr->retries.changed().connect([that](std::map<org::ofono::Interface::SimManager::PinType, std::uint8_t>)
         {
-            GMainLoopDispatch([that]()
-            {
-                that->update();
-            });
+            GMainLoopDispatch([that]() { that->update(); });
         });
 
     }
-    update();
+    GMainLoopDispatch([that]() { that->update(); });
 }
 
 Modem::Modem(org::ofono::Interface::Modem::Ptr ofonoModem)
@@ -261,7 +244,11 @@
 bool
 Modem::enterPin(PinType type, const std::string &pin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -269,11 +256,11 @@
     case PinType::none:
         return true;
     case PinType::pin:
-        return d->m_ofonoModem->simManager.get()->enterPin(org::ofono::Interface::SimManager::PinType::pin,
-                                                           pin);
+        return simmgr->enterPin(org::ofono::Interface::SimManager::PinType::pin,
+                                pin);
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->enterPin(org::ofono::Interface::SimManager::PinType::puk,
-                                                           pin);
+        return simmgr->enterPin(org::ofono::Interface::SimManager::PinType::puk,
+                                pin);
         break;
     }
 
@@ -284,7 +271,11 @@
 bool
 Modem::resetPin(PinType type, const std::string &puk, const std::string &pin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -292,9 +283,9 @@
     case PinType::none:
         return true;
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->resetPin(org::ofono::Interface::SimManager::PinType::puk,
-                                                           puk,
-                                                           pin);
+        return simmgr->resetPin(org::ofono::Interface::SimManager::PinType::puk,
+                                puk,
+                                pin);
     default:
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": Not Supported.");
     }
@@ -303,7 +294,11 @@
 bool
 Modem::changePin(PinType type, const std::string &oldPin, const std::string &newPin)
 {
-    if (!d->m_ofonoModem->simManager.get()) {
+    std::unique_lock<std::mutex> lock(d->m_ofonoModem->_lock);
+    auto simmgr = d->m_ofonoModem->simManager.get();
+    lock.unlock();
+
+    if (!simmgr) {
         throw std::runtime_error(std::string(__PRETTY_FUNCTION__) + ": no simManager.");
     }
 
@@ -311,13 +306,13 @@
     case PinType::none:
         return true;
     case PinType::pin:
-        return d->m_ofonoModem->simManager.get()->changePin(org::ofono::Interface::SimManager::PinType::pin,
-                                                            oldPin,
-                                                            newPin);
+        return simmgr->changePin(org::ofono::Interface::SimManager::PinType::pin,
+                                 oldPin,
+                                 newPin);
     case PinType::puk:
-        return d->m_ofonoModem->simManager.get()->changePin(org::ofono::Interface::SimManager::PinType::puk,
-                                                            oldPin,
-                                                            newPin);
+        return simmgr->changePin(org::ofono::Interface::SimManager::PinType::puk,
+                                 oldPin,
+                                 newPin);
         break;
     }
 

=== modified file 'src/menumodel-cpp/gio-helpers/util.cpp'
--- src/menumodel-cpp/gio-helpers/util.cpp	2014-12-09 14:56:21 +0000
+++ src/menumodel-cpp/gio-helpers/util.cpp	2015-01-30 12:31:15 +0000
@@ -19,43 +19,40 @@
 
 #include "util.h"
 
+// guards access to __funcs
 std::mutex GMainLoopDispatch::_lock;
-std::list<GMainLoopDispatch::Func *> GMainLoopDispatch::_funcs;
+std::list<GMainLoopDispatch::Func> GMainLoopDispatch::__funcs;
 
 gboolean
 GMainLoopDispatch::dispatch_cb(gpointer)
 {
     std::unique_lock<std::mutex> lock(_lock);
-    for (auto func : _funcs) {
-        lock.unlock();
-        (*func)();
-        lock.lock();
-        delete func;
+    auto funcs = __funcs;
+    __funcs.clear();
+    lock.unlock();
+
+    for (auto &func : funcs) {
+        func();
     }
-    _funcs.clear();
     return G_SOURCE_REMOVE;
 }
 
 GMainLoopDispatch::GMainLoopDispatch(Func func, int priority, bool force_delayed)
 {
-    std::unique_lock<std::mutex> lock(_lock);
-
     if (!force_delayed &&
          g_main_context_acquire(g_main_context_default())) {
-        // already running inside GMainLoop..
-        // free the lock and dispatch immediately.
-        lock.unlock();
+        // already running inside GMainLoop.
         func();
         g_main_context_release(g_main_context_default());
     } else {
-        std::function<void()> *funcPtr = new std::function<void()>(func);
-        if (_funcs.empty()) {
+        std::unique_lock<std::mutex> lock(_lock);
+        if (__funcs.empty()) {
             g_idle_add_full(priority,
                             GSourceFunc(GMainLoopDispatch::dispatch_cb),
                             NULL,
                             NULL);
         }
-        _funcs.push_back(funcPtr);
+        __funcs.push_back(func);
     }
 }
 

=== modified file 'src/menumodel-cpp/gio-helpers/util.h'
--- src/menumodel-cpp/gio-helpers/util.h	2014-12-09 14:56:21 +0000
+++ src/menumodel-cpp/gio-helpers/util.h	2015-01-30 12:31:15 +0000
@@ -74,7 +74,7 @@
     typedef std::function<void()> Func;
     static std::mutex _lock;
     // list keeps the order of the functions
-    static std::list<Func *> _funcs;
+    static std::list<Func> __funcs;
 
     //GMainLoopDispatch() = delete;
     GMainLoopDispatch(Func func, int priority=G_PRIORITY_HIGH, bool force_delayed=false);