From 745e276f6802f0f7fb2d548d04bf120c5516d7c6 Mon Sep 17 00:00:00 2001 From: Aaron McCarthy Date: Wed, 26 May 2010 14:24:26 +1000 Subject: Fix locking after merge from Qt Mobility. Simplify locking by using inline getter functions. Task-number: QTBUG-10296 --- .../bearer/symbian/qnetworksession_impl.cpp | 102 ++++++--------------- src/plugins/bearer/symbian/symbianengine.cpp | 70 ++++++++------ src/plugins/bearer/symbian/symbianengine.h | 18 +++- 3 files changed, 88 insertions(+), 102 deletions(-) diff --git a/src/plugins/bearer/symbian/qnetworksession_impl.cpp b/src/plugins/bearer/symbian/qnetworksession_impl.cpp index 3cd18fb..e08d135 100644 --- a/src/plugins/bearer/symbian/qnetworksession_impl.cpp +++ b/src/plugins/bearer/symbian/qnetworksession_impl.cpp @@ -122,19 +122,10 @@ void QNetworkSessionPrivateImpl::configurationRemoved(QNetworkConfigurationPriva if (!publicConfig.isValid()) return; - SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(config); - - symbianConfig->mutex.lock(); - TUint32 configNumericId = symbianConfig->numericId; - symbianConfig->mutex.unlock(); - - symbianConfig = toSymbianConfig(privateConfiguration(publicConfig)); - - symbianConfig->mutex.lock(); - TUint32 publicNumericId = symbianConfig->numericId; - symbianConfig->mutex.unlock(); + TUint32 publicNumericId = + toSymbianConfig(privateConfiguration(publicConfig))->numericIdentifier(); - if (configNumericId == publicNumericId) { + if (toSymbianConfig(config)->numericIdentifier() == publicNumericId) { #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug() << "QNS this : " << QString::number((uint)this) << " - " << "configurationRemoved IAP: " << QString::number(publicNumericId) << " : going to State: Invalid"; @@ -380,13 +371,11 @@ void QNetworkSessionPrivateImpl::open() SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(publicConfig)); - QMutexLocker configLocker(&symbianConfig->mutex); - - if (connInfo().iIapId == symbianConfig->numericId) { + if (connInfo().iIapId == symbianConfig->numericIdentifier()) { if (iConnection.Attach(connInfo, RConnection::EAttachTypeNormal) == KErrNone) { activeConfig = publicConfig; #ifndef QT_NO_NETWORKINTERFACE - activeInterface = interface(symbianConfig->numericId); + activeInterface = interface(symbianConfig->numericIdentifier()); #endif connected = ETrue; startTime = QDateTime::currentDateTime(); @@ -416,9 +405,7 @@ void QNetworkSessionPrivateImpl::open() TConnPrefList pref; TExtendedConnPref prefs; - symbianConfig->mutex.lock(); - prefs.SetIapId(symbianConfig->numericId); - symbianConfig->mutex.unlock(); + prefs.SetIapId(symbianConfig->numericIdentifier()); if (iConnectInBackground) { prefs.SetNoteBehaviour( TExtendedConnPref::ENoteBehaviourConnSilent ); } @@ -427,9 +414,7 @@ void QNetworkSessionPrivateImpl::open() TCommDbConnPref pref; pref.SetDialogPreference(ECommDbDialogPrefDoNotPrompt); - symbianConfig->mutex.lock(); - pref.SetIapId(symbianConfig->numericId); - symbianConfig->mutex.unlock(); + pref.SetIapId(symbianConfig->numericIdentifier()); #endif iConnection.Start(pref, iStatus); if (!IsActive()) { @@ -444,17 +429,13 @@ void QNetworkSessionPrivateImpl::open() #ifdef OCC_FUNCTIONALITY_AVAILABLE TConnPrefList snapPref; TExtendedConnPref prefs; - symbianConfig->mutex.lock(); - prefs.SetSnapId(symbianConfig->numericId); - symbianConfig->mutex.unlock(); + prefs.SetSnapId(symbianConfig->numericIdentifier()); if (iConnectInBackground) { prefs.SetNoteBehaviour( TExtendedConnPref::ENoteBehaviourConnSilent ); } snapPref.AppendL(&prefs); #else - symbianConfig->mutex.lock(); - TConnSnapPref snapPref(symbianConfig->numericId); - symbianConfig->mutex.unlock(); + TConnSnapPref snapPref(symbianConfig->numericIdentifier()); #endif iConnection.Start(snapPref, iStatus); if (!IsActive()) { @@ -592,10 +573,8 @@ void QNetworkSessionPrivateImpl::stop() SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(publicConfig)); - QMutexLocker configLocker(&symbianConfig->mutex); - // See if connection Id matches with our Id. If so, stop() it. - if (symbianConfig->connectionId == connectionId) { + if (symbianConfig->connectionIdentifier() == connectionId) { ret = iConnectionMonitor.SetBoolAttribute(connectionId, 0, // subConnectionId don't care KConnectionStop, @@ -715,12 +694,8 @@ void QNetworkSessionPrivateImpl::PreferredCarrierAvailable(TAccessPointInfo aOld SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(configs[i])); - QMutexLocker configLocker(&symbianConfig->mutex); - - if (symbianConfig->numericId == aNewAPInfo.AccessPoint()) { - configLocker.unlock(); + if (symbianConfig->numericIdentifier() == aNewAPInfo.AccessPoint()) emit preferredConfigurationChanged(configs[i], aIsSeamless); - } } } else { migrate(); @@ -854,9 +829,7 @@ quint64 QNetworkSessionPrivateImpl::transferredData(TUint dataType) const SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(configs[i])); - QMutexLocker configLocker(&symbianConfig->mutex); - - if (symbianConfig->numericId == apId) { + if (symbianConfig->numericIdentifier() == apId) { configFound = true; break; } @@ -865,10 +838,8 @@ quint64 QNetworkSessionPrivateImpl::transferredData(TUint dataType) const SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(config)); - symbianConfig->mutex.lock(); - if (symbianConfig->numericId == apId) + if (symbianConfig->numericIdentifier() == apId) configFound = true; - symbianConfig->mutex.unlock(); } if (configFound) { TUint tData; @@ -908,8 +879,7 @@ QNetworkConfiguration QNetworkSessionPrivateImpl::activeConfiguration(TUint32 ia SymbianNetworkConfigurationPrivate *childConfig = toSymbianConfig(privateConfiguration(children[i])); - QMutexLocker childLocker(&childConfig->mutex); - if (childConfig->numericId == iapId) + if (childConfig->numericIdentifier() == iapId) return children[i]; } @@ -928,15 +898,11 @@ QNetworkConfiguration QNetworkSessionPrivateImpl::activeConfiguration(TUint32 ia SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(pt)); if (symbianConfig) { - QMutexLocker configLocker(&symbianConfig->mutex); - for (int i=0; i < children.count(); i++) { SymbianNetworkConfigurationPrivate *childConfig = toSymbianConfig(privateConfiguration(children[i])); - QMutexLocker childLocker(&childConfig->mutex); - - if (childConfig->mappingName == symbianConfig->mappingName) { + if (childConfig->configMappingName() == symbianConfig->configMappingName()) { return children[i]; } } @@ -1034,15 +1000,13 @@ void QNetworkSessionPrivateImpl::RunL() SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(activeConfig)); - symbianConfig->mutex.lock(); #ifndef QT_NO_NETWORKINTERFACE - activeInterface = interface(symbianConfig->numericId); + activeInterface = interface(symbianConfig->numericIdentifier()); #endif if (publicConfig.type() == QNetworkConfiguration::UserChoice) { serviceConfig = QNetworkConfigurationManager() - .configurationFromIdentifier(symbianConfig->id); + .configurationFromIdentifier(activeConfig.identifier()); } - symbianConfig->mutex.unlock(); startTime = QDateTime::currentDateTime(); @@ -1105,13 +1069,11 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint newState == QNetworkSession::Connected) { activeConfig = activeConfiguration(accessPointId); +#ifndef QT_NO_NETWORKINTERFACE SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(activeConfig)); -#ifndef QT_NO_NETWORKINTERFACE - symbianConfig->mutex.lock(); - activeInterface = interface(symbianConfig->numericId); - symbianConfig->mutex.unlock(); + activeInterface = interface(symbianConfig->numericIdentifier()); #endif #ifdef SNAP_FUNCTIONALITY_AVAILABLE @@ -1187,10 +1149,7 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(publicConfig)); - QMutexLocker configLocker(&symbianConfig->mutex); - if (symbianConfig->numericId == accessPointId) { - configLocker.unlock(); - + if (symbianConfig->numericIdentifier() == accessPointId) { state = newState; #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug() << "QNS this : " << QString::number((uint)this) << " - " << "===> EMIT State changed B to: " << state; @@ -1202,10 +1161,7 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(activeConfig)); - QMutexLocker configLocker(&symbianConfig->mutex); - if (symbianConfig->numericId == accessPointId) { - configLocker.unlock(); - + if (symbianConfig->numericIdentifier() == accessPointId) { state = newState; #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug() << "QNS this : " << QString::number((uint)this) << " - " << "===> EMIT State changed C to: " << state; @@ -1219,9 +1175,7 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(subConfigurations[i])); - QMutexLocker configLocker(&symbianConfig->mutex); - - if (symbianConfig->numericId == accessPointId) { + if (symbianConfig->numericIdentifier() == accessPointId) { if (newState != QNetworkSession::Disconnected) { state = newState; #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG @@ -1234,8 +1188,6 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint if ((config.state() == QNetworkConfiguration::Defined) || (config.state() == QNetworkConfiguration::Discovered)) { - configLocker.unlock(); - state = newState; #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug() << "QNS this : " << QString::number((uint)this) << " - " << "===> EMIT State changed E to: " << state; @@ -1271,9 +1223,7 @@ bool QNetworkSessionPrivateImpl::newState(QNetworkSession::State newState, TUint SymbianNetworkConfigurationPrivate *symbianConfig = toSymbianConfig(privateConfiguration(publicConfig)); - symbianConfig->mutex.lock(); - iDeprecatedConnectionId = symbianConfig->connectionId; - symbianConfig->mutex.unlock(); + iDeprecatedConnectionId = symbianConfig->connectionIdentifier(); } return retVal; @@ -1369,7 +1319,11 @@ void QNetworkSessionPrivateImpl::handleSymbianConnectionStatusChange(TInt aConne qDebug() << "QNS this : " << QString::number((uint)this) << " - " << "reporting disconnection to manager."; #endif if (publicConfig.isValid()) { - engine->configurationStateChangeReport(toSymbianConfig(privateConfiguration(publicConfig))->numericId, QNetworkSession::Disconnected); + SymbianNetworkConfigurationPrivate *symbianConfig = + toSymbianConfig(privateConfiguration(publicConfig)); + + engine->configurationStateChangeReport(symbianConfig->numericIdentifier(), + QNetworkSession::Disconnected); } break; // Unhandled state diff --git a/src/plugins/bearer/symbian/symbianengine.cpp b/src/plugins/bearer/symbian/symbianengine.cpp index cea8b67..ed6aee6 100644 --- a/src/plugins/bearer/symbian/symbianengine.cpp +++ b/src/plugins/bearer/symbian/symbianengine.cpp @@ -338,34 +338,35 @@ void SymbianEngine::updateConfigurationsL() TRAP(error, cpPriv = configFromConnectionMethodL(connectionMethod)); if (error == KErrNone) { QNetworkConfigurationPrivatePointer ptr(cpPriv); - toSymbianConfig(ptr)->serviceNetworkPtr = privSNAP; accessPointConfigurations.insert(ptr->id, ptr); locker.unlock(); emit configurationAdded(ptr); locker.relock(); + QMutexLocker configLocker(&privSNAP->mutex); privSNAP->serviceNetworkMembers.append(ptr); } } else { knownConfigs.removeOne(iface); // Check that IAP can be found from related SNAP's configuration list bool iapFound = false; + QMutexLocker snapConfigLocker(&privSNAP->mutex); for (int i = 0; i < privSNAP->serviceNetworkMembers.count(); i++) { - if (toSymbianConfig(privSNAP->serviceNetworkMembers[i])->numericId == iapId) { + if (toSymbianConfig(privSNAP->serviceNetworkMembers[i])->numericIdentifier() == + iapId) { iapFound = true; break; } } - if (!iapFound) { - toSymbianConfig(priv)->serviceNetworkPtr = privSNAP; + if (!iapFound) privSNAP->serviceNetworkMembers.append(priv); - } } CleanupStack::PopAndDestroy(&connectionMethod); } + QMutexLocker snapConfigLocker(&privSNAP->mutex); if (privSNAP->serviceNetworkMembers.count() > 1) { // Roaming is supported only if SNAP contains more than one IAP privSNAP->roamingSupported = true; @@ -418,9 +419,10 @@ void SymbianEngine::updateConfigurationsL() foreach (const QString &iface, snapConfigurations.keys()) { QNetworkConfigurationPrivatePointer ptr2 = snapConfigurations.value(iface); // => Check if one of the IAPs of the SNAP is active + QMutexLocker snapConfigLocker(&ptr2->mutex); for (int i = 0; i < ptr2->serviceNetworkMembers.count(); ++i) { - if (toSymbianConfig(ptr2->serviceNetworkMembers[i])->numericId == - toSymbianConfig(ptr)->numericId) { + if (toSymbianConfig(ptr2->serviceNetworkMembers[i])->numericIdentifier() == + toSymbianConfig(ptr)->numericIdentifier()) { ptr2->serviceNetworkMembers.removeAt(i); break; } @@ -639,12 +641,14 @@ QNetworkConfigurationPrivatePointer SymbianEngine::defaultConfigurationL() } #endif - if (!ptr || !ptr->isValid) { - QString iface = QString::number(qHash(KUserChoiceIAPId)); - ptr = userChoiceConfigurations.value(iface); + if (ptr) { + QMutexLocker configLocker(&ptr->mutex); + if (ptr->isValid) + return ptr; } - - return ptr; + + QString iface = QString::number(qHash(KUserChoiceIAPId)); + return userChoiceConfigurations.value(iface); } void SymbianEngine::updateActiveAccessPoints() @@ -681,8 +685,9 @@ void SymbianEngine::updateActiveAccessPoints() online = true; inactiveConfigs.removeOne(ident); - QMutexLocker configLocker(&ptr->mutex); + ptr->mutex.lock(); toSymbianConfig(ptr)->connectionId = connectionId; + ptr->mutex.unlock(); // Configuration is Active changeConfigurationStateTo(ptr, QNetworkConfiguration::Active); @@ -736,6 +741,8 @@ void SymbianEngine::accessPointScanningReady(TBool scanSuccessful, TConnMonIapIn QNetworkConfigurationPrivatePointer ptr = accessPointConfigurations.value(ident); if (ptr) { unavailableConfigs.removeOne(ident); + + QMutexLocker configLocker(&ptr->mutex); if (ptr->state < QNetworkConfiguration::Active) { // Configuration is either Discovered or Active changeConfigurationStateAtMinTo(ptr, QNetworkConfiguration::Discovered); @@ -772,10 +779,15 @@ void SymbianEngine::updateStatesToSnaps() bool discovered = false; bool active = false; QNetworkConfigurationPrivatePointer ptr = snapConfigurations.value(iface); + + QMutexLocker snapConfigLocker(&ptr->mutex); + // => Check if one of the IAPs of the SNAP is discovered or active // => If one of IAPs is active, also SNAP is active // => If one of IAPs is discovered but none of the IAPs is active, SNAP is discovered for (int i=0; iserviceNetworkMembers.count(); i++) { + QMutexLocker configLocker(&ptr->serviceNetworkMembers[i]->mutex); + if ((ptr->serviceNetworkMembers[i]->state & QNetworkConfiguration::Active) == QNetworkConfiguration::Active) { active = true; @@ -991,9 +1003,11 @@ void SymbianEngine::EventL(const CConnMonEventBase& aEvent) QString ident = QString::number(qHash(apId)); QNetworkConfigurationPrivatePointer ptr = accessPointConfigurations.value(ident); if (ptr) { - QMutexLocker configLocker(&ptr->mutex); + ptr->mutex.lock(); toSymbianConfig(ptr)->connectionId = connectionId; - emit this->configurationStateChanged(toSymbianConfig(ptr)->numericId, connectionId, QNetworkSession::Connecting); + ptr->mutex.unlock(); + emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), + connectionId, QNetworkSession::Connecting); } } else if (connectionStatus == KLinkLayerOpen) { // Connection has been successfully opened @@ -1006,13 +1020,17 @@ void SymbianEngine::EventL(const CConnMonEventBase& aEvent) QString ident = QString::number(qHash(apId)); QNetworkConfigurationPrivatePointer ptr = accessPointConfigurations.value(ident); if (ptr) { - QMutexLocker configLocker(&ptr->mutex); + ptr->mutex.lock(); toSymbianConfig(ptr)->connectionId = connectionId; + ptr->mutex.unlock(); + // Configuration is Active if (changeConfigurationStateTo(ptr, QNetworkConfiguration::Active)) { updateStatesToSnaps(); } - emit this->configurationStateChanged(toSymbianConfig(ptr)->numericId, connectionId, QNetworkSession::Connected); + emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), + connectionId, QNetworkSession::Connected); + if (!iOnline) { iOnline = true; emit this->onlineStateChanged(iOnline); @@ -1022,8 +1040,8 @@ void SymbianEngine::EventL(const CConnMonEventBase& aEvent) TUint connectionId = realEvent->ConnectionId(); QNetworkConfigurationPrivatePointer ptr = dataByConnectionId(connectionId); if (ptr) { - QMutexLocker configLocker(&ptr->mutex); - emit this->configurationStateChanged(toSymbianConfig(ptr)->numericId, connectionId, QNetworkSession::Closing); + emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), + connectionId, QNetworkSession::Closing); } } else if (connectionStatus == KLinkLayerClosed || connectionStatus == KConnectionClosed) { @@ -1037,8 +1055,8 @@ void SymbianEngine::EventL(const CConnMonEventBase& aEvent) updateStatesToSnaps(); } - QMutexLocker configLocker(&ptr->mutex); - emit this->configurationStateChanged(toSymbianConfig(ptr)->numericId, connectionId, QNetworkSession::Disconnected); + emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), + connectionId, QNetworkSession::Disconnected); } bool online = false; @@ -1135,10 +1153,9 @@ void SymbianEngine::configurationStateChangeReport(TUint32 accessPointId, QNetwo updateStatesToSnaps(); } - QMutexLocker configLocker(&ptr->mutex); - emit this->configurationStateChanged(toSymbianConfig(ptr)->numericId, - toSymbianConfig(ptr)->connectionId, - QNetworkSession::Disconnected); + emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), + toSymbianConfig(ptr)->connectionIdentifier(), + QNetworkSession::Disconnected); } } break; @@ -1170,8 +1187,7 @@ QNetworkConfigurationPrivatePointer SymbianEngine::dataByConnectionId(TUint aCon accessPointConfigurations.constBegin(); while (i != accessPointConfigurations.constEnd()) { QNetworkConfigurationPrivatePointer ptr = i.value(); - QMutexLocker configLocker(&ptr->mutex); - if (toSymbianConfig(ptr)->connectionId == aConnectionId) + if (toSymbianConfig(ptr)->connectionIdentifier() == aConnectionId) return ptr; ++i; diff --git a/src/plugins/bearer/symbian/symbianengine.h b/src/plugins/bearer/symbian/symbianengine.h index 7d565db..841d79d 100644 --- a/src/plugins/bearer/symbian/symbianengine.h +++ b/src/plugins/bearer/symbian/symbianengine.h @@ -87,7 +87,23 @@ public: QString bearerName() const; - QNetworkConfigurationPrivatePointer serviceNetworkPtr; + inline TUint32 numericIdentifier() const + { + QMutexLocker locker(&mutex); + return numericId; + } + + inline TUint connectionIdentifier() const + { + QMutexLocker locker(&mutex); + return connectionId; + } + + inline QString configMappingName() const + { + QMutexLocker locker(&mutex); + return mappingName; + } QString mappingName; -- cgit v0.12 From 2666e68f49c4a8185de796438fd3f672fcec0365 Mon Sep 17 00:00:00 2001 From: Aaron McCarthy Date: Fri, 28 May 2010 09:30:54 +1000 Subject: Fix up cancelling of active object. Task-number: QTBUG-10296 --- src/plugins/bearer/symbian/symbianengine.cpp | 78 ++++++++++++---------------- 1 file changed, 34 insertions(+), 44 deletions(-) diff --git a/src/plugins/bearer/symbian/symbianengine.cpp b/src/plugins/bearer/symbian/symbianengine.cpp index ed6aee6..bc93d0e 100644 --- a/src/plugins/bearer/symbian/symbianengine.cpp +++ b/src/plugins/bearer/symbian/symbianengine.cpp @@ -899,15 +899,7 @@ void SymbianEngine::stopCommsDatabaseNotifications() if (iWaitingCommsDatabaseNotifications) { iWaitingCommsDatabaseNotifications = EFalse; - if (!IsActive()) { - SetActive(); - // Make sure that notifier recorded events will not be returned - // as soon as the client issues the next RequestNotification() request. - ipCommsDB->RequestNotification(iStatus); - ipCommsDB->CancelRequestNotification(); - } else { - ipCommsDB->CancelRequestNotification(); - } + Cancel(); } } @@ -922,46 +914,44 @@ void SymbianEngine::RunL() return; } - if (iStatus != KErrCancel) { - RDbNotifier::TEvent event = STATIC_CAST(RDbNotifier::TEvent, iStatus.Int()); + RDbNotifier::TEvent event = STATIC_CAST(RDbNotifier::TEvent, iStatus.Int()); - switch (event) { - case RDbNotifier::EUnlock: /** All read locks have been removed. */ - case RDbNotifier::ECommit: /** A transaction has been committed. */ - case RDbNotifier::ERollback: /** A transaction has been rolled back */ - case RDbNotifier::ERecover: /** The database has been recovered */ + switch (event) { + case RDbNotifier::EUnlock: /** All read locks have been removed. */ + case RDbNotifier::ECommit: /** A transaction has been committed. */ + case RDbNotifier::ERollback: /** A transaction has been rolled back */ + case RDbNotifier::ERecover: /** The database has been recovered */ #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG - qDebug("QNCM CommsDB event (of type RDbNotifier::TEvent) received: %d", iStatus.Int()); + qDebug("QNCM CommsDB event (of type RDbNotifier::TEvent) received: %d", iStatus.Int()); #endif - iIgnoringUpdates = true; - // Other events than ECommit get lower priority. In practice with those events, - // we delay_before_updating methods, whereas - // with ECommit we _update_before_delaying the reaction to next event. - // Few important notes: 1) listening to only ECommit does not seem to be adequate, - // but updates will be missed. Hence other events are reacted upon too. - // 2) RDbNotifier records the most significant event, and that will be returned once - // we issue new RequestNotification, and hence updates will not be missed even - // when we are 'not reacting to them' for few seconds. - if (event == RDbNotifier::ECommit) { - TRAPD(error, updateConfigurationsL()); - if (error == KErrNone) { - updateStatesToSnaps(); - } - waitRandomTime(); - } else { - waitRandomTime(); - TRAPD(error, updateConfigurationsL()); - if (error == KErrNone) { - updateStatesToSnaps(); - } + iIgnoringUpdates = true; + // Other events than ECommit get lower priority. In practice with those events, + // we delay_before_updating methods, whereas + // with ECommit we _update_before_delaying the reaction to next event. + // Few important notes: 1) listening to only ECommit does not seem to be adequate, + // but updates will be missed. Hence other events are reacted upon too. + // 2) RDbNotifier records the most significant event, and that will be returned once + // we issue new RequestNotification, and hence updates will not be missed even + // when we are 'not reacting to them' for few seconds. + if (event == RDbNotifier::ECommit) { + TRAPD(error, updateConfigurationsL()); + if (error == KErrNone) { + updateStatesToSnaps(); + } + waitRandomTime(); + } else { + waitRandomTime(); + TRAPD(error, updateConfigurationsL()); + if (error == KErrNone) { + updateStatesToSnaps(); } - iIgnoringUpdates = false; // Wait time done, allow updating again - iWaitingCommsDatabaseNotifications = true; - break; - default: - // Do nothing - break; } + iIgnoringUpdates = false; // Wait time done, allow updating again + iWaitingCommsDatabaseNotifications = true; + break; + default: + // Do nothing + break; } if (iWaitingCommsDatabaseNotifications) { -- cgit v0.12 From feb9949a11abbf244e0bfcdc128981596758de40 Mon Sep 17 00:00:00 2001 From: Aaron McCarthy Date: Fri, 28 May 2010 10:07:00 +1000 Subject: Fix multithreaded use of bearer management on Symbian. Move more initialization code into initialize() function. The constructor for the SymbianEngine class can be called from any thread. Move all initialization into initialize() function which is guarinteed to be run in the main thread. This is required as some Symbian resources are not shared between thread in the same process. The implementation of the defaultConfiguration() function could only be called from the main thread. Cache a copy of the default network configuration when the configurations are updated and just return this when the defaultConfiguration() function is called. Only lock the engine mutex from external entry points. Remove need for locking in waitRandomTime() function. Prevents dead lock due to nested event loop. Task-number: QTBUG-10296 --- src/plugins/bearer/symbian/symbianengine.cpp | 126 +++++++++++---------------- src/plugins/bearer/symbian/symbianengine.h | 6 +- 2 files changed, 52 insertions(+), 80 deletions(-) diff --git a/src/plugins/bearer/symbian/symbianengine.cpp b/src/plugins/bearer/symbian/symbianengine.cpp index bc93d0e..8c26cf0 100644 --- a/src/plugins/bearer/symbian/symbianengine.cpp +++ b/src/plugins/bearer/symbian/symbianengine.cpp @@ -115,13 +115,18 @@ QString SymbianNetworkConfigurationPrivate::bearerName() const SymbianEngine::SymbianEngine(QObject *parent) : QBearerEngine(parent), CActive(CActive::EPriorityIdle), iFirstUpdate(true), iInitOk(true), - iIgnoringUpdates(false), iTimeToWait(0), iIgnoreEventLoop(0) + iIgnoringUpdates(false) { +} + +void SymbianEngine::initialize() +{ + QMutexLocker locker(&mutex); + CActiveScheduler::Add(this); // Seed the randomgenerator qsrand(QTime(0,0,0).secsTo(QTime::currentTime()) + QCoreApplication::applicationPid()); - iIgnoreEventLoop = new QEventLoop(this); TRAPD(error, ipCommsDB = CCommsDatabase::NewL(EDatabaseTypeIAP)); if (error != KErrNone) { @@ -132,18 +137,13 @@ SymbianEngine::SymbianEngine(QObject *parent) TRAP_IGNORE(iConnectionMonitor.ConnectL()); TRAP_IGNORE(iConnectionMonitor.NotifyEventL(*this)); -#ifdef SNAP_FUNCTIONALITY_AVAILABLE +#ifdef SNAP_FUNCTIONALITY_AVAILABLE TRAP(error, iCmManager.OpenL()); if (error != KErrNone) { iInitOk = false; return; } #endif -} - -void SymbianEngine::initialize() -{ - QMutexLocker locker(&mutex); SymbianNetworkConfigurationPrivate *cpPriv = new SymbianNetworkConfigurationPrivate; cpPriv->name = "UserChoice"; @@ -238,19 +238,14 @@ void SymbianEngine::requestUpdate() void SymbianEngine::updateConfigurations() { - QMutexLocker locker(&mutex); - - if (!iInitOk) { + if (!iInitOk) return; - } TRAP_IGNORE(updateConfigurationsL()); } void SymbianEngine::updateConfigurationsL() { - QMutexLocker locker(&mutex); - QList knownConfigs = accessPointConfigurations.keys(); QList knownSnapConfigs = snapConfigurations.keys(); @@ -276,9 +271,9 @@ void SymbianEngine::updateConfigurationsL() QNetworkConfigurationPrivatePointer ptr(cpPriv); accessPointConfigurations.insert(ptr->id, ptr); - locker.unlock(); + mutex.unlock(); emit configurationAdded(ptr); - locker.relock(); + mutex.lock(); } } CleanupStack::PopAndDestroy(&connectionMethod); @@ -317,9 +312,9 @@ void SymbianEngine::updateConfigurationsL() QNetworkConfigurationPrivatePointer ptr(cpPriv); snapConfigurations.insert(ident, ptr); - locker.unlock(); + mutex.unlock(); emit configurationAdded(ptr); - locker.relock(); + mutex.lock(); CleanupStack::Pop(cpPriv); } @@ -340,9 +335,9 @@ void SymbianEngine::updateConfigurationsL() QNetworkConfigurationPrivatePointer ptr(cpPriv); accessPointConfigurations.insert(ptr->id, ptr); - locker.unlock(); + mutex.unlock(); emit configurationAdded(ptr); - locker.relock(); + mutex.lock(); QMutexLocker configLocker(&privSNAP->mutex); privSNAP->serviceNetworkMembers.append(ptr); @@ -411,9 +406,9 @@ void SymbianEngine::updateConfigurationsL() //remove non existing IAP QNetworkConfigurationPrivatePointer ptr = accessPointConfigurations.take(oldIface); - locker.unlock(); + mutex.unlock(); emit configurationRemoved(ptr); - locker.relock(); + mutex.lock(); // Remove non existing IAP from SNAPs foreach (const QString &iface, snapConfigurations.keys()) { @@ -434,18 +429,21 @@ void SymbianEngine::updateConfigurationsL() //remove non existing SNAPs QNetworkConfigurationPrivatePointer ptr = snapConfigurations.take(oldIface); - locker.unlock(); + mutex.unlock(); emit configurationRemoved(ptr); - locker.relock(); + mutex.lock(); } + + // find default configuration. + stopCommsDatabaseNotifications(); + TRAP_IGNORE(defaultConfig = defaultConfigurationL()); + startCommsDatabaseNotifications(); } #ifdef SNAP_FUNCTIONALITY_AVAILABLE SymbianNetworkConfigurationPrivate *SymbianEngine::configFromConnectionMethodL( RCmConnectionMethod& connectionMethod) { - QMutexLocker locker(&mutex); - SymbianNetworkConfigurationPrivate *cpPriv = new SymbianNetworkConfigurationPrivate; CleanupStack::PushL(cpPriv); @@ -528,8 +526,6 @@ SymbianNetworkConfigurationPrivate *SymbianEngine::configFromConnectionMethodL( bool SymbianEngine::readNetworkConfigurationValuesFromCommsDb( TUint32 aApId, SymbianNetworkConfigurationPrivate *apNetworkConfiguration) { - QMutexLocker locker(&mutex); - TRAPD(error, readNetworkConfigurationValuesFromCommsDbL(aApId,apNetworkConfiguration)); if (error != KErrNone) { return false; @@ -540,8 +536,6 @@ bool SymbianEngine::readNetworkConfigurationValuesFromCommsDb( void SymbianEngine::readNetworkConfigurationValuesFromCommsDbL( TUint32 aApId, SymbianNetworkConfigurationPrivate *apNetworkConfiguration) { - QMutexLocker locker(&mutex); - CApDataHandler* pDataHandler = CApDataHandler::NewLC(*ipCommsDB); CApAccessPointItem* pAPItem = CApAccessPointItem::NewLC(); TBuf name; @@ -604,15 +598,7 @@ QNetworkConfigurationPrivatePointer SymbianEngine::defaultConfiguration() { QMutexLocker locker(&mutex); - QNetworkConfigurationPrivatePointer ptr; - - if (iInitOk) { - stopCommsDatabaseNotifications(); - TRAP_IGNORE(ptr = defaultConfigurationL()); - startCommsDatabaseNotifications(); - } - - return ptr; + return defaultConfig; } QStringList SymbianEngine::accessPointConfigurationIdentifiers() @@ -624,8 +610,6 @@ QStringList SymbianEngine::accessPointConfigurationIdentifiers() QNetworkConfigurationPrivatePointer SymbianEngine::defaultConfigurationL() { - QMutexLocker locker(&mutex); - QNetworkConfigurationPrivatePointer ptr; #ifdef SNAP_FUNCTIONALITY_AVAILABLE @@ -653,8 +637,6 @@ QNetworkConfigurationPrivatePointer SymbianEngine::defaultConfigurationL() void SymbianEngine::updateActiveAccessPoints() { - QMutexLocker locker(&mutex); - bool online = false; QList inactiveConfigs = accessPointConfigurations.keys(); @@ -707,16 +689,14 @@ void SymbianEngine::updateActiveAccessPoints() if (iOnline != online) { iOnline = online; - locker.unlock(); + mutex.unlock(); emit this->onlineStateChanged(iOnline); - locker.relock(); + mutex.lock(); } } void SymbianEngine::updateAvailableAccessPoints() { - QMutexLocker locker(&mutex); - if (!ipAccessPointsAvailabilityScanner) { ipAccessPointsAvailabilityScanner = new AccessPointsAvailabilityScanner(*this, iConnectionMonitor); } @@ -728,8 +708,6 @@ void SymbianEngine::updateAvailableAccessPoints() void SymbianEngine::accessPointScanningReady(TBool scanSuccessful, TConnMonIapInfo iapInfo) { - QMutexLocker locker(&mutex); - iUpdateGoingOn = false; if (scanSuccessful) { QList unavailableConfigs = accessPointConfigurations.keys(); @@ -764,16 +742,14 @@ void SymbianEngine::accessPointScanningReady(TBool scanSuccessful, TConnMonIapIn if (!iFirstUpdate) { startCommsDatabaseNotifications(); - locker.unlock(); + mutex.unlock(); emit updateCompleted(); - locker.relock(); + mutex.lock(); } } void SymbianEngine::updateStatesToSnaps() { - QMutexLocker locker(&mutex); - // Go through SNAPs and set correct state to SNAPs foreach (const QString &iface, snapConfigurations.keys()) { bool discovered = false; @@ -810,16 +786,14 @@ void SymbianEngine::updateStatesToSnaps() bool SymbianEngine::changeConfigurationStateTo(QNetworkConfigurationPrivatePointer ptr, QNetworkConfiguration::StateFlags newState) { - QMutexLocker locker(&mutex); - ptr->mutex.lock(); if (newState != ptr->state) { ptr->state = newState; ptr->mutex.unlock(); - locker.unlock(); + mutex.unlock(); emit configurationChanged(ptr); - locker.relock(); + mutex.lock(); return true; } else { @@ -835,16 +809,14 @@ bool SymbianEngine::changeConfigurationStateTo(QNetworkConfigurationPrivatePoint bool SymbianEngine::changeConfigurationStateAtMinTo(QNetworkConfigurationPrivatePointer ptr, QNetworkConfiguration::StateFlags newState) { - QMutexLocker locker(&mutex); - ptr->mutex.lock(); if ((newState | ptr->state) != ptr->state) { ptr->state = (ptr->state | newState); ptr->mutex.unlock(); - locker.unlock(); + mutex.unlock(); emit configurationChanged(ptr); - locker.relock(); + mutex.lock(); return true; } else { @@ -861,16 +833,14 @@ bool SymbianEngine::changeConfigurationStateAtMinTo(QNetworkConfigurationPrivate bool SymbianEngine::changeConfigurationStateAtMaxTo(QNetworkConfigurationPrivatePointer ptr, QNetworkConfiguration::StateFlags newState) { - QMutexLocker locker(&mutex); - ptr->mutex.lock(); if ((newState & ptr->state) != ptr->state) { ptr->state = (newState & ptr->state); ptr->mutex.unlock(); - locker.unlock(); + mutex.unlock(); emit configurationChanged(ptr); - locker.relock(); + mutex.lock(); return true; } else { @@ -881,8 +851,6 @@ bool SymbianEngine::changeConfigurationStateAtMaxTo(QNetworkConfigurationPrivate void SymbianEngine::startCommsDatabaseNotifications() { - QMutexLocker locker(&mutex); - if (!iWaitingCommsDatabaseNotifications) { iWaitingCommsDatabaseNotifications = ETrue; if (!IsActive()) { @@ -895,8 +863,6 @@ void SymbianEngine::startCommsDatabaseNotifications() void SymbianEngine::stopCommsDatabaseNotifications() { - QMutexLocker locker(&mutex); - if (iWaitingCommsDatabaseNotifications) { iWaitingCommsDatabaseNotifications = EFalse; Cancel(); @@ -938,9 +904,13 @@ void SymbianEngine::RunL() if (error == KErrNone) { updateStatesToSnaps(); } + locker.unlock(); waitRandomTime(); + locker.relock(); } else { + locker.unlock(); waitRandomTime(); + locker.relock(); TRAPD(error, updateConfigurationsL()); if (error == KErrNone) { updateStatesToSnaps(); @@ -1129,6 +1099,8 @@ void SymbianEngine::EventL(const CConnMonEventBase& aEvent) // manager). Currently only 'Disconnected' state is of interest because it has proven to be troublesome. void SymbianEngine::configurationStateChangeReport(TUint32 accessPointId, QNetworkSession::State newState) { + QMutexLocker locker(&mutex); + #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug() << "QNCM A session reported state change for IAP ID: " << accessPointId << " whose new state is: " << newState; #endif @@ -1143,9 +1115,11 @@ void SymbianEngine::configurationStateChangeReport(TUint32 accessPointId, QNetwo updateStatesToSnaps(); } + locker.unlock(); emit configurationStateChanged(toSymbianConfig(ptr)->numericIdentifier(), toSymbianConfig(ptr)->connectionIdentifier(), QNetworkSession::Disconnected); + locker.relock(); } } break; @@ -1157,21 +1131,17 @@ void SymbianEngine::configurationStateChangeReport(TUint32 accessPointId, QNetwo // Waits for 2..6 seconds. void SymbianEngine::waitRandomTime() { - iTimeToWait = (qAbs(qrand()) % 7) * 1000; - if (iTimeToWait < 2000) { - iTimeToWait = 2000; - } + int iTimeToWait = qMax(2000, (qAbs(qrand()) % 7) * 1000); #ifdef QT_BEARERMGMT_SYMBIAN_DEBUG qDebug("QNCM waiting random time: %d ms", iTimeToWait); #endif - QTimer::singleShot(iTimeToWait, iIgnoreEventLoop, SLOT(quit())); - iIgnoreEventLoop->exec(); + QEventLoop loop; + QTimer::singleShot(iTimeToWait, &loop, SLOT(quit())); + loop.exec(); } QNetworkConfigurationPrivatePointer SymbianEngine::dataByConnectionId(TUint aConnectionId) { - QMutexLocker locker(&mutex); - QNetworkConfiguration item; QHash::const_iterator i = accessPointConfigurations.constBegin(); @@ -1224,6 +1194,8 @@ void AccessPointsAvailabilityScanner::StartScanning() void AccessPointsAvailabilityScanner::RunL() { + QMutexLocker locker(&iOwner.mutex); + if (iStatus.Int() != KErrNone) { iIapBuf().iCount = 0; iOwner.accessPointScanningReady(false,iIapBuf()); diff --git a/src/plugins/bearer/symbian/symbianengine.h b/src/plugins/bearer/symbian/symbianengine.h index 841d79d..18fd249 100644 --- a/src/plugins/bearer/symbian/symbianengine.h +++ b/src/plugins/bearer/symbian/symbianengine.h @@ -212,11 +212,11 @@ private: // Data TBool iInitOk; TBool iUpdateGoingOn; TBool iIgnoringUpdates; - TUint iTimeToWait; - QEventLoop* iIgnoreEventLoop; AccessPointsAvailabilityScanner* ipAccessPointsAvailabilityScanner; - + + QNetworkConfigurationPrivatePointer defaultConfig; + friend class QNetworkSessionPrivate; friend class AccessPointsAvailabilityScanner; friend class QNetworkSessionPrivateImpl; -- cgit v0.12