shill: cellular: Update UnlockRequired and UnlockRetries separately.
Before this CL, if shill received a PropertiesChanged signal from
ModemManager that contained a value for 'UnlockRetries' but not for
'UnlockRequired' and the SIM was locked at the time, it incorrectly
updated the value of the "Cellular.SIMLockStatus/1/LockType" property to
'""'. Chrome uses this value to determine that the SIM is not locked and
thus this caused the UI to behave incorrectly if the user entered an
incorrect PIN.
BUG=chromium:249550
TEST=(1) Build and run unit tests.
(2) On a device with an Icera or LTE modem that works with
ModemManager AND for which the plugin implements reporting the
'UnlockRetries' property:
- Without this CL: Entering an incorrect PIN when the SIM is
locked will cause the PIN dialog to display no error and
disappear and the modem will remain disabled. Attempting
to unlock the modem through the UI will not be possible
until shill is restarted.
- With this CL: Entering an incorrect PIN when the SIM is
locked will cause the PIN dialog to report that the PIN
was incorrect and will show the correct number of unlock
retries left.
Change-Id: Ib018d0424c22a8d48502c1c1112b14565fcf12d0
Reviewed-on: https://gerrit.chromium.org/gerrit/58606
Commit-Queue: Arman Uguray <armansito@chromium.org>
Reviewed-by: Arman Uguray <armansito@chromium.org>
Tested-by: Arman Uguray <armansito@chromium.org>
diff --git a/cellular_capability.h b/cellular_capability.h
index 7c02ff9..8a59c36 100644
--- a/cellular_capability.h
+++ b/cellular_capability.h
@@ -57,16 +57,6 @@
// capabilities.
class CellularCapability {
public:
- // SimLockStatus represents the fields in the Cellular.SIMLockStatus
- // DBUS property of the shill device.
- struct SimLockStatus {
- SimLockStatus() : enabled(false), retries_left(0) {}
-
- bool enabled;
- std::string lock_type;
- uint32 retries_left;
- };
-
static const int kTimeoutActivate;
static const int kTimeoutConnect;
static const int kTimeoutDefault;
diff --git a/cellular_capability_gsm.h b/cellular_capability_gsm.h
index be0ad51..6e92d52 100644
--- a/cellular_capability_gsm.h
+++ b/cellular_capability_gsm.h
@@ -125,6 +125,8 @@
FRIEND_TEST(CellularTest, StartGSMRegister);
FRIEND_TEST(ModemTest, CreateDeviceFromProperties);
+ // SimLockStatus represents the fields in the Cellular.SIMLockStatus
+ // DBUS property of the shill device.
struct SimLockStatus {
public:
SimLockStatus() : enabled(false), retries_left(0) {}
diff --git a/cellular_capability_universal.cc b/cellular_capability_universal.cc
index 1e1d959..6bb1860 100644
--- a/cellular_capability_universal.cc
+++ b/cellular_capability_universal.cc
@@ -189,8 +189,20 @@
KeyValueStore CellularCapabilityUniversal::SimLockStatusToProperty(
Error */*error*/) {
KeyValueStore status;
+ string lock_type;
+ switch (sim_lock_status_.lock_type) {
+ case MM_MODEM_LOCK_SIM_PIN:
+ lock_type = "sim-pin";
+ break;
+ case MM_MODEM_LOCK_SIM_PUK:
+ lock_type = "sim-puk";
+ break;
+ default:
+ lock_type = "";
+ break;
+ }
status.SetBool(flimflam::kSIMLockEnabledProperty, sim_lock_status_.enabled);
- status.SetString(flimflam::kSIMLockTypeProperty, sim_lock_status_.lock_type);
+ status.SetString(flimflam::kSIMLockTypeProperty, lock_type);
status.SetUint(flimflam::kSIMLockRetriesLeftProperty,
sim_lock_status_.retries_left);
return status;
@@ -1505,23 +1517,20 @@
// not needed: MM_MODEM_PROPERTY_EQUIPMENTIDENTIFIER
// Unlock required and SimLock
- bool locks_changed = false;
uint32_t unlock_required; // This is really of type MMModemLock
if (DBusProperties::GetUint32(properties,
MM_MODEM_PROPERTY_UNLOCKREQUIRED,
- &unlock_required)) {
- locks_changed = true;
- }
- LockRetryData lock_retries;
+ &unlock_required))
+ OnLockTypeChanged(static_cast<MMModemLock>(unlock_required));
+
+ // Unlock retries
DBusPropertiesMap::const_iterator it =
properties.find(MM_MODEM_PROPERTY_UNLOCKRETRIES);
if (it != properties.end()) {
- lock_retries = it->second.operator LockRetryData();
- locks_changed = true;
+ LockRetryData lock_retries = it->second.operator LockRetryData();
+ OnLockRetriesChanged(lock_retries);
}
- if (locks_changed)
- OnLockRetriesChanged(static_cast<MMModemLock>(unlock_required),
- lock_retries);
+
if (DBusProperties::GetUint32(properties,
MM_MODEM_PROPERTY_ACCESSTECHNOLOGIES,
&uint_value))
@@ -1700,20 +1709,10 @@
}
void CellularCapabilityUniversal::OnLockRetriesChanged(
- MMModemLock unlock_required,
const LockRetryData &lock_retries) {
- switch (unlock_required) {
- case MM_MODEM_LOCK_SIM_PIN:
- sim_lock_status_.lock_type = "sim-pin";
- break;
- case MM_MODEM_LOCK_SIM_PUK:
- sim_lock_status_.lock_type = "sim-puk";
- break;
- default:
- sim_lock_status_.lock_type = "";
- break;
- }
- LockRetryData::const_iterator it = lock_retries.find(unlock_required);
+ SLOG(Cellular, 2) << __func__;
+ LockRetryData::const_iterator it =
+ lock_retries.find(sim_lock_status_.lock_type);
if (it != lock_retries.end()) {
sim_lock_status_.retries_left = it->second;
} else {
@@ -1723,6 +1722,13 @@
OnSimLockStatusChanged();
}
+void CellularCapabilityUniversal::OnLockTypeChanged(
+ MMModemLock lock_type) {
+ SLOG(Cellular, 2) << __func__ << ": " << lock_type;
+ sim_lock_status_.lock_type = lock_type;
+ OnSimLockStatusChanged();
+}
+
void CellularCapabilityUniversal::OnSimLockStatusChanged() {
SLOG(Cellular, 2) << __func__;
cellular()->adaptor()->EmitKeyValueStoreChanged(
@@ -1731,7 +1737,8 @@
// If the SIM is currently unlocked, assume that we need to refresh
// carrier information, since a locked SIM prevents shill from obtaining
// the necessary data to establish a connection later (e.g. IMSI).
- if (!sim_path_.empty() && sim_lock_status_.lock_type.empty()) {
+ if (!sim_path_.empty() &&
+ sim_lock_status_.lock_type == MM_MODEM_LOCK_NONE) {
scoped_ptr<DBusPropertiesProxyInterface> properties_proxy(
proxy_factory()->CreateDBusPropertiesProxy(sim_path_,
cellular()->dbus_owner()));
diff --git a/cellular_capability_universal.h b/cellular_capability_universal.h
index 6be2b8d..2ce9c68 100644
--- a/cellular_capability_universal.h
+++ b/cellular_capability_universal.h
@@ -190,8 +190,11 @@
FRIEND_TEST(CellularCapabilityUniversalMainTest, IsValidSimPath);
FRIEND_TEST(CellularCapabilityUniversalMainTest, NormalizeMdn);
FRIEND_TEST(CellularCapabilityUniversalMainTest, OnListBearersReply);
+ FRIEND_TEST(CellularCapabilityUniversalMainTest, OnLockRetriesChanged);
+ FRIEND_TEST(CellularCapabilityUniversalMainTest, OnLockTypeChanged);
FRIEND_TEST(CellularCapabilityUniversalMainTest,
OnModemCurrentCapabilitiesChanged);
+ FRIEND_TEST(CellularCapabilityUniversalMainTest, OnSimLockPropertiesChanged);
FRIEND_TEST(CellularCapabilityUniversalMainTest, PropertiesChanged);
FRIEND_TEST(CellularCapabilityUniversalMainTest, Reset);
FRIEND_TEST(CellularCapabilityUniversalMainTest, Scan);
@@ -200,6 +203,7 @@
FRIEND_TEST(CellularCapabilityUniversalMainTest,
ShouldDetectOutOfCredit);
FRIEND_TEST(CellularCapabilityUniversalMainTest, SimLockStatusChanged);
+ FRIEND_TEST(CellularCapabilityUniversalMainTest, SimLockStatusToProperty);
FRIEND_TEST(CellularCapabilityUniversalMainTest, SimPathChanged);
FRIEND_TEST(CellularCapabilityUniversalMainTest, SimPropertiesChanged);
FRIEND_TEST(CellularCapabilityUniversalMainTest, StartModem);
@@ -228,6 +232,19 @@
HandleNewRegistrationStateForServiceRequiringActivation);
FRIEND_TEST(CellularTest, ModemStateChangeLostRegistration);
+ // SimLockStatus represents the fields in the Cellular.SIMLockStatus
+ // DBUS property of the shill device.
+ struct SimLockStatus {
+ public:
+ SimLockStatus() : enabled(false),
+ lock_type(MM_MODEM_LOCK_UNKNOWN),
+ retries_left(0) {}
+
+ bool enabled;
+ MMModemLock lock_type;
+ uint32 retries_left;
+ };
+
// Methods used in starting a modem
void EnableModem(Error *error, const ResultCallback &callback);
void Start_ModemAlreadyEnabled(const ResultCallback &callback);
@@ -307,8 +324,8 @@
void OnModemRevisionChanged(const std::string &revision);
void OnModemStateChanged(Cellular::ModemState state);
void OnAccessTechnologiesChanged(uint32 access_technologies);
- void OnLockRetriesChanged(MMModemLock unlock_required,
- const LockRetryData &lock_retries);
+ void OnLockRetriesChanged(const LockRetryData &lock_retries);
+ void OnLockTypeChanged(MMModemLock unlock_required);
void OnSimLockStatusChanged();
// Returns false if the MDN is empty or if the MDN consists of all 0s.
diff --git a/cellular_capability_universal_unittest.cc b/cellular_capability_universal_unittest.cc
index e60b1de..4259882 100644
--- a/cellular_capability_universal_unittest.cc
+++ b/cellular_capability_universal_unittest.cc
@@ -565,7 +565,7 @@
capability_->spn_ = "";
// SIM is locked.
- capability_->sim_lock_status_.lock_type = "sim-pin";
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PIN;
capability_->OnSimLockStatusChanged();
Mock::VerifyAndClearExpectations(modem_info_.mock_pending_activation_store());
@@ -582,7 +582,7 @@
GetActivationState(PendingActivationStore::kIdentifierICCID, _))
.Times(1);
- capability_->sim_lock_status_.lock_type = "";
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_NONE;
capability_->OnSimLockStatusChanged();
Mock::VerifyAndClearExpectations(modem_info_.mock_pending_activation_store());
@@ -2079,4 +2079,110 @@
EXPECT_TRUE(capability_->ShouldDetectOutOfCredit());
}
+TEST_F(CellularCapabilityUniversalMainTest, SimLockStatusToProperty) {
+ Error error;
+ KeyValueStore store = capability_->SimLockStatusToProperty(&error);
+ EXPECT_FALSE(store.GetBool(flimflam::kSIMLockEnabledProperty));
+ EXPECT_TRUE(store.GetString(flimflam::kSIMLockTypeProperty).empty());
+ EXPECT_EQ(0, store.GetUint(flimflam::kSIMLockRetriesLeftProperty));
+
+ capability_->sim_lock_status_.enabled = true;
+ capability_->sim_lock_status_.retries_left = 3;
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PIN;
+ store = capability_->SimLockStatusToProperty(&error);
+ EXPECT_TRUE(store.GetBool(flimflam::kSIMLockEnabledProperty));
+ EXPECT_EQ("sim-pin", store.GetString(flimflam::kSIMLockTypeProperty));
+ EXPECT_EQ(3, store.GetUint(flimflam::kSIMLockRetriesLeftProperty));
+
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PUK;
+ store = capability_->SimLockStatusToProperty(&error);
+ EXPECT_EQ("sim-puk", store.GetString(flimflam::kSIMLockTypeProperty));
+
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PIN2;
+ store = capability_->SimLockStatusToProperty(&error);
+ EXPECT_TRUE(store.GetString(flimflam::kSIMLockTypeProperty).empty());
+
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PUK2;
+ store = capability_->SimLockStatusToProperty(&error);
+ EXPECT_TRUE(store.GetString(flimflam::kSIMLockTypeProperty).empty());
+}
+
+TEST_F(CellularCapabilityUniversalMainTest, OnLockRetriesChanged) {
+ CellularCapabilityUniversal::LockRetryData data;
+ const uint32 kDefaultRetries = 999;
+
+ capability_->OnLockRetriesChanged(data);
+ EXPECT_EQ(kDefaultRetries, capability_->sim_lock_status_.retries_left);
+
+ data[MM_MODEM_LOCK_SIM_PIN] = 3;
+ capability_->OnLockRetriesChanged(data);
+ EXPECT_EQ(kDefaultRetries, capability_->sim_lock_status_.retries_left);
+
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PIN2;
+ capability_->OnLockRetriesChanged(data);
+ EXPECT_EQ(kDefaultRetries, capability_->sim_lock_status_.retries_left);
+
+ capability_->sim_lock_status_.lock_type = MM_MODEM_LOCK_SIM_PIN;
+ capability_->OnLockRetriesChanged(data);
+ EXPECT_EQ(3, capability_->sim_lock_status_.retries_left);
+}
+
+TEST_F(CellularCapabilityUniversalMainTest, OnLockTypeChanged) {
+ EXPECT_EQ(MM_MODEM_LOCK_UNKNOWN, capability_->sim_lock_status_.lock_type);
+
+ capability_->OnLockTypeChanged(MM_MODEM_LOCK_NONE);
+ EXPECT_EQ(MM_MODEM_LOCK_NONE, capability_->sim_lock_status_.lock_type);
+
+ capability_->OnLockTypeChanged(MM_MODEM_LOCK_SIM_PIN);
+ EXPECT_EQ(MM_MODEM_LOCK_SIM_PIN, capability_->sim_lock_status_.lock_type);
+
+ capability_->OnLockTypeChanged(MM_MODEM_LOCK_SIM_PUK);
+ EXPECT_EQ(MM_MODEM_LOCK_SIM_PUK, capability_->sim_lock_status_.lock_type);
+}
+
+TEST_F(CellularCapabilityUniversalMainTest, OnSimLockPropertiesChanged) {
+ EXPECT_EQ(MM_MODEM_LOCK_UNKNOWN, capability_->sim_lock_status_.lock_type);
+ EXPECT_EQ(0, capability_->sim_lock_status_.retries_left);
+
+ DBusPropertiesMap changed;
+ vector<string> invalidated;
+
+ capability_->OnModemPropertiesChanged(changed, invalidated);
+ EXPECT_EQ(MM_MODEM_LOCK_UNKNOWN, capability_->sim_lock_status_.lock_type);
+ EXPECT_EQ(0, capability_->sim_lock_status_.retries_left);
+
+ ::DBus::Variant variant;
+ ::DBus::MessageIter writer = variant.writer();
+
+ // Unlock retries changed, but the SIM wasn't locked.
+ CellularCapabilityUniversal::LockRetryData retry_data;
+ retry_data[MM_MODEM_LOCK_SIM_PIN] = 3;
+ writer << retry_data;
+ changed[MM_MODEM_PROPERTY_UNLOCKRETRIES] = variant;
+
+ capability_->OnModemPropertiesChanged(changed, invalidated);
+ EXPECT_EQ(MM_MODEM_LOCK_UNKNOWN, capability_->sim_lock_status_.lock_type);
+ EXPECT_EQ(999, capability_->sim_lock_status_.retries_left);
+
+ // Unlock retries changed and the SIM got locked.
+ variant.clear();
+ writer = variant.writer();
+ writer << static_cast<uint32>(MM_MODEM_LOCK_SIM_PIN);
+ changed[MM_MODEM_PROPERTY_UNLOCKREQUIRED] = variant;
+ capability_->OnModemPropertiesChanged(changed, invalidated);
+ EXPECT_EQ(MM_MODEM_LOCK_SIM_PIN, capability_->sim_lock_status_.lock_type);
+ EXPECT_EQ(3, capability_->sim_lock_status_.retries_left);
+
+ // Only unlock retries changed.
+ changed.erase(MM_MODEM_PROPERTY_UNLOCKREQUIRED);
+ retry_data[MM_MODEM_LOCK_SIM_PIN] = 2;
+ variant.clear();
+ writer = variant.writer();
+ writer << retry_data;
+ changed[MM_MODEM_PROPERTY_UNLOCKRETRIES] = variant;
+ capability_->OnModemPropertiesChanged(changed, invalidated);
+ EXPECT_EQ(MM_MODEM_LOCK_SIM_PIN, capability_->sim_lock_status_.lock_type);
+ EXPECT_EQ(2, capability_->sim_lock_status_.retries_left);
+}
+
} // namespace shill