shill: cellular: Move ownership of MobileOperatorInfo objects to Cellular.
Before this CL, |home_provider_info_| and |service_operator_info_| objects were
owned by ModemInfo. The aim was to allow |Cellular| objects who's lifetime does
not overlap to reuse these objects. But on a device with multiple modems, two
concomitant |Cellular| objects would interfere when they try to use the shared
MobileOperatorInfo objects.
This CL moves the ownership to |Cellular|. Preliminary experiments show that
both the space and time cost of creating MobileOperatorInfo objects is small
enough to all recreating them on modem reset. This is a stop-gap solution. The
more elegant solution is to split the MobileOperatorInfo object so that the
costly parts can continue to be owned by ModemInfo.
BUG=chromium:363874
TEST=(1) Run shill unittests.
(2) Run cellular_ServiceName and network_3GModemControl.
Change-Id: I85d0b5bc6f82b934c3bcec6c2fd5408a1d9b03ff
Reviewed-on: https://chromium-review.googlesource.com/197130
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
Commit-Queue: Prathmesh Prabhu <pprabhu@chromium.org>
Tested-by: Prathmesh Prabhu <pprabhu@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 8ea6133..0cf1693 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -119,8 +119,10 @@
weak_ptr_factory_(this),
state_(kStateDisabled),
modem_state_(kModemStateUnknown),
+ home_provider_info_(new MobileOperatorInfo(modem_info->dispatcher())),
+ serving_operator_info_(new MobileOperatorInfo(modem_info->dispatcher())),
mobile_operator_info_observer_(
- new Cellular::MobileOperatorInfoObserver(this, modem_info)),
+ new Cellular::MobileOperatorInfoObserver(this)),
dbus_owner_(owner),
dbus_service_(service),
dbus_path_(path),
@@ -139,16 +141,12 @@
is_ppp_authenticating_(false),
scanning_timeout_milliseconds_(kDefaultScanningTimeoutMilliseconds) {
RegisterProperties();
- // TODO(pprabhu): This will break when there are multiple modems on the
- // device. So, split MobileOperatorInfo into a context that contains no
- // device specific state, and one that does. The context should be owned by
- // the |modem_info_|, while the latter should be owned by the cellular device.
- modem_info_->home_provider_info()->Reset();
- modem_info_->serving_operator_info()->Reset();
- modem_info_->home_provider_info()->AddObserver(
- mobile_operator_info_observer_.get());
- modem_info_->serving_operator_info()->AddObserver(
- mobile_operator_info_observer_.get());
+ // TODO(pprabhu) Split MobileOperatorInfo into a context that stores the
+ // costly database, and lighter objects that |Cellular| can own.
+ home_provider_info_->Init();
+ serving_operator_info_->Init();
+ home_provider_info()->AddObserver(mobile_operator_info_observer_.get());
+ serving_operator_info()->AddObserver(mobile_operator_info_observer_.get());
InitCapability(type);
SLOG(Cellular, 2) << "Cellular device " << this->link_name()
<< " initialized.";
@@ -164,9 +162,8 @@
// may not have been removed.
manager()->RemoveTerminationAction(FriendlyName());
- modem_info_->home_provider_info()->RemoveObserver(
- mobile_operator_info_observer_.get());
- modem_info_->serving_operator_info()->RemoveObserver(
+ home_provider_info()->RemoveObserver(mobile_operator_info_observer_.get());
+ serving_operator_info()->RemoveObserver(
mobile_operator_info_observer_.get());
}
@@ -1421,6 +1418,15 @@
adaptor()->EmitUint16Changed(kPRLVersionProperty, prl_version_);
}
+void Cellular::set_home_provider_info(MobileOperatorInfo *home_provider_info) {
+ home_provider_info_.reset(home_provider_info);
+}
+
+void Cellular::set_serving_operator_info(
+ MobileOperatorInfo *serving_operator_info) {
+ serving_operator_info_.reset(serving_operator_info);
+}
+
void Cellular::UpdateHomeProvider(const MobileOperatorInfo *operator_info) {
SLOG(Cellular, 3) << __func__;
// TODO(pprabhu) Change |set_home_provider| to take Stringmap argument and
@@ -1520,10 +1526,8 @@
// /////////////////////////////////////////////////////////////////////////////
// MobileOperatorInfoObserver implementation.
Cellular::MobileOperatorInfoObserver::MobileOperatorInfoObserver(
- Cellular *cellular,
- ModemInfo *modem_info)
- : cellular_(cellular),
- modem_info_(modem_info) {}
+ Cellular *cellular)
+ : cellular_(cellular) {}
Cellular::MobileOperatorInfoObserver::~MobileOperatorInfoObserver() {}
@@ -1531,9 +1535,9 @@
SLOG(Cellular, 3) << __func__;
const MobileOperatorInfo *home_provider_info =
- modem_info_->home_provider_info();
+ cellular_->home_provider_info();
const MobileOperatorInfo *serving_operator_info =
- modem_info_->serving_operator_info();
+ cellular_->serving_operator_info();
const bool home_provider_known =
home_provider_info->IsMobileNetworkOperatorKnown();
diff --git a/cellular.h b/cellular.h
index ca30414..0c023ff 100644
--- a/cellular.h
+++ b/cellular.h
@@ -29,6 +29,7 @@
class CellularCapability;
class Error;
class ExternalTask;
+class MobileOperatorInfo;
class PPPDeviceFactory;
class ProxyFactory;
@@ -140,6 +141,12 @@
void CompleteActivation(Error *error);
const CellularServiceRefPtr &service() const { return service_; }
+ MobileOperatorInfo *home_provider_info() const {
+ return home_provider_info_.get();
+ }
+ MobileOperatorInfo *serving_operator_info() const {
+ return serving_operator_info_.get();
+ }
// Deregisters and destructs the current service and destroys the connection,
// if any. This also eliminates the circular references between this device
@@ -303,6 +310,11 @@
void set_supported_carriers(const Strings &supported_carriers);
void set_prl_version(uint16 prl_version);
+ // Takes ownership.
+ void set_home_provider_info(MobileOperatorInfo *home_provider_info);
+ // Takes ownership.
+ void set_serving_operator_info(MobileOperatorInfo *serving_operator_info);
+
private:
friend class ActivePassiveOutOfCreditsDetectorTest;
friend class CellularTest;
@@ -410,18 +422,14 @@
class MobileOperatorInfoObserver : public MobileOperatorInfo::Observer {
public:
- // Both |cellular| and |modem_info| must have lifespan longer than this
- // object.
- // In practice, this is enforced:
- // (1) |cellular| owns this object.
- // (2) |modem_info| is guaranteed to outlive |cellular|.
- MobileOperatorInfoObserver(Cellular *cellular, ModemInfo *modem_info);
+ // |cellular| must have lifespan longer than this object. In practice this
+ // is enforced because |cellular| owns this object.
+ MobileOperatorInfoObserver(Cellular *cellular);
virtual ~MobileOperatorInfoObserver();
virtual void OnOperatorChanged() override;
private:
Cellular *const cellular_;
- ModemInfo *const modem_info_;
DISALLOW_COPY_AND_ASSIGN(MobileOperatorInfoObserver);
};
@@ -504,6 +512,13 @@
scoped_ptr<CellularCapability> capability_;
+ // Operator info objects. These objects receive updates as we receive
+ // information about the network operators from the SIM or OTA. In turn, they
+ // send out updates through their observer interfaces whenever the identity of
+ // the network operator changes, or any other property of the operator
+ // changes.
+ scoped_ptr<MobileOperatorInfo> home_provider_info_;
+ scoped_ptr<MobileOperatorInfo> serving_operator_info_;
// Observer object to listen to updates from the operator info objects.
scoped_ptr<MobileOperatorInfoObserver> mobile_operator_info_observer_;
diff --git a/cellular_capability_cdma.cc b/cellular_capability_cdma.cc
index 25b8521..4a16f6e 100644
--- a/cellular_capability_cdma.cc
+++ b/cellular_capability_cdma.cc
@@ -414,9 +414,9 @@
DBusProperties::GetString(properties,
"payment_url_postdata",
&olp_post_data)) {
- modem_info()->serving_operator_info()->UpdateOnlinePortal(olp_url,
- olp_method,
- olp_post_data);
+ cellular()->serving_operator_info()->UpdateOnlinePortal(olp_url,
+ olp_method,
+ olp_post_data);
}
}
diff --git a/cellular_capability_classic.cc b/cellular_capability_classic.cc
index ffd249a..aece7d3 100644
--- a/cellular_capability_classic.cc
+++ b/cellular_capability_classic.cc
@@ -281,7 +281,7 @@
if (error.IsSuccess()) {
if (DBusProperties::GetString(props, "carrier", &prop_value)) {
cellular()->set_carrier(prop_value);
- modem_info()->home_provider_info()->UpdateOperatorName(prop_value);
+ cellular()->home_provider_info()->UpdateOperatorName(prop_value);
}
if (DBusProperties::GetString(props, "meid", &prop_value)) {
cellular()->set_meid(prop_value);
@@ -291,7 +291,7 @@
}
if (DBusProperties::GetString(props, kModemPropertyIMSI, &prop_value)) {
cellular()->set_imsi(prop_value);
- modem_info()->home_provider_info()->UpdateIMSI(prop_value);
+ cellular()->home_provider_info()->UpdateIMSI(prop_value);
}
if (DBusProperties::GetString(props, "esn", &prop_value)) {
cellular()->set_esn(prop_value);
diff --git a/cellular_capability_gsm.cc b/cellular_capability_gsm.cc
index 9466457..e9d09b2 100644
--- a/cellular_capability_gsm.cc
+++ b/cellular_capability_gsm.cc
@@ -354,7 +354,7 @@
callback);
card_proxy_->GetIMSI(&error, cb, kTimeoutDefault);
if (error.IsFailure()) {
- modem_info()->home_provider_info()->Reset();
+ cellular()->home_provider_info()->Reset();
callback.Run(error);
}
} else {
@@ -853,8 +853,8 @@
registration_state_ = status;
serving_operator_.SetCode(operator_code);
serving_operator_.SetName(operator_name);
- modem_info()->serving_operator_info()->UpdateMCCMNC(operator_code);
- modem_info()->serving_operator_info()->UpdateOperatorName(operator_name);
+ cellular()->serving_operator_info()->UpdateMCCMNC(operator_code);
+ cellular()->serving_operator_info()->UpdateOperatorName(operator_name);
UpdateOperatorInfo();
cellular()->HandleNewRegistrationState();
}
@@ -895,7 +895,7 @@
SLOG(Cellular, 2) << "IMSI: " << imsi;
cellular()->set_imsi(imsi);
cellular()->set_sim_present(true);
- modem_info()->home_provider_info()->UpdateIMSI(imsi);
+ cellular()->home_provider_info()->UpdateIMSI(imsi);
SetHomeProvider();
callback.Run(error);
} else if (!sim_lock_status_.lock_type.empty()) {
@@ -914,7 +914,7 @@
get_imsi_retry_delay_milliseconds_);
} else {
LOG(INFO) << "GetIMSI failed - " << error;
- modem_info()->home_provider_info()->Reset();
+ cellular()->home_provider_info()->Reset();
callback.Run(error);
}
}
@@ -926,7 +926,7 @@
if (error.IsSuccess()) {
SLOG(Cellular, 2) << "SPN: " << spn;
spn_ = spn;
- modem_info()->home_provider_info()->UpdateOperatorName(spn);
+ cellular()->home_provider_info()->UpdateOperatorName(spn);
SetHomeProvider();
} else {
SLOG(Cellular, 2) << "GetSPN failed - " << error;
diff --git a/cellular_capability_universal.cc b/cellular_capability_universal.cc
index 0741464..999b0a1 100644
--- a/cellular_capability_universal.cc
+++ b/cellular_capability_universal.cc
@@ -1616,7 +1616,7 @@
cellular()->set_sim_present(false);
OnSimIdentifierChanged("");
OnOperatorIdChanged("");
- modem_info()->home_provider_info()->Reset();
+ cellular()->home_provider_info()->Reset();
} else {
cellular()->set_sim_present(true);
scoped_ptr<DBusPropertiesProxyInterface> properties_proxy(
@@ -1882,8 +1882,8 @@
registration_state_ = updated_state;
serving_operator_.SetCode(updated_operator_code);
serving_operator_.SetName(updated_operator_name);
- modem_info()->serving_operator_info()->UpdateMCCMNC(updated_operator_code);
- modem_info()->serving_operator_info()->UpdateOperatorName(
+ cellular()->serving_operator_info()->UpdateMCCMNC(updated_operator_code);
+ cellular()->serving_operator_info()->UpdateOperatorName(
updated_operator_name);
// Update the carrier name for |serving_operator_|.
@@ -1972,19 +1972,19 @@
OnSpnChanged(value);
if (DBusProperties::GetString(props, MM_SIM_PROPERTY_IMSI, &value)) {
cellular()->set_imsi(value);
- modem_info()->home_provider_info()->UpdateIMSI(value);
+ cellular()->home_provider_info()->UpdateIMSI(value);
}
SetHomeProvider();
}
void CellularCapabilityUniversal::OnSpnChanged(const std::string &spn) {
spn_ = spn;
- modem_info()->home_provider_info()->UpdateOperatorName(spn);
+ cellular()->home_provider_info()->UpdateOperatorName(spn);
}
void CellularCapabilityUniversal::OnSimIdentifierChanged(const string &id) {
cellular()->set_sim_identifier(id);
- modem_info()->home_provider_info()->UpdateICCID(id);
+ cellular()->home_provider_info()->UpdateICCID(id);
UpdatePendingActivationState();
}
@@ -1992,7 +1992,7 @@
const string &operator_id) {
SLOG(Cellular, 2) << "Operator ID = '" << operator_id << "'";
operator_id_ = operator_id;
- modem_info()->home_provider_info()->UpdateMCCMNC(operator_id);
+ cellular()->home_provider_info()->UpdateMCCMNC(operator_id);
}
OutOfCreditsDetector::OOCType
diff --git a/cellular_capability_universal_cdma.cc b/cellular_capability_universal_cdma.cc
index 597b9a4..37a993e 100644
--- a/cellular_capability_universal_cdma.cc
+++ b/cellular_capability_universal_cdma.cc
@@ -622,8 +622,8 @@
cdma_evdo_registration_state_ = state_evdo;
sid_ = sid;
nid_ = nid;
- modem_info()->serving_operator_info()->UpdateSID(UintToString(sid));
- modem_info()->serving_operator_info()->UpdateNID(UintToString(nid));
+ cellular()->serving_operator_info()->UpdateSID(UintToString(sid));
+ cellular()->serving_operator_info()->UpdateNID(UintToString(nid));
UpdateOperatorInfo();
cellular()->HandleNewRegistrationState();
}
diff --git a/modem_info.cc b/modem_info.cc
index a5ed1de..29a418f 100644
--- a/modem_info.cc
+++ b/modem_info.cc
@@ -11,7 +11,6 @@
#include "shill/cellular_operator_info.h"
#include "shill/logging.h"
#include "shill/manager.h"
-#include "shill/mobile_operator_info.h"
#include "shill/modem_manager.h"
#include "shill/pending_activation_store.h"
@@ -49,9 +48,7 @@
manager_(manager),
glib_(glib),
provider_db_path_(kMobileProviderDBPath),
- provider_db_(NULL),
- home_provider_info_(new MobileOperatorInfo(dispatcher)),
- serving_operator_info_(new MobileOperatorInfo(dispatcher)) {}
+ provider_db_(NULL) {}
ModemInfo::~ModemInfo() {
Stop();
@@ -63,8 +60,6 @@
manager_->storage_path());
cellular_operator_info_.reset(new CellularOperatorInfo());
cellular_operator_info_->Load(FilePath(kCellularOperatorInfoPath));
- home_provider_info_->Init();
- serving_operator_info_->Init();
// TODO(petkov): Consider initializing the mobile provider database lazily
// only if a GSM modem needs to be registered.
@@ -102,15 +97,6 @@
cellular_operator_info_.reset(cellular_operator_info);
}
-void ModemInfo::set_home_provider_info(MobileOperatorInfo *home_provider_info) {
- home_provider_info_.reset(home_provider_info);
-}
-
-void ModemInfo::set_serving_operator_info(
- MobileOperatorInfo *serving_operator_info) {
- serving_operator_info_.reset(serving_operator_info);
-}
-
void ModemInfo::RegisterModemManager(ModemManager *manager) {
modem_managers_.push_back(manager); // Passes ownership.
manager->Start();
diff --git a/modem_info.h b/modem_info.h
index f344fc0..d6ad4a0 100644
--- a/modem_info.h
+++ b/modem_info.h
@@ -21,7 +21,6 @@
class GLib;
class Manager;
class Metrics;
-class MobileOperatorInfo;
class ModemManager;
class PendingActivationStore;
@@ -52,12 +51,6 @@
return cellular_operator_info_.get();
}
mobile_provider_db *provider_db() const { return provider_db_; }
- MobileOperatorInfo *home_provider_info() const {
- return home_provider_info_.get();
- }
- MobileOperatorInfo *serving_operator_info() const {
- return serving_operator_info_.get();
- }
protected:
// Write accessors for unit-tests.
@@ -83,10 +76,6 @@
void set_mobile_provider_db(mobile_provider_db *provider_db) {
provider_db_ = provider_db;
}
- // Takes ownership.
- void set_home_provider_info(MobileOperatorInfo *home_provider_info);
- // Takes ownership.
- void set_serving_operator_info(MobileOperatorInfo *serving_operator_info);
private:
friend class ModemInfoTest;
@@ -110,13 +99,6 @@
scoped_ptr<CellularOperatorInfo> cellular_operator_info_;
std::string provider_db_path_; // For testing.
mobile_provider_db *provider_db_; // Database instance owned by |this|.
- // Operator info objects. These objects receive updates as we receive
- // information about the network operators from the SIM or OTA. In turn, they
- // send out updates through their observer interfaces whenever the identity of
- // the network operator changes, or any other property of the operator
- // changes.
- scoped_ptr<MobileOperatorInfo> home_provider_info_;
- scoped_ptr<MobileOperatorInfo> serving_operator_info_;
DISALLOW_COPY_AND_ASSIGN(ModemInfo);
};