shill: Cellular: Fix Create/DestroyService(s)
This moves all creation and destruction of CellularService
instances to Cellular::CreateServices or DestroyServices
and updates CellularServiceProvider accordingly.
This also changes CellularServiceProvider::RemoveServices
to actually remove the services to address the associated bug.
BUG=b:181680557
TEST=unit tests and existing tast/tauto tests pass
Change-Id: I5a49b4f01b54a693aa37862c4416f6588f9dcea8
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2737805
Tested-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
(cherry picked from commit 4c19c92943e90bb2e525de02248f0fb7985ec16d)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2830419
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
diff --git a/shill/cellular/cellular.cc b/shill/cellular/cellular.cc
index 86ef3bd..f3c9e9e 100644
--- a/shill/cellular/cellular.cc
+++ b/shill/cellular/cellular.cc
@@ -550,10 +550,9 @@
const Error& error) {
SLOG(this, 1) << __func__ << ": " << GetStateString(state_);
SetCapabilityState(CapabilityState::kCellularStopped);
- // Destroy the cellular service regardless of any errors that occur during
- // the stop process since we do not know the state of the modem at this
- // point.
- DestroyService();
+ // Destroy any cellular services regardless of any errors that occur during
+ // the stop process since we do not know the state of the modem at this point.
+ DestroyServices();
if (state_ != kStateDisabled)
SetState(kStateDisabled);
callback.Run(error);
@@ -866,38 +865,41 @@
// registered means we've successfully connected
StartLocationPolling();
}
- UpdateService();
+ UpdateServices();
}
-void Cellular::UpdateService() {
+void Cellular::UpdateServices() {
+ SLOG(this, 2) << __func__;
// If Cellular is disabled or does not have an associated SIM+ICCID, ensure
// that it does not have an associated Service.
if (state_ == kStateDisabled || iccid_.empty()) {
if (service_)
- DestroyService();
+ DestroyServices();
return;
}
// If the associated Service does not have a matching ICCID, destroy it.
if (service_ && service_->iccid() != iccid_)
- DestroyService();
+ DestroyServices();
- // Ensure that a Service matching the Device SIM Profile exists.
- if (!service_)
- CreateService();
+ // Ensure that a Service matching the Device SIM Profile exists and has its
+ // |connectable_| property set correctly.
+ if (!service_) {
+ CreateServices();
+ } else {
+ service_->SetDevice(this);
+ }
if (state_ == kStateRegistered && modem_state_ == kModemStateConnected)
OnConnected();
- if (!service_->cellular())
- service_->SetDevice(this);
service_->SetNetworkTechnology(capability_->GetNetworkTechnologyString());
service_->SetRoamingState(capability_->GetRoamingStateString());
manager()->UpdateService(service_);
}
-void Cellular::CreateService() {
+void Cellular::CreateServices() {
if (service_for_testing_)
return;
@@ -909,8 +911,19 @@
CHECK(capability_);
DCHECK(manager()->cellular_service_provider());
+
+ // Create or update Cellular Services for the primary SIM.
service_ =
manager()->cellular_service_provider()->LoadServicesForDevice(this);
+
+ // Create or update Cellular Services for secondary SIMs.
+ for (const SimProperties& sim_properties : sim_slot_properties_) {
+ if (sim_properties.iccid.empty() || sim_properties.iccid == iccid_)
+ continue;
+ manager()->cellular_service_provider()->LoadServicesForSecondarySim(
+ sim_properties.eid, sim_properties.iccid, sim_properties.imsi);
+ }
+
capability_->OnServiceCreated();
// We might have missed a property update because the service wasn't created
@@ -919,17 +932,18 @@
OnOperatorChanged();
}
-void Cellular::DestroyService() {
+void Cellular::DestroyServices() {
if (service_for_testing_)
return;
SLOG(this, 2) << __func__;
DropConnection();
- if (service_) {
- DCHECK(manager()->cellular_service_provider());
- manager()->cellular_service_provider()->RemoveServicesForDevice(this);
- service_ = nullptr;
- }
+ if (!service_)
+ return;
+
+ DCHECK(manager()->cellular_service_provider());
+ manager()->cellular_service_provider()->RemoveServices();
+ service_ = nullptr;
}
void Cellular::CreateCapability(ModemInfo* modem_info) {
@@ -1347,10 +1361,10 @@
use_attach_apn_ = value;
if (capability_) {
- // Re-creating the service will set again the attach APN
- // and eventually re-attach if needed.
- DestroyService();
- CreateService();
+ // Re-creating services will set the attach APN again and eventually
+ // re-attach if needed.
+ DestroyServices();
+ CreateServices();
}
adaptor()->EmitBoolChanged(kUseAttachAPNProperty, value);
@@ -1788,12 +1802,13 @@
serving_operator_info()->UpdateIMSI(imsi_);
// Ensure Service creation once SIM properties are set.
- UpdateService();
+ UpdateServices();
}
void Cellular::SetSimSlotProperties(
const std::vector<SimProperties>& slot_properties) {
SLOG(this, 1) << __func__;
+ sim_slot_properties_ = slot_properties;
// Set |sim_slot_info_| and emit SIMSlotInfo
sim_slot_info_.clear();
for (const SimProperties& sim_properties : slot_properties) {
@@ -1805,22 +1820,6 @@
sim_slot_info_.push_back(properties);
}
adaptor()->EmitKeyValueStoresChanged(kSIMSlotInfoProperty, sim_slot_info_);
-
- // Update Cellular Services for secondary SIMs.
- for (const SimProperties& sim_properties : slot_properties) {
- if (sim_properties.iccid.empty()) {
- LOG(WARNING) << "SIM slot with no profile, eID: " << sim_properties.eid;
- continue;
- }
- // Skip the primary SIM
- if (sim_properties.iccid == iccid_)
- continue;
-
- // TODO(b:169581681): Remove logging once stable.
- LOG(INFO) << "Secondary SIM: " << sim_properties.iccid;
- manager()->cellular_service_provider()->LoadServicesForSecondarySim(
- sim_properties.eid, sim_properties.iccid, sim_properties.imsi);
- }
}
void Cellular::set_mdn(const string& mdn) {
diff --git a/shill/cellular/cellular.h b/shill/cellular/cellular.h
index 94b7d33..de76b0f 100644
--- a/shill/cellular/cellular.h
+++ b/shill/cellular/cellular.h
@@ -475,17 +475,17 @@
void InitCapability(Type type);
- // Called when |state_| becomes enabled or SetSimProperties are set.
- // Creates or destroys |service_| as required.
- void UpdateService();
+ // Creates or destroys services as required.
+ void UpdateServices();
- // Creates and registers the default CellularService for the Device.
- void CreateService();
+ // Creates and registers services for the available SIMs and sets
+ // |service_| to the primary (active) service.
+ void CreateServices();
- // Deregisters and destructs the current service and destroys the connection,
- // if any. This also eliminates the circular references between this device
- // and the associated service, allowing eventual device destruction.
- void DestroyService();
+ // Destroys all services and the connection, if any. This also eliminates any
+ // circular references between this device and the associated service,
+ // allowing eventual device destruction.
+ void DestroyServices();
// HelpRegisterDerived*: Expose a property over RPC, with the name |name|.
//
@@ -611,6 +611,8 @@
std::string imsi_;
bool sim_present_;
+ // vector of SimProperties, ordered by slot.
+ std::vector<SimProperties> sim_slot_properties_;
// vector of KeyValueStore dictionaries, emitted as Device.SIMSlotInfo.
KeyValueStores sim_slot_info_;
diff --git a/shill/cellular/cellular_service_provider.cc b/shill/cellular/cellular_service_provider.cc
index 7b0b5c4..f7f0b25 100644
--- a/shill/cellular/cellular_service_provider.cc
+++ b/shill/cellular/cellular_service_provider.cc
@@ -151,6 +151,8 @@
CellularServiceRefPtr CellularServiceProvider::LoadServicesForDevice(
Cellular* device) {
+ SLOG(this, 2) << __func__ << ": " << device->iccid();
+
CellularServiceRefPtr active_service =
LoadMatchingServicesFromProfile(device->GetSimCardId(), device->iccid(),
device->imsi(), device->eid(), device);
@@ -166,18 +168,12 @@
// LoadServicesForSecondarySim, after this is called.
std::vector<CellularServiceRefPtr> services_to_remove;
for (CellularServiceRefPtr& service : services_) {
- if (service->cellular() != device)
+ if (service->sim_card_id() != device->GetSimCardId())
services_to_remove.push_back(service);
}
for (CellularServiceRefPtr& service : services_to_remove)
RemoveService(service);
- // Set Device=null for services not matching |iccid|.
- for (CellularServiceRefPtr& service : services_) {
- if (service->iccid() != device->iccid())
- service->SetDevice(nullptr);
- }
-
return active_service;
}
@@ -215,13 +211,11 @@
service = new CellularService(manager_, service_imsi, service_iccid,
sim_card_id);
service->Load(storage);
- service->SetDevice(device);
- if (!device)
- service->set_eid(eid);
+ SetDeviceForService(service, device, eid);
AddService(service);
} else {
SLOG(this, 2) << "Cellular service exists for ICCID: " << service_iccid;
- service->SetDevice(device);
+ SetDeviceForService(service, device, eid);
}
if (service_iccid == iccid)
active_service = service;
@@ -230,7 +224,7 @@
if (!active_service) {
SLOG(this, 1) << "No existing Cellular service with ICCID: " << iccid;
active_service = new CellularService(manager_, imsi, iccid, sim_card_id);
- active_service->SetDevice(device);
+ SetDeviceForService(active_service, device, eid);
AddService(active_service);
}
return active_service;
@@ -246,18 +240,10 @@
/*device=*/nullptr);
}
-void CellularServiceProvider::RemoveServicesForDevice(Cellular* device) {
- std::string sim_card_id = device->GetSimCardId();
- LOG(INFO) << __func__ << ": " << sim_card_id;
- // Set |device| to null for services associated with |device|. When a new
- // Cellular device is created (e.g. after a modem resets after a sim swap),
- // services not matching the new device will be removed in
- // LoadServicesForDevice(). This allows services to continue to exist during a
- // modem reset when Modem and Cellular may get temporarily destroyed.
- for (CellularServiceRefPtr& service : services_) {
- if (service->cellular() == device)
- service->SetDevice(nullptr);
- }
+void CellularServiceProvider::RemoveServices() {
+ SLOG(this, 1) << __func__;
+ while (!services_.empty())
+ RemoveService(services_.back());
}
void CellularServiceProvider::AddService(CellularServiceRefPtr service) {
@@ -282,6 +268,17 @@
services_.erase(iter);
}
+void CellularServiceProvider::SetDeviceForService(CellularServiceRefPtr service,
+ Cellular* device,
+ const std::string& eid) {
+ if (device && service->iccid() == device->iccid()) {
+ service->SetDevice(device);
+ } else {
+ service->SetDevice(nullptr);
+ service->set_eid(eid);
+ }
+}
+
CellularServiceRefPtr CellularServiceProvider::FindService(
const std::string& iccid) {
const auto iter = std::find_if(
diff --git a/shill/cellular/cellular_service_provider.h b/shill/cellular/cellular_service_provider.h
index 98e6431..30b67e4 100644
--- a/shill/cellular/cellular_service_provider.h
+++ b/shill/cellular/cellular_service_provider.h
@@ -51,8 +51,8 @@
const std::string& iccid,
const std::string& imsi);
- // Removes any services associated with |device|.
- void RemoveServicesForDevice(Cellular* device);
+ // Removes all services.
+ void RemoveServices();
void set_profile_for_testing(ProfileRefPtr profile) { profile_ = profile; }
@@ -67,6 +67,9 @@
Cellular* device);
void AddService(CellularServiceRefPtr service);
void RemoveService(CellularServiceRefPtr service);
+ void SetDeviceForService(CellularServiceRefPtr service,
+ Cellular* device,
+ const std::string& eid);
CellularServiceRefPtr FindService(const std::string& imsi);
Manager* manager_;
diff --git a/shill/cellular/cellular_service_provider_test.cc b/shill/cellular/cellular_service_provider_test.cc
index eb91635..f08a54f 100644
--- a/shill/cellular/cellular_service_provider_test.cc
+++ b/shill/cellular/cellular_service_provider_test.cc
@@ -103,7 +103,7 @@
EventDispatcherForTest dispatcher_;
NiceMock<MockControl> control_;
NiceMock<MockMetrics> metrics_;
- MockManager manager_;
+ NiceMock<MockManager> manager_;
MockModemInfo modem_info_;
NiceMock<MockDeviceInfo> device_info_;
FakeStore storage_;
@@ -123,18 +123,22 @@
EXPECT_TRUE(service->IsVisible());
EXPECT_TRUE(service->connectable());
- // RemoveServicesForDevice does not destroy the services, but they should no
- // longer be marked as connectable.
- provider()->RemoveServicesForDevice(device.get());
- EXPECT_EQ(1u, GetProviderServices().size());
- EXPECT_TRUE(service->IsVisible());
- EXPECT_FALSE(service->connectable());
-
// Stopping should remove all services.
provider()->Stop();
EXPECT_EQ(0u, GetProviderServices().size());
}
+TEST_F(CellularServiceProviderTest, RemoveServices) {
+ CellularRefPtr device = CreateDevice("imsi1", "iccid1");
+ CellularServiceRefPtr service =
+ provider()->LoadServicesForDevice(device.get());
+ ASSERT_TRUE(service);
+ EXPECT_EQ(1u, GetProviderServices().size());
+
+ provider()->RemoveServices();
+ EXPECT_EQ(0u, GetProviderServices().size());
+}
+
TEST_F(CellularServiceProviderTest, LoadServiceFromProfile) {
CellularRefPtr device = CreateDevice("imsi1", "iccid1");
std::string identifier = device->GetStorageIdentifier();
@@ -193,13 +197,6 @@
EXPECT_EQ(1u, GetProviderServices().size());
unsigned int serial_number1 = service->serial_number();
- // Removing services for the device does not destroy the services, but they
- // should no longer be marked as connectable.
- provider()->RemoveServicesForDevice(device.get());
- EXPECT_EQ(1u, GetProviderServices().size());
- EXPECT_TRUE(service->IsVisible());
- EXPECT_FALSE(service->connectable());
-
// Adding a device with a new ICCID should create a new service with a
// different serial number.
device = CreateDevice("imsi2", "iccid2");
diff --git a/shill/cellular/cellular_test.cc b/shill/cellular/cellular_test.cc
index 131d507..2dbe466 100644
--- a/shill/cellular/cellular_test.cc
+++ b/shill/cellular/cellular_test.cc
@@ -785,14 +785,14 @@
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
// Compare substrings explicitly using EXPECT_EQ for better error message.
size_t prefix_len = strlen(Cellular::kGenericServiceNamePrefix);
EXPECT_EQ(Cellular::kGenericServiceNamePrefix,
device_->service_->friendly_name().substr(0, prefix_len));
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (2) Service created, then home provider determined => Name provided by
// home provider.
@@ -800,7 +800,7 @@
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_home_provider_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
// Now emulate an event for updated home provider information.
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
EXPECT_CALL(*mock_home_provider_info_, IsMobileNetworkOperatorKnown())
@@ -811,7 +811,7 @@
EXPECT_EQ(kHomeProviderName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (3) Service created, then serving operator determined => Name provided by
// serving operator.
@@ -819,7 +819,7 @@
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
// Now emulate an event for updated serving operator information.
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
@@ -830,7 +830,7 @@
EXPECT_EQ(kServingOperatorName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (4) Service created, then home provider determined, then serving operator
// determined => final name is serving operator.
@@ -838,7 +838,7 @@
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
// Now emulate an event for updated home provider information.
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
EXPECT_CALL(*mock_home_provider_info_, IsMobileNetworkOperatorKnown())
@@ -856,7 +856,7 @@
EXPECT_EQ(kServingOperatorName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (5) Service created, then serving operator determined, then home provider
// determined => final name is serving operator.
@@ -864,7 +864,7 @@
.WillRepeatedly(Return(false));
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
// Now emulate an event for updated serving operator information.
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
@@ -882,7 +882,7 @@
EXPECT_EQ(kServingOperatorName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (6) Serving operator known, home provider known, and then service created
// => Name is serving operator.
@@ -894,11 +894,11 @@
.WillRepeatedly(ReturnRef(kHomeProviderName));
EXPECT_CALL(*mock_serving_operator_info_, operator_name())
.WillRepeatedly(ReturnRef(kServingOperatorName));
- device_->CreateService();
+ device_->CreateServices();
EXPECT_EQ(kServingOperatorName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (7) Serving operator known, home provider known, and roaming state is set
// => Name is the form of "home provider | serving operator".
@@ -910,14 +910,14 @@
.WillRepeatedly(ReturnRef(kHomeProviderName));
EXPECT_CALL(*mock_serving_operator_info_, operator_name())
.WillRepeatedly(ReturnRef(kServingOperatorName));
- device_->CreateService();
+ device_->CreateServices();
device_->service_->roaming_state_ = kRoamingStateRoaming;
device_->OnOperatorChanged();
EXPECT_EQ(kHomeProviderName + " | " + kServingOperatorName,
device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (8) Like (7) but home provider and serving operator have the same name
// => Only one name is shown.
@@ -929,13 +929,13 @@
.WillRepeatedly(ReturnRef(kHomeProviderName));
EXPECT_CALL(*mock_serving_operator_info_, operator_name())
.WillRepeatedly(ReturnRef(kHomeProviderName));
- device_->CreateService();
+ device_->CreateServices();
device_->service_->roaming_state_ = kRoamingStateRoaming;
device_->OnOperatorChanged();
EXPECT_EQ(kHomeProviderName, device_->service_->friendly_name());
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
}
#endif // !defined(DISABLE_CELLULAR_CAPABILITY_CLASSIC_TESTS)
@@ -956,7 +956,7 @@
EXPECT_CALL(*mock_serving_operator_info_, IsMobileNetworkOperatorKnown())
.WillRepeatedly(Return(false));
- device_->CreateService();
+ device_->CreateServices();
home_provider = device_->home_provider();
VerifyOperatorMap(home_provider, "", "", "");
@@ -964,7 +964,7 @@
VerifyOperatorMap(serving_operator, "", "", "");
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (2) serving operator known.
// When home provider is not known, serving operator proxies in.
@@ -979,7 +979,7 @@
EXPECT_CALL(*mock_serving_operator_info_, country())
.WillRepeatedly(ReturnRef(kServingOperatorCountry));
- device_->CreateService();
+ device_->CreateServices();
home_provider = device_->home_provider();
VerifyOperatorMap(home_provider, kServingOperatorCode, kServingOperatorName,
@@ -989,7 +989,7 @@
kServingOperatorName, kServingOperatorCountry);
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (3) home provider known.
// When serving operator is not known, home provider proxies in.
@@ -1004,7 +1004,7 @@
EXPECT_CALL(*mock_home_provider_info_, country())
.WillRepeatedly(ReturnRef(kHomeProviderCountry));
- device_->CreateService();
+ device_->CreateServices();
home_provider = device_->home_provider();
VerifyOperatorMap(home_provider, kHomeProviderCode, kHomeProviderName,
@@ -1014,7 +1014,7 @@
kHomeProviderCountry);
Mock::VerifyAndClearExpectations(mock_home_provider_info_);
Mock::VerifyAndClearExpectations(mock_serving_operator_info_);
- device_->DestroyService();
+ device_->DestroyServices();
// (4) Serving operator known, home provider known.
EXPECT_CALL(*mock_home_provider_info_, IsMobileNetworkOperatorKnown())
@@ -1034,7 +1034,7 @@
EXPECT_CALL(*mock_serving_operator_info_, country())
.WillRepeatedly(ReturnRef(kServingOperatorCountry));
- device_->CreateService();
+ device_->CreateServices();
home_provider = device_->home_provider();
VerifyOperatorMap(home_provider, kHomeProviderCode, kHomeProviderName,
@@ -1097,9 +1097,9 @@
sim_properties.iccid = "test_iccid";
sim_properties.imsi = "test_imsi";
device_->SetPrimarySimProperties(sim_properties);
- device_->CreateService();
+ device_->CreateServices();
EXPECT_EQ("cellular_test_iccid", device_->service()->GetStorageIdentifier());
- device_->DestroyService();
+ device_->DestroyServices();
}
TEST_P(CellularTest, Connect) {
