shill: make setting a property to its current value a no-op
Before this change, setting the EAP authentication properties
on a WiFiService would cause the connection to be dropped.
The connection would drop even if the new values were the same
as the old.
With this change, the connection is only dropped if the new
values differ from the old.
Overview of changes:
- AccessorInterface: have property setters return a bool (rather
than void). Setters should return true to indicate the value was
changed, and false otherwise.
- PropertyAccessor and derived classes:
- Implement the new AccessorInterface.
- Add tests that we implement the new AccessorInterface.
- Custom property setters (various classes):
- Update existing custom setters to return true if the value was
changed, and false otherwise.
- Add tests that custom setters implement the desired behavior.
- DBusAdaptor
- Change SetProperty to propagate the return value of
PropertyStore's setters, rather than Error::IsSuccess().
- In combination with other changes, this means
DBusAdaptor::SetProperty now returns false if the new value is
the same as the old.
- This also suppresses some spurious change notifications from
IPConfig, Manager, and Profile objects.
- Add tests that DBusAdaptor::SetProperty implements the desired
behavior.
- PropertyStore
- Add a change callback. This optional argument to the ctor is
invoked if a setter or clearer modified its property. This
is so that classes don't have to depend on their RPC adaptors
to inform them of a change. (See changes in Service.)
- Have setters pass through the return value of the Accessor,
rather than returning Error::IsSuccess(). In
combination with other changes, this means that setters
now returns false if the new value is the same as the old.
- Add tests that PropertyStore invokes the change callback
appropriately.
- ClearPropertyNonexistent, SetPropertyNonexistent: no callback
- ClearProperty: callback
- SetProperty: callback if and only if property changed
- Service
- Register OnPropertyChanged with PropertyStore, instead of relying
on a callback from ServiceDBusAdaptor. Two reasons for the change:
1) The RPC adaptors should be as trivial as possible, and
2) We can't test code in the RPC adaptors.
3) If we can't test code in the RPC adaptors, go to 1.
- ServiceDBusAdaptor: remove OnPropertyChange callback in SetProperty.
See Service for the rationale.
- Update existing SetProperty tests (various classes)
We now use values that differ from the current value of the property.
This ensures that the setter returns true.
- WiFiServiceTest: add a case to test that EAP authentication property
changes caused cached credentials to be cleared appropriately. This is
redundant given some of the other tests. But given that this was the
original problem in the bug, it seems worth testing specifically.
- HACKING: add some guidelines for what to do when adding properties.
While there:
- Change some HelpRegister... functions to HelpRegisterConst...
- Update some tests to check error.is_set() before reading
error.name(). This avoids a stray pointer dereference.
- Add SetStringmapsProperty to PropertyStore. This is needed because
PropertyStoreTypedTest now tests setters.
- Remove duplicate kAutoConnectProperty test case in ServiceTest.SetProperty
- Remove unused local in WiFiServiceTest.SetPassphraseRemovedCachedCredentials
- Remove unused method Device::HelpRegisterDerivedStrings
- Remove KeyValueStore from the set of types exercised by
PropertyStoreTypedTest. We only use KeyValueStore for const
properties, and PropertyStoreTypedTest tests setting and
clearing.
- Add PropertyChanges test to EthernetEapServiceTest.
BUG=chromium:233681
TEST=new unit tests
Change-Id: I9bdd89fbe6f19101dfcd5f126f2ba9c81533ff97
Reviewed-on: https://gerrit.chromium.org/gerrit/49733
Commit-Queue: mukesh agrawal <quiche@chromium.org>
Reviewed-by: mukesh agrawal <quiche@chromium.org>
Tested-by: mukesh agrawal <quiche@chromium.org>
diff --git a/HACKING b/HACKING
index a7e5fbf..76f0040 100644
--- a/HACKING
+++ b/HACKING
@@ -124,3 +124,11 @@
2. Add the corresponding scope name to the kScopeNames array in
scope_logger.cc.
3. Update the GetAllScopeNames test in scope_logger_unittest.cc.
+
+- When adding externally visible (i.e. via RPC) properties to an object,
+ make sure that a) its setter emits any change notification required by
+ Chrome, and that b) its setter properly handles no-op changes.
+
+ Test that the property changes are handled correctly by adding test
+ cases similar to those in CellularServiceTest.PropertyChanges, and
+ CellularServiceTest.CustomSetterNoopChange.
\ No newline at end of file
diff --git a/accessor_interface.h b/accessor_interface.h
index 50dab1e..73fc6db 100644
--- a/accessor_interface.h
+++ b/accessor_interface.h
@@ -34,8 +34,11 @@
virtual void Clear(Error *error) = 0;
// Provides read-only access. Sets |error| on failure.
virtual T Get(Error *error) = 0;
- // Attempts to set the wrapped value. Sets |error| on failure.
- virtual void Set(const T &value, Error *error) = 0;
+ // Attempts to set the wrapped value. Sets |error| on failure. The
+ // return value indicates whether or not the wrapped value was
+ // modified. If the new value is the same as the old value, Set
+ // returns false, but with |error| unchanged.
+ virtual bool Set(const T &value, Error *error) = 0;
private:
DISALLOW_COPY_AND_ASSIGN(AccessorInterface);
diff --git a/cellular.cc b/cellular.cc
index 4f56668..7706c50 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -119,9 +119,8 @@
store->RegisterConstString(flimflam::kDBusConnectionProperty, &dbus_owner_);
store->RegisterConstString(flimflam::kDBusServiceProperty, &dbus_service_);
store->RegisterConstString(flimflam::kDBusObjectProperty, &dbus_path_);
- HelpRegisterDerivedString(flimflam::kTechnologyFamilyProperty,
- &Cellular::GetTechnologyFamily,
- NULL);
+ HelpRegisterConstDerivedString(flimflam::kTechnologyFamilyProperty,
+ &Cellular::GetTechnologyFamily);
HelpRegisterDerivedBool(flimflam::kCellularAllowRoamingProperty,
&Cellular::GetAllowRoaming,
&Cellular::SetAllowRoaming);
@@ -180,20 +179,19 @@
void Cellular::HelpRegisterDerivedBool(
const string &name,
bool(Cellular::*get)(Error *error),
- void(Cellular::*set)(const bool &value, Error *error)) {
+ bool(Cellular::*set)(const bool &value, Error *error)) {
mutable_store()->RegisterDerivedBool(
name,
BoolAccessor(
new CustomAccessor<Cellular, bool>(this, get, set)));
}
-void Cellular::HelpRegisterDerivedString(
+void Cellular::HelpRegisterConstDerivedString(
const string &name,
- string(Cellular::*get)(Error *),
- void(Cellular::*set)(const string&, Error *)) {
+ string(Cellular::*get)(Error *)) {
mutable_store()->RegisterDerivedString(
name,
- StringAccessor(new CustomAccessor<Cellular, string>(this, get, set)));
+ StringAccessor(new CustomAccessor<Cellular, string>(this, get, NULL)));
}
void Cellular::Start(Error *error,
@@ -715,11 +713,11 @@
return capability_->IsActivating();
}
-void Cellular::SetAllowRoaming(const bool &value, Error */*error*/) {
+bool Cellular::SetAllowRoaming(const bool &value, Error */*error*/) {
SLOG(Cellular, 2) << __func__
<< "(" << allow_roaming_ << "->" << value << ")";
if (allow_roaming_ == value) {
- return;
+ return false;
}
allow_roaming_ = value;
manager()->UpdateDevice(this);
@@ -733,6 +731,7 @@
Disconnect(&error);
}
adaptor()->EmitBoolChanged(flimflam::kCellularAllowRoamingProperty, value);
+ return true;
}
void Cellular::StartTermination() {
diff --git a/cellular.h b/cellular.h
index 2be9331..c36a4b4 100644
--- a/cellular.h
+++ b/cellular.h
@@ -261,6 +261,7 @@
FRIEND_TEST_ALL_PREFIXES(CellularTest, ConnectAddsTerminationAction);
FRIEND_TEST(CellularTest, ConnectFailure);
FRIEND_TEST(CellularTest, ConnectFailureNoService);
+ FRIEND_TEST(CellularTest, CustomSetterNoopChange);
FRIEND_TEST(CellularTest, DisableModem);
FRIEND_TEST(CellularTest, Disconnect);
FRIEND_TEST(CellularTest, DisconnectFailure);
@@ -306,18 +307,17 @@
void HelpRegisterDerivedBool(
const std::string &name,
bool(Cellular::*get)(Error *error),
- void(Cellular::*set)(const bool &value, Error *error));
- void HelpRegisterDerivedString(
+ bool(Cellular::*set)(const bool &value, Error *error));
+ void HelpRegisterConstDerivedString(
const std::string &name,
- std::string(Cellular::*get)(Error *error),
- void(Cellular::*set)(const std::string &value, Error *error));
+ std::string(Cellular::*get)(Error *error));
void OnConnectReply(const Error &error);
void OnDisconnectReply(const Error &error);
// DBUS accessors to read/modify the allow roaming property
bool GetAllowRoaming(Error */*error*/) { return allow_roaming_; }
- void SetAllowRoaming(const bool &value, Error *error);
+ bool SetAllowRoaming(const bool &value, Error *error);
// When shill terminates or ChromeOS suspends, this function is called to
// disconnect from the cellular network.
diff --git a/cellular_capability_classic.h b/cellular_capability_classic.h
index 7784036..334f7aa 100644
--- a/cellular_capability_classic.h
+++ b/cellular_capability_classic.h
@@ -177,11 +177,6 @@
FRIEND_TEST(CellularTest, ModemStateChangeEnable);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
- void HelpRegisterDerivedBool(
- const std::string &name,
- bool(CellularCapability::*get)(Error *error),
- void(CellularCapability::*set)(const bool &value, Error *error));
-
// Method reply and signal callbacks from Modem interface
void OnModemStateChangedSignal(
uint32 old_state, uint32 new_state, uint32 reason);
diff --git a/cellular_capability_gsm.cc b/cellular_capability_gsm.cc
index c684135..43e203d 100644
--- a/cellular_capability_gsm.cc
+++ b/cellular_capability_gsm.cc
@@ -75,10 +75,9 @@
store->RegisterConstBool(flimflam::kScanningProperty, &scanning_);
store->RegisterUint16(flimflam::kScanIntervalProperty, &scan_interval_);
store->RegisterConstBool(shill::kSIMPresentProperty, &sim_present_);
- HelpRegisterDerivedKeyValueStore(
+ HelpRegisterConstDerivedKeyValueStore(
flimflam::kSIMLockStatusProperty,
- &CellularCapabilityGSM::SimLockStatusToProperty,
- NULL);
+ &CellularCapabilityGSM::SimLockStatusToProperty);
store->RegisterConstStringmaps(flimflam::kCellularApnListProperty,
&apn_list_);
scanning_supported_ = true;
@@ -109,16 +108,14 @@
return status;
}
-void CellularCapabilityGSM::HelpRegisterDerivedKeyValueStore(
+void CellularCapabilityGSM::HelpRegisterConstDerivedKeyValueStore(
const string &name,
- KeyValueStore(CellularCapabilityGSM::*get)(Error *error),
- void(CellularCapabilityGSM::*set)(
- const KeyValueStore &value, Error *error)) {
+ KeyValueStore(CellularCapabilityGSM::*get)(Error *error)) {
cellular()->mutable_store()->RegisterDerivedKeyValueStore(
name,
KeyValueStoreAccessor(
new CustomAccessor<CellularCapabilityGSM, KeyValueStore>(
- this, get, set)));
+ this, get, NULL)));
}
void CellularCapabilityGSM::InitProxies() {
diff --git a/cellular_capability_gsm.h b/cellular_capability_gsm.h
index 1bb956a..be0ad51 100644
--- a/cellular_capability_gsm.h
+++ b/cellular_capability_gsm.h
@@ -174,11 +174,9 @@
void SetupApnTryList();
void FillConnectPropertyMap(DBusPropertiesMap *properties);
- void HelpRegisterDerivedKeyValueStore(
+ void HelpRegisterConstDerivedKeyValueStore(
const std::string &name,
- KeyValueStore(CellularCapabilityGSM::*get)(Error *error),
- void(CellularCapabilityGSM::*set)(
- const KeyValueStore &value, Error *error));
+ KeyValueStore(CellularCapabilityGSM::*get)(Error *error));
bool IsUnderlyingDeviceRegistered() const;
diff --git a/cellular_capability_universal.cc b/cellular_capability_universal.cc
index d95c6c4..8581c12 100644
--- a/cellular_capability_universal.cc
+++ b/cellular_capability_universal.cc
@@ -179,10 +179,9 @@
store->RegisterConstBool(flimflam::kScanningProperty,
&scanning_or_searching_);
store->RegisterUint16(flimflam::kScanIntervalProperty, &scan_interval_);
- HelpRegisterDerivedKeyValueStore(
+ HelpRegisterConstDerivedKeyValueStore(
flimflam::kSIMLockStatusProperty,
- &CellularCapabilityUniversal::SimLockStatusToProperty,
- NULL);
+ &CellularCapabilityUniversal::SimLockStatusToProperty);
store->RegisterConstString(shill::kSIMOperatorIdProperty, &operator_id_);
store->RegisterConstBool(shill::kSIMPresentProperty, &sim_present_);
store->RegisterConstStringmaps(flimflam::kCellularApnListProperty,
@@ -199,16 +198,14 @@
return status;
}
-void CellularCapabilityUniversal::HelpRegisterDerivedKeyValueStore(
+void CellularCapabilityUniversal::HelpRegisterConstDerivedKeyValueStore(
const string &name,
- KeyValueStore(CellularCapabilityUniversal::*get)(Error *error),
- void(CellularCapabilityUniversal::*set)(
- const KeyValueStore &value, Error *error)) {
+ KeyValueStore(CellularCapabilityUniversal::*get)(Error *error)) {
cellular()->mutable_store()->RegisterDerivedKeyValueStore(
name,
KeyValueStoreAccessor(
new CustomAccessor<CellularCapabilityUniversal, KeyValueStore>(
- this, get, set)));
+ this, get, NULL)));
}
void CellularCapabilityUniversal::InitProxies() {
diff --git a/cellular_capability_universal.h b/cellular_capability_universal.h
index 297c9e9..9c85fc4 100644
--- a/cellular_capability_universal.h
+++ b/cellular_capability_universal.h
@@ -264,11 +264,9 @@
void SetupApnTryList();
void FillConnectPropertyMap(DBusPropertiesMap *properties);
- void HelpRegisterDerivedKeyValueStore(
+ void HelpRegisterConstDerivedKeyValueStore(
const std::string &name,
- KeyValueStore(CellularCapabilityUniversal::*get)(Error *error),
- void(CellularCapabilityUniversal::*set)(
- const KeyValueStore &value, Error *error));
+ KeyValueStore(CellularCapabilityUniversal::*get)(Error *error));
// Returns true if a connect error should be retried. This function
// abstracts modem specific behavior for modems which do a lousy job
diff --git a/cellular_service.cc b/cellular_service.cc
index 36f0bbf..d5a54c0 100644
--- a/cellular_service.cc
+++ b/cellular_service.cc
@@ -156,7 +156,7 @@
void CellularService::HelpRegisterDerivedStringmap(
const string &name,
Stringmap(CellularService::*get)(Error *error),
- void(CellularService::*set)(
+ bool(CellularService::*set)(
const Stringmap &value, Error *error)) {
mutable_store()->RegisterDerivedStringmap(
name,
@@ -183,18 +183,23 @@
return apn_info_;
}
-void CellularService::SetApn(const Stringmap &value, Error *error) {
+bool CellularService::SetApn(const Stringmap &value, Error *error) {
// Only copy in the fields we care about, and validate the contents.
// If the "apn" field is missing or empty, the APN is cleared.
string str;
- if (!GetNonEmptyField(value, flimflam::kApnProperty, &str)) {
- apn_info_.clear();
- } else {
- apn_info_[flimflam::kApnProperty] = str;
+ Stringmap new_apn_info;
+ if (GetNonEmptyField(value, flimflam::kApnProperty, &str)) {
+ new_apn_info[flimflam::kApnProperty] = str;
if (GetNonEmptyField(value, flimflam::kApnUsernameProperty, &str))
- apn_info_[flimflam::kApnUsernameProperty] = str;
+ new_apn_info[flimflam::kApnUsernameProperty] = str;
if (GetNonEmptyField(value, flimflam::kApnPasswordProperty, &str))
- apn_info_[flimflam::kApnPasswordProperty] = str;
+ new_apn_info[flimflam::kApnPasswordProperty] = str;
+ }
+ if (apn_info_ == new_apn_info) {
+ return false;
+ }
+ apn_info_ = new_apn_info;
+ if (ContainsKey(apn_info_, flimflam::kApnProperty)) {
// Clear the last good APN, otherwise the one the user just
// set won't be used, since LastGoodApn comes first in the
// search order when trying to connect. Only do this if a
@@ -204,6 +209,7 @@
}
adaptor()->EmitStringmapChanged(flimflam::kCellularApnProperty, apn_info_);
SaveToCurrentProfile();
+ return true;
}
void CellularService::SetLastGoodApn(const Stringmap &apn_info) {
diff --git a/cellular_service.h b/cellular_service.h
index 226d330..19d1ff6 100644
--- a/cellular_service.h
+++ b/cellular_service.h
@@ -147,6 +147,7 @@
FRIEND_TEST(CellularServiceTest, OutOfCreditsNotDetectedConnectionNotDropped);
FRIEND_TEST(CellularServiceTest, OutOfCreditsNotDetectedIntermittentNetwork);
FRIEND_TEST(CellularServiceTest, OutOfCreditsNotEnforced);
+ FRIEND_TEST(CellularServiceTest, CustomSetterNoopChange);
static const char kAutoConnActivating[];
static const char kAutoConnDeviceDisabled[];
@@ -159,12 +160,12 @@
void HelpRegisterDerivedStringmap(
const std::string &name,
Stringmap(CellularService::*get)(Error *error),
- void(CellularService::*set)(const Stringmap &value, Error *error));
+ bool(CellularService::*set)(const Stringmap &value, Error *error));
virtual std::string GetDeviceRpcId(Error *error);
Stringmap GetApn(Error *error);
- void SetApn(const Stringmap &value, Error *error);
+ bool SetApn(const Stringmap &value, Error *error);
static void SaveApn(StoreInterface *storage,
const std::string &storage_group,
const Stringmap *apn_info,
diff --git a/cellular_service_unittest.cc b/cellular_service_unittest.cc
index 62f15a2..9577015 100644
--- a/cellular_service_unittest.cc
+++ b/cellular_service_unittest.cc
@@ -572,4 +572,28 @@
Mock::VerifyAndClearExpectations(adaptor_);
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(CellularServiceTest, CustomSetterNoopChange) {
+ // Test that we didn't break any setters provided by the base class.
+ TestCustomSetterNoopChange(service_, modem_info_.mock_manager());
+
+ // Test the new setter we added.
+ // First set up our environment...
+ static const char kApn[] = "TheAPN";
+ static const char kUsername[] = "commander.data";
+ Error error;
+ Stringmap testapn;
+ ProfileRefPtr profile(new NiceMock<MockProfile>(nullptr, nullptr, nullptr));
+ service_->set_profile(profile);
+ testapn[flimflam::kApnProperty] = kApn;
+ testapn[flimflam::kApnUsernameProperty] = kUsername;
+ // ... then set to a known value ...
+ EXPECT_TRUE(service_->SetApn(testapn, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // ... then set to same value.
+ EXPECT_FALSE(service_->SetApn(testapn, &error));
+ EXPECT_TRUE(error.IsSuccess());
+}
+
} // namespace shill
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index c71ac12..fe3c785 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -92,10 +92,12 @@
TEST_F(CellularPropertyTest, SetProperty) {
{
::DBus::Error error;
+ ::DBus::Variant allow_roaming;
+ allow_roaming.writer().append_bool(true);
EXPECT_TRUE(DBusAdaptor::SetProperty(
device_->mutable_store(),
flimflam::kCellularAllowRoamingProperty,
- PropertyStoreTest::kBoolV,
+ allow_roaming,
&error));
}
// Ensure that attempting to write a R/O property returns InvalidArgs error.
@@ -105,6 +107,7 @@
flimflam::kAddressProperty,
PropertyStoreTest::kStringV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
{
@@ -113,6 +116,7 @@
flimflam::kCarrierProperty,
PropertyStoreTest::kStringV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
}
@@ -975,4 +979,13 @@
EXPECT_TRUE(device_->allow_roaming_);
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(CellularTest, CustomSetterNoopChange) {
+ Error error;
+ EXPECT_FALSE(device_->allow_roaming_);
+ EXPECT_FALSE(device_->SetAllowRoaming(false, &error));
+ EXPECT_TRUE(error.IsSuccess());
+}
+
} // namespace shill
diff --git a/dbus_adaptor.cc b/dbus_adaptor.cc
index 5341d6a..6e28833 100644
--- a/dbus_adaptor.cc
+++ b/dbus_adaptor.cc
@@ -47,39 +47,43 @@
const ::DBus::Variant &value,
::DBus::Error *error) {
Error e;
+ bool ret;
if (DBusAdaptor::IsBool(value.signature()))
- store->SetBoolProperty(name, value.reader().get_bool(), &e);
+ ret = store->SetBoolProperty(name, value.reader().get_bool(), &e);
else if (DBusAdaptor::IsByte(value.signature()))
- store->SetUint8Property(name, value.reader().get_byte(), &e);
+ ret = store->SetUint8Property(name, value.reader().get_byte(), &e);
else if (DBusAdaptor::IsInt16(value.signature()))
- store->SetInt16Property(name, value.reader().get_int16(), &e);
+ ret = store->SetInt16Property(name, value.reader().get_int16(), &e);
else if (DBusAdaptor::IsInt32(value.signature()))
- store->SetInt32Property(name, value.reader().get_int32(), &e);
+ ret = store->SetInt32Property(name, value.reader().get_int32(), &e);
else if (DBusAdaptor::IsPath(value.signature()))
- store->SetStringProperty(name, value.reader().get_path(), &e);
+ ret = store->SetStringProperty(name, value.reader().get_path(), &e);
else if (DBusAdaptor::IsString(value.signature()))
- store->SetStringProperty(name, value.reader().get_string(), &e);
+ ret = store->SetStringProperty(name, value.reader().get_string(), &e);
else if (DBusAdaptor::IsStringmap(value.signature()))
- store->SetStringmapProperty(name,
- value.operator map<string, string>(),
- &e);
+ ret = store->SetStringmapProperty(name,
+ value.operator map<string, string>(),
+ &e);
else if (DBusAdaptor::IsStringmaps(value.signature())) {
SLOG(DBus, 1) << " can't yet handle setting type " << value.signature();
+ ret = false;
e.Populate(Error::kInternalError);
} else if (DBusAdaptor::IsStrings(value.signature()))
- store->SetStringsProperty(name, value.operator vector<string>(), &e);
+ ret = store->SetStringsProperty(name, value.operator vector<string>(), &e);
else if (DBusAdaptor::IsUint16(value.signature()))
- store->SetUint16Property(name, value.reader().get_uint16(), &e);
+ ret = store->SetUint16Property(name, value.reader().get_uint16(), &e);
else if (DBusAdaptor::IsUint32(value.signature()))
- store->SetUint32Property(name, value.reader().get_uint32(), &e);
+ ret = store->SetUint32Property(name, value.reader().get_uint32(), &e);
else if (DBusAdaptor::IsUint64(value.signature()))
- store->SetUint64Property(name, value.reader().get_uint64(), &e);
+ ret = store->SetUint64Property(name, value.reader().get_uint64(), &e);
else if (DBusAdaptor::IsKeyValueStore(value.signature())) {
SLOG(DBus, 1) << " can't yet handle setting type " << value.signature();
+ ret = false;
e.Populate(Error::kInternalError);
} else {
NOTREACHED() << " unknown type: " << value.signature();
+ ret = false;
e.Populate(Error::kInternalError);
}
@@ -87,7 +91,7 @@
e.ToDBusError(error);
}
- return e.IsSuccess();
+ return ret;
}
// static
diff --git a/dbus_adaptor.h b/dbus_adaptor.h
index dcb9bd1..f7d2836 100644
--- a/dbus_adaptor.h
+++ b/dbus_adaptor.h
@@ -37,6 +37,10 @@
DBusAdaptor(DBus::Connection* conn, const std::string &object_path);
virtual ~DBusAdaptor();
+ // Set the property with |name| through |store|. Returns true if and
+ // only if the property was changed. Updates |error| if a) an error
+ // was encountered, and b) |error| is non-NULL. Otherwise, |error| is
+ // unchanged.
static bool SetProperty(PropertyStore *store,
const std::string &name,
const ::DBus::Variant &value,
diff --git a/dbus_adaptor_unittest.cc b/dbus_adaptor_unittest.cc
index 39d1fa5..b56bfa2 100644
--- a/dbus_adaptor_unittest.cc
+++ b/dbus_adaptor_unittest.cc
@@ -217,6 +217,48 @@
EXPECT_TRUE(DBusAdaptor::SetProperty(&store, "", byte_v_, &e11));
}
+// SetProperty method should propagate failures. This should happen
+// even if error isn't set. (This is to accomodate the fact that, if
+// the property already has the desired value, the store will return
+// false, without setting an error.)
+TEST_F(DBusAdaptorTest, SetPropertyFailure) {
+ MockPropertyStore store;
+ ::DBus::Error e1, e2, e3, e4, e5, e6, e7, e8, e9, e10, e11;
+
+ EXPECT_CALL(store, Contains(_)).WillRepeatedly(Return(false));
+ EXPECT_CALL(store, SetBoolProperty("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetInt16Property("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetInt32Property("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetStringProperty("", _, _))
+ .WillOnce(Return(false));
+ EXPECT_CALL(store, SetStringmapProperty("", _, _))
+ .WillOnce(Return(false));
+ EXPECT_CALL(store, SetStringsProperty("", _, _))
+ .WillOnce(Return(false));
+ EXPECT_CALL(store, SetUint8Property("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetUint16Property("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetUint32Property("", _, _)).WillOnce(Return(false));
+ EXPECT_CALL(store, SetUint64Property("", _, _)).WillOnce(Return(false));
+
+ string string_path("/false/path");
+ ::DBus::Path path(string_path);
+ ::DBus::Variant path_v = DBusAdaptor::PathToVariant(path);
+ EXPECT_CALL(store, SetStringProperty("", StrEq(string_path), _))
+ .WillOnce(Return(false));
+
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", bool_v_, &e1));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", path_v, &e2));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", string_v_, &e3));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", strings_v_, &e4));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", int16_v_, &e5));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", int32_v_, &e6));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", uint16_v_, &e7));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", uint32_v_, &e8));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", uint64_v_, &e9));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", stringmap_v_, &e10));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", byte_v_, &e11));
+}
+
void SetError(const string &/*name*/, Error *error) {
error->Populate(Error::kInvalidProperty);
}
diff --git a/device.cc b/device.cc
index 5a21384..5a3d17a 100644
--- a/device.cc
+++ b/device.cc
@@ -142,9 +142,8 @@
&Device::AvailableIPConfigs);
store_.RegisterConstString(flimflam::kNameProperty, &link_name_);
store_.RegisterConstBool(flimflam::kPoweredProperty, &enabled_);
- HelpRegisterDerivedString(flimflam::kTypeProperty,
- &Device::GetTechnologyString,
- NULL);
+ HelpRegisterConstDerivedString(flimflam::kTypeProperty,
+ &Device::GetTechnologyString);
HelpRegisterConstDerivedUint64(shill::kLinkMonitorResponseTimeProperty,
&Device::GetLinkMonitorResponseTime);
@@ -400,22 +399,12 @@
dhcp_provider_->DestroyLease(name);
}
-void Device::HelpRegisterDerivedString(
+void Device::HelpRegisterConstDerivedString(
const string &name,
- string(Device::*get)(Error *error),
- void(Device::*set)(const string &value, Error *error)) {
+ string(Device::*get)(Error *error)) {
store_.RegisterDerivedString(
name,
- StringAccessor(new CustomAccessor<Device, string>(this, get, set)));
-}
-
-void Device::HelpRegisterDerivedStrings(
- const string &name,
- Strings(Device::*get)(Error *error),
- void(Device::*set)(const Strings &value, Error *error)) {
- store_.RegisterDerivedStrings(
- name,
- StringsAccessor(new CustomAccessor<Device, Strings>(this, get, set)));
+ StringAccessor(new CustomAccessor<Device, string>(this, get, NULL)));
}
void Device::HelpRegisterConstDerivedRpcIdentifiers(
diff --git a/device.h b/device.h
index 4ef7b80..79a2f6e 100644
--- a/device.h
+++ b/device.h
@@ -392,15 +392,9 @@
const ServiceRefPtr &selected_service() const { return selected_service_; }
- void HelpRegisterDerivedString(
+ void HelpRegisterConstDerivedString(
const std::string &name,
- std::string(Device::*get)(Error *),
- void(Device::*set)(const std::string&, Error *));
-
- void HelpRegisterDerivedStrings(
- const std::string &name,
- Strings(Device::*get)(Error *error),
- void(Device::*set)(const Strings &value, Error *error));
+ std::string(Device::*get)(Error *));
void HelpRegisterConstDerivedRpcIdentifiers(
const std::string &name,
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index dacafbf..ec9dafd 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -586,6 +586,7 @@
flimflam::kAddressProperty,
PropertyStoreTest::kStringV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
diff --git a/eap_credentials.cc b/eap_credentials.cc
index 4eb0899..89f30d4 100644
--- a/eap_credentials.cc
+++ b/eap_credentials.cc
@@ -432,31 +432,44 @@
use_system_cas_ = true;
}
-void EapCredentials::SetEapPassword(const string &password, Error */*error*/) {
+bool EapCredentials::SetEapPassword(const string &password, Error */*error*/) {
+ if (password_ == password) {
+ return false;
+ }
password_ = password;
+ return true;
}
-void EapCredentials::SetEapPrivateKeyPassword(const string &password,
+bool EapCredentials::SetEapPrivateKeyPassword(const string &password,
Error */*error*/) {
+ if (private_key_password_ == password) {
+ return false;
+ }
private_key_password_ = password;
+ return true;
}
string EapCredentials::GetKeyManagement(Error */*error*/) {
return key_management_;
}
-void EapCredentials::SetKeyManagement(const std::string &key_management,
+bool EapCredentials::SetKeyManagement(const std::string &key_management,
Error */*error*/) {
- if (!key_management.empty()) {
- key_management_ = key_management;
+ if (key_management.empty()) {
+ return false;
}
+ if (key_management_ == key_management) {
+ return false;
+ }
+ key_management_ = key_management;
+ return true;
}
void EapCredentials::HelpRegisterDerivedString(
PropertyStore *store,
const string &name,
string(EapCredentials::*get)(Error *),
- void(EapCredentials::*set)(const string&, Error *)) {
+ bool(EapCredentials::*set)(const string&, Error *)) {
store->RegisterDerivedString(
name,
StringAccessor(new CustomAccessor<EapCredentials, string>(
@@ -466,7 +479,7 @@
void EapCredentials::HelpRegisterWriteOnlyDerivedString(
PropertyStore *store,
const string &name,
- void(EapCredentials::*set)(const string &, Error *),
+ bool(EapCredentials::*set)(const string &, Error *),
void(EapCredentials::*clear)(Error *),
const string *default_value) {
store->RegisterDerivedString(
diff --git a/eap_credentials.h b/eap_credentials.h
index ef4dcc7..da5ab12 100644
--- a/eap_credentials.h
+++ b/eap_credentials.h
@@ -97,7 +97,7 @@
virtual void Reset();
// Setter that guards against emptying the "Key Management" value.
- virtual void SetKeyManagement(const std::string &key_management,
+ virtual bool SetKeyManagement(const std::string &key_management,
Error *error);
// Getters and setters.
@@ -121,7 +121,7 @@
PropertyStore *store,
const std::string &name,
std::string(EapCredentials::*get)(Error *error),
- void(EapCredentials::*set)(const std::string &value, Error *error));
+ bool(EapCredentials::*set)(const std::string &value, Error *error));
// Expose a property in |store|, with the name |name|.
//
@@ -134,7 +134,7 @@
void HelpRegisterWriteOnlyDerivedString(
PropertyStore *store,
const std::string &name,
- void(EapCredentials::*set)(const std::string &value, Error *error),
+ bool(EapCredentials::*set)(const std::string &value, Error *error),
void(EapCredentials::*clear)(Error *error),
const std::string *default_value);
@@ -149,8 +149,8 @@
bool save);
// Setters for write-only RPC properties.
- void SetEapPassword(const std::string &password, Error *error);
- void SetEapPrivateKeyPassword(const std::string &password, Error *error);
+ bool SetEapPassword(const std::string &password, Error *error);
+ bool SetEapPrivateKeyPassword(const std::string &password, Error *error);
// RPC getter for key_management_.
std::string GetKeyManagement(Error *error);
diff --git a/eap_credentials_unittest.cc b/eap_credentials_unittest.cc
index 6a3d3b0..25e8593 100644
--- a/eap_credentials_unittest.cc
+++ b/eap_credentials_unittest.cc
@@ -110,6 +110,12 @@
const string &GetKeyManagement() {
return eap_.key_management_;
}
+ bool SetEapPassword(const string &password, Error *error) {
+ return eap_.SetEapPassword(password, error);
+ }
+ bool SetEapPrivateKeyPassword(const string &password, Error *error) {
+ return eap_.SetEapPrivateKeyPassword(password, error);
+ }
EapCredentials eap_;
MockCertificateFile certificate_file_;
@@ -451,4 +457,44 @@
EXPECT_EQ(kKeyManagement1, GetKeyManagement());
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(EapCredentialsTest, CustomSetterNoopChange) {
+ // SetEapKeyManagement
+ {
+ const string kKeyManagement("foo");
+ Error error;
+ // Set to known value.
+ EXPECT_TRUE(eap_.SetKeyManagement(kKeyManagement, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(eap_.SetKeyManagement(kKeyManagement, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetEapPassword
+ {
+ const string kPassword("foo");
+ Error error;
+ // Set to known value.
+ EXPECT_TRUE(SetEapPassword(kPassword, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(SetEapPassword(kPassword, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetEapPrivateKeyPassword
+ {
+ const string kPrivateKeyPassword("foo");
+ Error error;
+ // Set to known value.
+ EXPECT_TRUE(SetEapPrivateKeyPassword(kPrivateKeyPassword, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(SetEapPrivateKeyPassword(kPrivateKeyPassword, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+}
+
} // namespace shill
diff --git a/ethernet_eap_service_unittest.cc b/ethernet_eap_service_unittest.cc
index 5620568..fe61a58 100644
--- a/ethernet_eap_service_unittest.cc
+++ b/ethernet_eap_service_unittest.cc
@@ -8,11 +8,13 @@
#include <chromeos/dbus/service_constants.h>
#include <gtest/gtest.h>
+#include "shill/mock_adaptors.h"
#include "shill/mock_control.h"
#include "shill/mock_ethernet_eap_provider.h"
#include "shill/mock_event_dispatcher.h"
#include "shill/mock_manager.h"
#include "shill/mock_metrics.h"
+#include "shill/service_property_change_test.h"
#include "shill/technology.h"
using testing::Return;
@@ -31,6 +33,10 @@
virtual ~EthernetEapServiceTest() {}
protected:
+ ServiceMockAdaptor *GetAdaptor() {
+ return dynamic_cast<ServiceMockAdaptor *>(service_->adaptor());
+ }
+
MockControl control_;
MockEventDispatcher dispatcher_;
MockMetrics metrics_;
@@ -65,4 +71,14 @@
EXPECT_FALSE(service_->Unload());
}
+TEST_F(EthernetEapServiceTest, PropertyChanges) {
+ TestCommonPropertyChanges(service_, GetAdaptor());
+}
+
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(EthernetEapServiceTest, CustomSetterNoopChange) {
+ TestCustomSetterNoopChange(service_, &manager_);
+}
+
} // namespace shill
diff --git a/ethernet_service.cc b/ethernet_service.cc
index e9dcbb0..8fab092 100644
--- a/ethernet_service.cc
+++ b/ethernet_service.cc
@@ -64,15 +64,15 @@
kServiceType, ethernet_->address().c_str());
}
-void EthernetService::SetAutoConnectFull(const bool &connect,
+bool EthernetService::SetAutoConnectFull(const bool &connect,
Error *error) {
if (!connect) {
Error::PopulateAndLog(
error, Error::kInvalidArguments,
"Auto-connect on Ethernet services must not be disabled.");
- return;
+ return false;
}
- Service::SetAutoConnectFull(connect, error);
+ return Service::SetAutoConnectFull(connect, error);
}
void EthernetService::Remove(Error *error) {
diff --git a/ethernet_service.h b/ethernet_service.h
index af32816..d7c0b63 100644
--- a/ethernet_service.h
+++ b/ethernet_service.h
@@ -34,7 +34,7 @@
// ethernet_<MAC>
virtual std::string GetStorageIdentifier() const;
virtual bool IsAutoConnectByDefault() const { return true; }
- virtual void SetAutoConnectFull(const bool &connect, Error *error);
+ virtual bool SetAutoConnectFull(const bool &connect, Error *error);
virtual void Remove(Error *error);
diff --git a/ethernet_service_unittest.cc b/ethernet_service_unittest.cc
index 427231b..a773866 100644
--- a/ethernet_service_unittest.cc
+++ b/ethernet_service_unittest.cc
@@ -45,7 +45,7 @@
return service_->GetAutoConnect(NULL);
}
- void SetAutoConnect(const bool connect, Error *error) {
+ bool SetAutoConnect(const bool connect, Error *error) {
return service_->SetAutoConnectFull(connect, error);
}
@@ -90,4 +90,10 @@
TestCommonPropertyChanges(service_, GetAdaptor());
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(EthernetServiceTest, CustomSetterNoopChange) {
+ TestCustomSetterNoopChange(service_, &mock_manager_);
+}
+
} // namespace shill
diff --git a/manager.cc b/manager.cc
index 2e35674..d81f818 100644
--- a/manager.cc
+++ b/manager.cc
@@ -122,15 +122,13 @@
&Manager::GetActiveProfileRpcIdentifier,
NULL);
store_.RegisterBool(flimflam::kArpGatewayProperty, &props_.arp_gateway);
- HelpRegisterDerivedStrings(flimflam::kAvailableTechnologiesProperty,
- &Manager::AvailableTechnologies,
- NULL);
+ HelpRegisterConstDerivedStrings(flimflam::kAvailableTechnologiesProperty,
+ &Manager::AvailableTechnologies);
HelpRegisterDerivedString(flimflam::kCheckPortalListProperty,
&Manager::GetCheckPortalList,
&Manager::SetCheckPortalList);
- HelpRegisterDerivedStrings(flimflam::kConnectedTechnologiesProperty,
- &Manager::ConnectedTechnologies,
- NULL);
+ HelpRegisterConstDerivedStrings(flimflam::kConnectedTechnologiesProperty,
+ &Manager::ConnectedTechnologies);
store_.RegisterString(flimflam::kCountryProperty, &props_.country);
HelpRegisterDerivedString(flimflam::kDefaultTechnologyProperty,
&Manager::DefaultTechnology,
@@ -140,9 +138,8 @@
&Manager::GetDefaultServiceRpcIdentifier);
HelpRegisterConstDerivedRpcIdentifiers(flimflam::kDevicesProperty,
&Manager::EnumerateDevices);
- HelpRegisterDerivedStrings(flimflam::kEnabledTechnologiesProperty,
- &Manager::EnabledTechnologies,
- NULL);
+ HelpRegisterConstDerivedStrings(flimflam::kEnabledTechnologiesProperty,
+ &Manager::EnabledTechnologies);
HelpRegisterDerivedString(shill::kIgnoredDNSSearchPathsProperty,
&Manager::GetIgnoredDNSSearchPaths,
&Manager::SetIgnoredDNSSearchPaths);
@@ -164,9 +161,8 @@
&Manager::EnumerateCompleteServices);
HelpRegisterConstDerivedRpcIdentifiers(flimflam::kServiceWatchListProperty,
&Manager::EnumerateWatchedServices);
- HelpRegisterDerivedStrings(kUninitializedTechnologiesProperty,
- &Manager::UninitializedTechnologies,
- NULL);
+ HelpRegisterConstDerivedStrings(kUninitializedTechnologiesProperty,
+ &Manager::UninitializedTechnologies);
// Set default technology order "by hand", to avoid invoking side
// effects of SetTechnologyOrder.
@@ -1277,19 +1273,18 @@
void Manager::HelpRegisterDerivedString(
const string &name,
string(Manager::*get)(Error *),
- void(Manager::*set)(const string&, Error *)) {
+ bool(Manager::*set)(const string&, Error *)) {
store_.RegisterDerivedString(
name,
StringAccessor(new CustomAccessor<Manager, string>(this, get, set)));
}
-void Manager::HelpRegisterDerivedStrings(
+void Manager::HelpRegisterConstDerivedStrings(
const string &name,
- Strings(Manager::*get)(Error *),
- void(Manager::*set)(const Strings &, Error *)) {
+ Strings(Manager::*get)(Error *)) {
store_.RegisterDerivedStrings(
name,
- StringsAccessor(new CustomAccessor<Manager, Strings>(this, get, set)));
+ StringsAccessor(new CustomAccessor<Manager, Strings>(this, get, NULL)));
}
void Manager::SortServices() {
@@ -1587,23 +1582,31 @@
props_.check_portal_list;
}
-void Manager::SetCheckPortalList(const string &portal_list, Error *error) {
- props_.check_portal_list = portal_list;
+bool Manager::SetCheckPortalList(const string &portal_list, Error *error) {
use_startup_portal_list_ = false;
+ if (props_.check_portal_list == portal_list) {
+ return false;
+ }
+ props_.check_portal_list = portal_list;
+ return true;
}
string Manager::GetIgnoredDNSSearchPaths(Error */*error*/) {
return props_.ignored_dns_search_paths;
}
-void Manager::SetIgnoredDNSSearchPaths(const string &ignored_paths,
+bool Manager::SetIgnoredDNSSearchPaths(const string &ignored_paths,
Error */*error*/) {
- props_.ignored_dns_search_paths = ignored_paths;
+ if (props_.ignored_dns_search_paths == ignored_paths) {
+ return false;
+ }
vector<string> ignored_path_list;
if (!ignored_paths.empty()) {
base::SplitString(ignored_paths, ',', &ignored_path_list);
}
+ props_.ignored_dns_search_paths = ignored_paths;
resolver_->set_ignored_search_list(ignored_path_list);
+ return true;
}
// called via RPC (e.g., from ManagerDBusAdaptor)
diff --git a/manager.h b/manager.h
index 17e2f5d..202c7b7 100644
--- a/manager.h
+++ b/manager.h
@@ -411,8 +411,8 @@
RpcIdentifier GetDefaultServiceRpcIdentifier(Error *error);
std::string GetIgnoredDNSSearchPaths(Error *error);
ServiceRefPtr GetServiceInner(const KeyValueStore &args, Error *error);
- void SetCheckPortalList(const std::string &portal_list, Error *error);
- void SetIgnoredDNSSearchPaths(const std::string &ignored_paths, Error *error);
+ bool SetCheckPortalList(const std::string &portal_list, Error *error);
+ bool SetIgnoredDNSSearchPaths(const std::string &ignored_paths, Error *error);
void EmitDefaultService();
bool IsTechnologyInList(const std::string &technology_list,
Technology::Identifier tech) const;
@@ -440,11 +440,10 @@
void HelpRegisterDerivedString(
const std::string &name,
std::string(Manager::*get)(Error *),
- void(Manager::*set)(const std::string&, Error *));
- void HelpRegisterDerivedStrings(
+ bool(Manager::*set)(const std::string&, Error *));
+ void HelpRegisterConstDerivedStrings(
const std::string &name,
- Strings(Manager::*get)(Error *),
- void(Manager::*set)(const Strings&, Error *));
+ Strings(Manager::*get)(Error *));
bool HasProfile(const Profile::Identifier &ident);
void PushProfileInternal(const Profile::Identifier &ident,
diff --git a/manager_unittest.cc b/manager_unittest.cc
index 10ddc4b..81c1d46 100644
--- a/manager_unittest.cc
+++ b/manager_unittest.cc
@@ -265,8 +265,12 @@
manager()->resolver_ = resolver;
}
- void SetIgnoredDNSSearchPaths(const string &search_paths) {
- manager()->SetIgnoredDNSSearchPaths(search_paths, NULL);
+ bool SetIgnoredDNSSearchPaths(const string &search_paths, Error *error) {
+ return manager()->SetIgnoredDNSSearchPaths(search_paths, error);
+ }
+
+ bool SetCheckPortalList(const string &check_portal_list, Error *error) {
+ return manager()->SetCheckPortalList(check_portal_list, error);
}
const string &GetIgnoredDNSSearchPaths() {
@@ -1411,16 +1415,20 @@
TEST_F(ManagerTest, SetProperty) {
{
::DBus::Error error;
+ ::DBus::Variant offline_mode;
+ offline_mode.writer().append_bool(true);
EXPECT_TRUE(DBusAdaptor::SetProperty(manager()->mutable_store(),
flimflam::kOfflineModeProperty,
- PropertyStoreTest::kBoolV,
+ offline_mode,
&error));
}
{
::DBus::Error error;
+ ::DBus::Variant country;
+ country.writer().append_string("a_country");
EXPECT_TRUE(DBusAdaptor::SetProperty(manager()->mutable_store(),
flimflam::kCountryProperty,
- PropertyStoreTest::kStringV,
+ country,
&error));
}
// Attempt to write with value of wrong type should return InvalidArgs.
@@ -3239,25 +3247,27 @@
TEST_F(ManagerTest, IgnoredSearchList) {
scoped_ptr<MockResolver> resolver(new StrictMock<MockResolver>());
- SetResolver(resolver.get());
vector<string> ignored_paths;
- EXPECT_CALL(*resolver.get(), set_ignored_search_list(ignored_paths));
- SetIgnoredDNSSearchPaths("");
- EXPECT_EQ("", GetIgnoredDNSSearchPaths());
+ SetResolver(resolver.get());
const string kIgnored0 = "chromium.org";
ignored_paths.push_back(kIgnored0);
EXPECT_CALL(*resolver.get(), set_ignored_search_list(ignored_paths));
- SetIgnoredDNSSearchPaths(kIgnored0);
+ SetIgnoredDNSSearchPaths(kIgnored0, NULL);
EXPECT_EQ(kIgnored0, GetIgnoredDNSSearchPaths());
const string kIgnored1 = "google.com";
const string kIgnoredSum = kIgnored0 + "," + kIgnored1;
ignored_paths.push_back(kIgnored1);
EXPECT_CALL(*resolver.get(), set_ignored_search_list(ignored_paths));
- SetIgnoredDNSSearchPaths(kIgnoredSum);
+ SetIgnoredDNSSearchPaths(kIgnoredSum, NULL);
EXPECT_EQ(kIgnoredSum, GetIgnoredDNSSearchPaths());
+ ignored_paths.clear();
+ EXPECT_CALL(*resolver.get(), set_ignored_search_list(ignored_paths));
+ SetIgnoredDNSSearchPaths("", NULL);
+ EXPECT_EQ("", GetIgnoredDNSSearchPaths());
+
SetResolver(Resolver::GetInstance());
}
@@ -3769,4 +3779,38 @@
Mock::VerifyAndClearExpectations(wifi_provider);
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(ManagerTest, CustomSetterNoopChange) {
+ // SetCheckPortalList
+ {
+ static const string kCheckPortalList = "weird-device,weirder-device";
+ Error error;
+ // Set to known value.
+ EXPECT_TRUE(SetCheckPortalList(kCheckPortalList, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(SetCheckPortalList(kCheckPortalList, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetIgnoredDNSSearchPaths
+ {
+ NiceMock<MockResolver> resolver;
+ static const string kIgnoredPaths = "example.com,example.org";
+ Error error;
+ SetResolver(&resolver);
+ // Set to known value.
+ EXPECT_CALL(resolver, set_ignored_search_list(_));
+ EXPECT_TRUE(SetIgnoredDNSSearchPaths(kIgnoredPaths, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ Mock::VerifyAndClearExpectations(&resolver);
+ // Set to same value.
+ EXPECT_CALL(resolver, set_ignored_search_list(_)).Times(0);
+ EXPECT_FALSE(SetIgnoredDNSSearchPaths(kIgnoredPaths, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ Mock::VerifyAndClearExpectations(&resolver);
+ }
+}
+
} // namespace shill
diff --git a/mock_eap_credentials.h b/mock_eap_credentials.h
index 3d14d68..43991da 100644
--- a/mock_eap_credentials.h
+++ b/mock_eap_credentials.h
@@ -30,7 +30,7 @@
MOCK_CONST_METHOD3(Save, void(
StoreInterface *store, const std::string &id, bool save_credentials));
MOCK_METHOD0(Reset, void());
- MOCK_METHOD2(SetKeyManagement, void(const std::string &key_management,
+ MOCK_METHOD2(SetKeyManagement, bool(const std::string &key_management,
Error *error));
MOCK_CONST_METHOD0(identity, const std::string &());
MOCK_CONST_METHOD0(key_management, const std::string &());
diff --git a/mock_service.h b/mock_service.h
index 3598115..304ed05 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -63,6 +63,7 @@
MOCK_CONST_METHOD0(explicitly_disconnected, bool());
MOCK_CONST_METHOD0(eap, const EapCredentials *());
MOCK_CONST_METHOD0(technology, Technology::Identifier());
+ MOCK_METHOD1(OnPropertyChanged, void(const std::string &property));
// Set a string for this Service via |store|. Can be wired to Save() for
// test purposes.
bool FauxSave(StoreInterface *store);
diff --git a/profile.cc b/profile.cc
index c88d835..7f9f655 100644
--- a/profile.cc
+++ b/profile.cc
@@ -57,12 +57,10 @@
// flimflam::kOfflineModeProperty: Registered in DefaultProfile
// flimflam::kPortalURLProperty: Registered in DefaultProfile
- HelpRegisterDerivedStrings(flimflam::kServicesProperty,
- &Profile::EnumerateAvailableServices,
- NULL);
- HelpRegisterDerivedStrings(flimflam::kEntriesProperty,
- &Profile::EnumerateEntries,
- NULL);
+ HelpRegisterConstDerivedStrings(flimflam::kServicesProperty,
+ &Profile::EnumerateAvailableServices);
+ HelpRegisterConstDerivedStrings(flimflam::kEntriesProperty,
+ &Profile::EnumerateEntries);
}
Profile::~Profile() {}
@@ -380,13 +378,12 @@
return false;
}
-void Profile::HelpRegisterDerivedStrings(
+void Profile::HelpRegisterConstDerivedStrings(
const string &name,
- Strings(Profile::*get)(Error *),
- void(Profile::*set)(const Strings&, Error *)) {
+ Strings(Profile::*get)(Error *)) {
store_.RegisterDerivedStrings(
name,
- StringsAccessor(new CustomAccessor<Profile, Strings>(this, get, set)));
+ StringsAccessor(new CustomAccessor<Profile, Strings>(this, get, NULL)));
}
} // namespace shill
diff --git a/profile.h b/profile.h
index 4020e4a..d3b72d6 100644
--- a/profile.h
+++ b/profile.h
@@ -204,10 +204,9 @@
static bool IsValidIdentifierToken(const std::string &token);
- void HelpRegisterDerivedStrings(
+ void HelpRegisterConstDerivedStrings(
const std::string &name,
- Strings(Profile::*get)(Error *error),
- void(Profile::*set)(const Strings&, Error *error));
+ Strings(Profile::*get)(Error *error));
// Data members shared with subclasses via getter/setters above in the
// protected: section
diff --git a/property_accessor.h b/property_accessor.h
index 58c085e..fb0d6df 100644
--- a/property_accessor.h
+++ b/property_accessor.h
@@ -32,6 +32,16 @@
// new_foo = accessors["foo"]->Get(); // new_foo == false
// // Clear resets |foo| to its value when the PropertyAccessor was created.
// accessors["foo"]->Clear(); // foo == true
+//
+// Generic accessors that provide write capability will check that the
+// new value differs from the present one. If the old and new values
+// are the same, the setter will not invoke the assignment operator, and
+// will return false.
+//
+// Custom accessors are responsible for handling set-to-same-value
+// themselves. It is not possible to handle that here, because some
+// custom getters return default values, rather than the actual
+// value. (I'm looking at you, WiFi::GetBgscanMethod.)
template <class T>
class PropertyAccessor : public AccessorInterface<T> {
public:
@@ -43,8 +53,12 @@
void Clear(Error *error) { Set(default_value_, error); }
T Get(Error */*error*/) { return *property_; }
- void Set(const T &value, Error */*error*/) {
+ bool Set(const T &value, Error */*error*/) {
+ if (*property_ == value) {
+ return false;
+ }
*property_ = value;
+ return true;
}
private:
@@ -67,10 +81,11 @@
error->Populate(Error::kInvalidArguments, "Property is read-only");
}
T Get(Error */*error*/) { return *property_; }
- void Set(const T &/*value*/, Error *error) {
+ bool Set(const T &/*value*/, Error *error) {
// TODO(quiche): check if this is the right error.
// (maybe Error::kPermissionDenied instead?)
error->Populate(Error::kInvalidArguments, "Property is read-only");
+ return false;
}
private:
@@ -92,8 +107,12 @@
error->Populate(Error::kPermissionDenied, "Property is write-only");
return T();
}
- void Set(const T &value, Error */*error*/) {
+ bool Set(const T &value, Error */*error*/) {
+ if (*property_ == value) {
+ return false;
+ }
*property_ = value;
+ return true;
}
private:
@@ -117,11 +136,12 @@
public:
// |target| is the object on which to call the methods |getter| and |setter|
// |setter| is allowed to be NULL, in which case we will simply reject
- // attempts to set via the accessor.
+ // attempts to set via the accessor. |setter| should return true if the
+ // value was changed, and false otherwise.
// It is an error to pass NULL for either of the other two arguments.
CustomAccessor(C *target,
T(C::*getter)(Error *error),
- void(C::*setter)(const T &value, Error *error))
+ bool(C::*setter)(const T &value, Error *error))
: target_(target),
default_value_(),
getter_(getter),
@@ -139,11 +159,12 @@
T Get(Error *error) {
return (target_->*getter_)(error);
}
- void Set(const T &value, Error *error) {
+ bool Set(const T &value, Error *error) {
if (setter_) {
- (target_->*setter_)(value, error);
+ return (target_->*setter_)(value, error);
} else {
error->Populate(Error::kInvalidArguments, "Property is read-only");
+ return false;
}
}
@@ -153,7 +174,7 @@
// the initializer list.
T default_value_;
T(C::*const getter_)(Error *error);
- void(C::*const setter_)(const T &value, Error *error);
+ bool(C::*const setter_)(const T &value, Error *error);
DISALLOW_COPY_AND_ASSIGN(CustomAccessor);
};
@@ -165,12 +186,13 @@
public:
// |target| is the object on which to call |setter| and |clearer|.
//
- // |target| and |setter| must be non-NULL.
+ // |target| and |setter| must be non-NULL. |setter| should return true
+ // if the value was changed, and false otherwise.
//
// Either |clearer| or |default_value|, but not both, must be non-NULL.
// Whichever is non-NULL is used to clear the property.
CustomWriteOnlyAccessor(C *target,
- void(C::*setter)(const T &value, Error *error),
+ bool(C::*setter)(const T &value, Error *error),
void(C::*clearer)(Error *error),
const T *default_value)
: target_(target),
@@ -198,13 +220,13 @@
error->Populate(Error::kPermissionDenied, "Property is write-only");
return T();
}
- void Set(const T &value, Error *error) {
- (target_->*setter_)(value, error);
+ bool Set(const T &value, Error *error) {
+ return (target_->*setter_)(value, error);
}
private:
C *const target_;
- void(C::*const setter_)(const T &value, Error *error);
+ bool(C::*const setter_)(const T &value, Error *error);
void(C::*const clearer_)(Error *error);
// |default_value_| is non-const because it can't be initialized in
// the initializer list.
@@ -220,14 +242,15 @@
public:
// |target| is the object on which to call the methods |getter| and |setter|.
// |setter| is allowed to be NULL, in which case we will simply reject
- // attempts to set via the accessor.
+ // attempts to set via the accessor. |setter| should return true if the
+ // value was changed, and false otherwise.
// |argument| is passed to the getter and setter methods to disambiguate
// between different properties in |target|.
// It is an error to pass NULL for any of |target|, |clearer| or |getter|.
CustomMappedAccessor(C *target,
void(C::*clearer)(const A &argument, Error *error),
T(C::*getter)(const A &argument, Error *error),
- void(C::*setter)(const A &argument, const T &value,
+ bool(C::*setter)(const A &argument, const T &value,
Error *error),
const A &argument)
: target_(target),
@@ -247,11 +270,12 @@
T Get(Error *error) {
return (target_->*getter_)(argument_, error);
}
- void Set(const T &value, Error *error) {
+ bool Set(const T &value, Error *error) {
if (setter_) {
- (target_->*setter_)(argument_, value, error);
+ return (target_->*setter_)(argument_, value, error);
} else {
error->Populate(Error::kInvalidArguments, "Property is read-only");
+ return false;
}
}
@@ -259,7 +283,7 @@
C *const target_;
void(C::*const clearer_)(const A &argument, Error *error);
T(C::*const getter_)(const A &argument, Error *error);
- void(C::*const setter_)(const A &argument, const T &value, Error *error);
+ bool(C::*const setter_)(const A &argument, const T &value, Error *error);
A argument_;
DISALLOW_COPY_AND_ASSIGN(CustomMappedAccessor);
};
diff --git a/property_accessor_unittest.cc b/property_accessor_unittest.cc
index 062f4f1..69e16be 100644
--- a/property_accessor_unittest.cc
+++ b/property_accessor_unittest.cc
@@ -33,9 +33,13 @@
EXPECT_EQ(int_store, accessor->Get(&error));
int32 expected_int32 = 127;
- accessor->Set(expected_int32, &error);
+ EXPECT_TRUE(accessor->Set(expected_int32, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_int32, accessor->Get(&error));
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor->Set(expected_int32, &error));
+ EXPECT_TRUE(error.IsSuccess());
accessor->Clear(&error);
EXPECT_TRUE(error.IsSuccess());
@@ -75,9 +79,14 @@
Error error;
int32 expected_int32 = 127;
WriteOnlyPropertyAccessor<int32> accessor(&int_store);
- accessor.Set(expected_int32, &error);
+ EXPECT_TRUE(accessor.Set(expected_int32, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_int32, *accessor.property_);
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor.Set(expected_int32, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // As a write-only, the value can't be read.
EXPECT_EQ(int32(), accessor.Get(&error));
ASSERT_FALSE(error.IsSuccess());
@@ -89,7 +98,7 @@
int32 orig_value = int_store = 0;
WriteOnlyPropertyAccessor<int32> accessor(&int_store);
- accessor.Set(127, &error);
+ EXPECT_TRUE(accessor.Set(127, &error));
accessor.Clear(&error);
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(orig_value, *accessor.property_);
@@ -105,9 +114,13 @@
EXPECT_EQ(int_store, accessor->Get(&error));
uint32 expected_uint32 = 127;
- accessor->Set(expected_uint32, &error);
+ EXPECT_TRUE(accessor->Set(expected_uint32, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_uint32, accessor->Get(&error));
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor->Set(expected_uint32, &error));
+ EXPECT_TRUE(error.IsSuccess());
accessor->Clear(&error);
EXPECT_TRUE(error.IsSuccess());
@@ -122,7 +135,7 @@
EXPECT_EQ(int_store, accessor->Get(&error));
uint32 expected_uint32 = 127;
- accessor->Set(expected_uint32, &error);
+ EXPECT_FALSE(accessor->Set(expected_uint32, &error));
ASSERT_FALSE(error.IsSuccess());
EXPECT_EQ(Error::kInvalidArguments, error.type());
EXPECT_EQ(int_store, accessor->Get(&error));
@@ -147,9 +160,14 @@
Error error;
uint32 expected_uint32 = 127;
WriteOnlyPropertyAccessor<uint32> accessor(&int_store);
- accessor.Set(expected_uint32, &error);
+ EXPECT_TRUE(accessor.Set(expected_uint32, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_uint32, *accessor.property_);
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor.Set(expected_uint32, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // As a write-only, the value can't be read.
EXPECT_EQ(uint32(), accessor.Get(&error));
ASSERT_FALSE(error.IsSuccess());
@@ -161,7 +179,7 @@
uint32 orig_value = int_store = 0;
WriteOnlyPropertyAccessor<uint32> accessor(&int_store);
- accessor.Set(127, &error);
+ EXPECT_TRUE(accessor.Set(127, &error));
accessor.Clear(&error);
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(orig_value, *accessor.property_);
@@ -177,9 +195,13 @@
EXPECT_EQ(storage, accessor->Get(&error));
string expected_string("what");
- accessor->Set(expected_string, &error);
+ EXPECT_TRUE(accessor->Set(expected_string, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_string, accessor->Get(&error));
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor->Set(expected_string, &error));
+ EXPECT_TRUE(error.IsSuccess());
accessor->Clear(&error);
EXPECT_TRUE(error.IsSuccess());
@@ -194,7 +216,7 @@
EXPECT_EQ(storage, accessor->Get(&error));
string expected_string("what");
- accessor->Set(expected_string, &error);
+ EXPECT_FALSE(accessor->Set(expected_string, &error));
ASSERT_FALSE(error.IsSuccess());
EXPECT_EQ(Error::kInvalidArguments, error.type());
EXPECT_EQ(storage, accessor->Get(&error));
@@ -218,21 +240,26 @@
{
Error error;
string expected_string = "what";
- WriteOnlyPropertyAccessor<string> accessor(&expected_string);
- accessor.Set(expected_string, &error);
+ WriteOnlyPropertyAccessor<string> accessor(&storage);
+ EXPECT_TRUE(accessor.Set(expected_string, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_string, *accessor.property_);
+ // Resetting to the same value should return false, but without
+ // an error.
+ EXPECT_FALSE(accessor.Set(expected_string, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // As a write-only, the value can't be read.
EXPECT_EQ(string(), accessor.Get(&error));
ASSERT_FALSE(error.IsSuccess());
- expected_string = "nooooo";
+ storage = "nooooo";
EXPECT_EQ("nooooo", *accessor.property_);
}
{
Error error;
string orig_value = storage = "original value";
WriteOnlyPropertyAccessor<string> accessor(&storage);
- accessor.Set("new value", &error);
+ EXPECT_TRUE(accessor.Set("new value", &error));
accessor.Clear(&error);
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(orig_value, *accessor.property_);
@@ -244,8 +271,12 @@
string Get(Error */*error*/) {
return value_;
}
- void Set(const string &value, Error */*error*/) {
+ bool Set(const string &value, Error */*error*/) {
+ if (value_ == value) {
+ return false;
+ }
value_ = value;
+ return true;
}
void Clear(Error */*error*/) {
value_.clear();
@@ -257,7 +288,9 @@
TEST(PropertyAccessorTest, CustomAccessorCorrectness) {
StringWrapper wrapper;
{
- // Custom accessor: read, write, clear, read-updated.
+ // Custom accessor: read, write, write-same, clear, read-updated.
+ // Together, write and write-same verify that the CustomAccessor
+ // template passes through the value from the called function.
Error error;
const string orig_value = wrapper.value_ = "original value";
CustomAccessor<StringWrapper, string> accessor(&wrapper,
@@ -267,9 +300,12 @@
EXPECT_TRUE(error.IsSuccess());
const string expected_string = "new value";
- accessor.Set(expected_string, &error);
+ EXPECT_TRUE(accessor.Set(expected_string, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_string, accessor.Get(&error));
+ // Set to same value.
+ EXPECT_FALSE(accessor.Set(expected_string, &error));
+ EXPECT_TRUE(error.IsSuccess());
accessor.Clear(&error);
EXPECT_TRUE(error.IsSuccess());
@@ -287,7 +323,7 @@
EXPECT_EQ(wrapper.value_, accessor.Get(&error));
const string expected_string = "what";
- accessor.Set(expected_string, &error);
+ EXPECT_FALSE(accessor.Set(expected_string, &error));
ASSERT_FALSE(error.IsSuccess());
EXPECT_EQ(Error::kInvalidArguments, error.type());
EXPECT_EQ(wrapper.value_, accessor.Get(&error));
@@ -326,9 +362,14 @@
const string expected_string = "what";
CustomWriteOnlyAccessor<StringWrapper, string> accessor(
&wrapper, &StringWrapper::Set, NULL, &default_value);
- accessor.Set(expected_string, &error);
+ EXPECT_TRUE(accessor.Set(expected_string, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_string, wrapper.value_);
+ // Set to same value. With the above, this verifies that the
+ // CustomWriteOnlyAccessor template passes through the return
+ // value.
+ EXPECT_FALSE(accessor.Set(expected_string, &error));
+ EXPECT_TRUE(error.IsSuccess());
}
{
// Test clearing.
@@ -362,16 +403,21 @@
const string expected_string = "what";
CustomWriteOnlyAccessor<StringWrapper, string> accessor(
&wrapper, &StringWrapper::Set, &StringWrapper::Clear, NULL);
- accessor.Set(expected_string, &error);
+ EXPECT_TRUE(accessor.Set(expected_string, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(expected_string, wrapper.value_);
+ // Set to same value. With the above, this verifies that the
+ // CustomWriteOnlyAccessor template passes through the return
+ // value.
+ EXPECT_FALSE(accessor.Set(expected_string, &error));
+ EXPECT_TRUE(error.IsSuccess());
}
{
// Test clearing.
Error error;
CustomWriteOnlyAccessor<StringWrapper, string> accessor(
&wrapper, &StringWrapper::Set, &StringWrapper::Clear, NULL);
- accessor.Set("new value", &error);
+ EXPECT_TRUE(accessor.Set("new value", &error));
EXPECT_EQ("new value", wrapper.value_);
accessor.Clear(&error);
EXPECT_TRUE(error.IsSuccess());
@@ -388,8 +434,12 @@
EXPECT_TRUE(ContainsKey(value_, key));
return value_[key];
}
- void Set(const string &key, const string &value, Error */*error*/) {
+ bool Set(const string &key, const string &value, Error */*error*/) {
+ if (value_[key] == value) {
+ return false;
+ }
value_[key] = value;
+ return true;
}
map<string,string> value_;
@@ -416,9 +466,14 @@
&wrapper, &StringMapWrapper::Clear, &StringMapWrapper::Get,
&StringMapWrapper::Set, kKey);
Error error;
- accessor.Set(kValue, &error);
+ EXPECT_TRUE(accessor.Set(kValue, &error));
EXPECT_TRUE(error.IsSuccess());
EXPECT_EQ(kValue, wrapper.value_[kKey]);
+ // Set to same value. With the above, this verifies that the
+ // CustomMappedAccessor template passes through the return
+ // value.
+ EXPECT_FALSE(accessor.Set(kValue, &error));
+ EXPECT_TRUE(error.IsSuccess());
}
{
// Test clearing.
diff --git a/property_store.cc b/property_store.cc
index 6b2e449..df6359d 100644
--- a/property_store.cc
+++ b/property_store.cc
@@ -23,6 +23,9 @@
PropertyStore::PropertyStore() {}
+PropertyStore::PropertyStore(PropertyChangeCallback on_property_changed) :
+ property_changed_callback_(on_property_changed) {}
+
PropertyStore::~PropertyStore() {}
bool PropertyStore::Contains(const string &prop) const {
@@ -155,6 +158,14 @@
"a string map");
}
+bool PropertyStore::SetStringmapsProperty(
+ const string &name,
+ const vector<map<string, string> > &values,
+ Error *error) {
+ return SetProperty(name, values, error, stringmaps_properties_,
+ "a stringmaps");
+}
+
bool PropertyStore::SetStringsProperty(const string &name,
const vector<string> &values,
Error *error) {
@@ -227,6 +238,11 @@
error->Populate(
Error::kInvalidProperty, "Property " + name + " does not exist.");
}
+ if (error->IsSuccess()) {
+ if (!property_changed_callback_.is_null()) {
+ property_changed_callback_.Run(name);
+ }
+ }
return error->IsSuccess();
}
@@ -618,10 +634,16 @@
Error *error,
map< string, std::tr1::shared_ptr< AccessorInterface<V> > >&collection,
const string &value_type_english) {
+ bool ret = false;
SLOG(Property, 2) << "Setting " << name << " as " << value_type_english
<< ".";
if (ContainsKey(collection, name)) {
- collection[name]->Set(value, error);
+ ret = collection[name]->Set(value, error);
+ if (ret) {
+ if (!property_changed_callback_.is_null()) {
+ property_changed_callback_.Run(name);
+ }
+ }
} else {
if (Contains(name)) {
error->Populate(
@@ -632,7 +654,7 @@
Error::kInvalidProperty, "Property " + name + " does not exist.");
}
}
- return error->IsSuccess();
+ return ret;
};
} // namespace shill
diff --git a/property_store.h b/property_store.h
index 56ade72..603c040 100644
--- a/property_store.h
+++ b/property_store.h
@@ -10,6 +10,7 @@
#include <vector>
#include <base/basictypes.h>
+#include <base/callback.h>
#include "shill/accessor_interface.h"
#include "shill/property_iterator.h"
@@ -20,7 +21,9 @@
class PropertyStore {
public:
+ typedef base::Callback<void(const std::string &)> PropertyChangeCallback;
PropertyStore();
+ explicit PropertyStore(PropertyChangeCallback property_change_callback);
virtual ~PropertyStore();
virtual bool Contains(const std::string& property) const;
@@ -59,9 +62,12 @@
// Methods to allow the setting, by name, of properties stored in this object.
// The property names are declared in chromeos/dbus/service_constants.h,
// so that they may be shared with libcros.
- // Upon success, these methods return true and leave |error| untouched.
- // Upon failure, they return false and set |error| appropriately, if it
- // is non-NULL.
+ // If the property is successfully changed, these methods return true,
+ // and leave |error| untouched.
+ // If the property is unchanged because it already has the desired value,
+ // these methods return false, and leave |error| untouched.
+ // If the property change fails, these methods return false, and update
+ // |error|. However, updating |error| is skipped if |error| is NULL.
virtual bool SetBoolProperty(const std::string &name,
bool value,
Error *error);
@@ -83,6 +89,11 @@
const std::map<std::string, std::string> &values,
Error *error);
+ virtual bool SetStringmapsProperty(
+ const std::string &name,
+ const std::vector<std::map<std::string, std::string> > &values,
+ Error *error);
+
virtual bool SetStringsProperty(const std::string &name,
const std::vector<std::string> &values,
Error *error);
@@ -241,6 +252,8 @@
std::map<std::string, Uint32Accessor> uint32_properties_;
std::map<std::string, Uint64Accessor> uint64_properties_;
+ PropertyChangeCallback property_changed_callback_;
+
DISALLOW_COPY_AND_ASSIGN(PropertyStore);
};
diff --git a/property_store_unittest.cc b/property_store_unittest.cc
index b4b29f8..2030c9e 100644
--- a/property_store_unittest.cc
+++ b/property_store_unittest.cc
@@ -22,9 +22,12 @@
#include "shill/property_accessor.h"
#include "shill/property_store.h"
+using base::Bind;
+using base::Unretained;
using std::map;
using std::string;
using std::vector;
+using ::testing::_;
using ::testing::Values;
namespace shill {
@@ -86,10 +89,13 @@
ASSERT_FALSE(storage_path().empty());
}
-TEST_P(PropertyStoreTest, TestProperty) {
- // Ensure that an attempt to write unknown properties returns InvalidProperty.
- PropertyStore store;
+TEST_P(PropertyStoreTest, SetPropertyNonexistent) {
+ // Ensure that an attempt to write unknown properties returns
+ // InvalidProperty, and does not yield a PropertyChange callback.
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
::DBus::Error error;
+ EXPECT_CALL(*this, TestCallback(_)).Times(0);
EXPECT_FALSE(DBusAdaptor::SetProperty(&store, "", GetParam(), &error));
EXPECT_EQ(invalid_prop(), error.name());
}
@@ -113,22 +119,44 @@
protected:
void RegisterProperty(
PropertyStore &store, const string &name, T *storage);
+ bool SetProperty(
+ PropertyStore &store, const string &name, Error *error);
};
typedef ::testing::Types<
- bool, int16, int32, KeyValueStore, string, Stringmap, Stringmaps, Strings,
- uint8, uint16, uint32> PropertyTypes;
+ bool, int16, int32, string, Stringmap, Stringmaps, Strings, uint8, uint16,
+ uint32> PropertyTypes;
TYPED_TEST_CASE(PropertyStoreTypedTest, PropertyTypes);
TYPED_TEST(PropertyStoreTypedTest, ClearProperty) {
- PropertyStore store;
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
Error error;
TypeParam property;
+
// |this| required due to two-phase lookup.
this->RegisterProperty(store, "some property", &property);
+ EXPECT_CALL(*this, TestCallback(_));
EXPECT_TRUE(store.ClearProperty("some property", &error));
}
+TYPED_TEST(PropertyStoreTypedTest, SetProperty) {
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
+ Error error;
+ TypeParam property = TypeParam(); // value-initialize primitives
+
+ // |this| required due to two-phase lookup.
+ this->RegisterProperty(store, "some property", &property);
+
+ // Change the value from the default (initialized above). Should
+ // generate a change callback. The second SetProperty, however,
+ // should not. Hence, we should get exactly one callback.
+ EXPECT_CALL(*this, TestCallback(_)).Times(1);
+ EXPECT_TRUE(this->SetProperty(store, "some property", &error));
+ EXPECT_FALSE(this->SetProperty(store, "some property", &error));
+}
+
template<> void PropertyStoreTypedTest<bool>::RegisterProperty(
PropertyStore &store, const string &name, bool *storage) {
store.RegisterBool(name, storage);
@@ -144,16 +172,6 @@
store.RegisterInt32(name, storage);
}
-template<> void PropertyStoreTypedTest<KeyValueStore>::RegisterProperty(
- PropertyStore &store, const string &name, KeyValueStore *storage) {
- // We use |RegisterDerivedKeyValueStore|, because there is no non-derived
- // version. (And it's not clear that we'll need one, outside of this
- // test.)
- store.RegisterDerivedKeyValueStore(
- name, KeyValueStoreAccessor(
- new PropertyAccessor<KeyValueStore>(storage)));
-}
-
template<> void PropertyStoreTypedTest<string>::RegisterProperty(
PropertyStore &store, const string &name, string *storage) {
store.RegisterString(name, storage);
@@ -189,6 +207,69 @@
store.RegisterUint32(name, storage);
}
+template<> bool PropertyStoreTypedTest<bool>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ bool new_value = true;
+ return store.SetBoolProperty(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<int16>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ int16 new_value = 1;
+ return store.SetInt16Property(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<int32>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ int32 new_value = 1;
+ return store.SetInt32Property(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<string>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ string new_value = "new value";
+ return store.SetStringProperty(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<Stringmap>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ Stringmap new_value;
+ new_value["new key"] = "new value";
+ return store.SetStringmapProperty(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<Stringmaps>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ Stringmaps new_value(1);
+ new_value[0]["new key"] = "new value";
+ return store.SetStringmapsProperty(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<Strings>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ Strings new_value(1);
+ new_value[0] = "new value";
+ return store.SetStringsProperty(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<uint8>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ uint8 new_value = 1;
+ return store.SetUint8Property(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<uint16>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ uint16 new_value = 1;
+ return store.SetUint16Property(name, new_value, error);
+}
+
+template<> bool PropertyStoreTypedTest<uint32>::SetProperty(
+ PropertyStore &store, const string &name, Error *error) {
+ uint32 new_value = 1;
+ return store.SetUint32Property(name, new_value, error);
+}
+
TEST_F(PropertyStoreTest, ClearBoolProperty) {
// We exercise both possibilities for the default value here,
// to ensure that Clear actually resets the property based on
@@ -209,24 +290,34 @@
}
TEST_F(PropertyStoreTest, ClearPropertyNonexistent) {
- PropertyStore store;
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
Error error;
+ EXPECT_CALL(*this, TestCallback(_)).Times(0);
EXPECT_FALSE(store.ClearProperty("", &error));
EXPECT_EQ(Error::kInvalidProperty, error.type());
}
+// Separate from SetPropertyNonexistent, because
+// DBusAdaptor::SetProperty doesn't support Stringmaps.
TEST_F(PropertyStoreTest, SetStringmapsProperty) {
- PropertyStore store;
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
::DBus::Error error;
+ EXPECT_CALL(*this, TestCallback(_)).Times(0);
EXPECT_FALSE(DBusAdaptor::SetProperty(
&store, "", PropertyStoreTest::kStringmapsV, &error));
EXPECT_EQ(internal_error(), error.name());
}
+// Separate from SetPropertyNonexistent, because
+// DBusAdaptor::SetProperty doesn't support KeyValueStore.
TEST_F(PropertyStoreTest, SetKeyValueStoreProperty) {
- PropertyStore store;
+ PropertyStore store(Bind(&PropertyStoreTest::TestCallback,
+ Unretained(this)));
::DBus::Error error;
+ EXPECT_CALL(*this, TestCallback(_)).Times(0);
EXPECT_FALSE(DBusAdaptor::SetProperty(
&store, "", PropertyStoreTest::kKeyValueStoreV, &error));
EXPECT_EQ(internal_error(), error.name());
diff --git a/property_store_unittest.h b/property_store_unittest.h
index 344080d..fb9c45a 100644
--- a/property_store_unittest.h
+++ b/property_store_unittest.h
@@ -50,6 +50,7 @@
virtual ~PropertyStoreTest();
virtual void SetUp();
+ MOCK_METHOD1(TestCallback, void(const std::string &property_name));
protected:
Manager *manager() { return &manager_; }
diff --git a/refptr_types.h b/refptr_types.h
index 5478c1a..cf951e1 100644
--- a/refptr_types.h
+++ b/refptr_types.h
@@ -76,6 +76,7 @@
typedef scoped_refptr<DHCPConfig> DHCPConfigRefPtr;
class Profile;
+typedef scoped_refptr<const Profile> ProfileConstRefPtr;
typedef scoped_refptr<Profile> ProfileRefPtr;
class Connection;
diff --git a/service.cc b/service.cc
index 4aeb44e..f527c83 100644
--- a/service.cc
+++ b/service.cc
@@ -105,7 +105,8 @@
Metrics *metrics,
Manager *manager,
Technology::Identifier technology)
- : state_(kStateIdle),
+ : weak_ptr_factory_(this),
+ state_(kStateIdle),
previous_state_(kStateIdle),
failure_(kFailureUnknown),
auto_connect_(false),
@@ -125,6 +126,9 @@
failed_time_(0),
has_ever_connected_(false),
auto_connect_cooldown_milliseconds_(0),
+ store_(PropertyStore::PropertyChangeCallback(
+ base::Bind(&Service::OnPropertyChanged,
+ weak_ptr_factory_.GetWeakPtr()))),
dispatcher_(dispatcher),
unique_name_(base::UintToString(serial_number_++)),
friendly_name_(unique_name_),
@@ -132,7 +136,6 @@
metrics_(metrics),
manager_(manager),
sockets_(new Sockets()),
- weak_ptr_factory_(this),
time_(Time::GetInstance()),
diagnostics_reporter_(DiagnosticsReporter::GetInstance()) {
HelpRegisterDerivedBool(flimflam::kAutoConnectProperty,
@@ -153,9 +156,8 @@
&Service::GetCheckPortal,
&Service::SetCheckPortal);
store_.RegisterConstBool(flimflam::kConnectableProperty, &connectable_);
- HelpRegisterDerivedRpcIdentifier(flimflam::kDeviceProperty,
- &Service::GetDeviceRpcId,
- NULL);
+ HelpRegisterConstDerivedRpcIdentifier(flimflam::kDeviceProperty,
+ &Service::GetDeviceRpcId);
store_.RegisterConstStrings(kEapRemoteCertificationProperty,
&remote_certification_);
HelpRegisterDerivedString(flimflam::kGuidProperty,
@@ -168,12 +170,10 @@
store_.RegisterConstString(flimflam::kErrorProperty, &error_);
store_.RegisterConstString(shill::kErrorDetailsProperty, &error_details_);
store_.RegisterConstBool(flimflam::kFavoriteProperty, &favorite_);
- HelpRegisterDerivedUint16(shill::kHTTPProxyPortProperty,
- &Service::GetHTTPProxyPort,
- NULL);
- HelpRegisterDerivedRpcIdentifier(shill::kIPConfigProperty,
- &Service::GetIPConfigRpcIdentifier,
- NULL);
+ HelpRegisterConstDerivedUint16(shill::kHTTPProxyPortProperty,
+ &Service::GetHTTPProxyPort);
+ HelpRegisterConstDerivedRpcIdentifier(shill::kIPConfigProperty,
+ &Service::GetIPConfigRpcIdentifier);
HelpRegisterDerivedBool(flimflam::kIsActiveProperty,
&Service::IsActive,
NULL);
@@ -927,6 +927,9 @@
SLOG(Service, 2) << "SetProfile from "
<< (profile_ ? profile_->GetFriendlyName() : "")
<< " to " << (p ? p->GetFriendlyName() : "");
+ if (profile_ == p) {
+ return;
+ }
profile_ = p;
Error error;
string profile_rpc_id = GetProfileRpcId(&error);
@@ -1066,7 +1069,7 @@
void Service::HelpRegisterDerivedBool(
const string &name,
bool(Service::*get)(Error *),
- void(Service::*set)(const bool&, Error *)) {
+ bool(Service::*set)(const bool&, Error *)) {
store_.RegisterDerivedBool(
name,
BoolAccessor(new CustomAccessor<Service, bool>(this, get, set)));
@@ -1075,7 +1078,7 @@
void Service::HelpRegisterDerivedInt32(
const string &name,
int32(Service::*get)(Error *),
- void(Service::*set)(const int32&, Error *)) {
+ bool(Service::*set)(const int32&, Error *)) {
store_.RegisterDerivedInt32(
name,
Int32Accessor(new CustomAccessor<Service, int32>(this, get, set)));
@@ -1084,29 +1087,27 @@
void Service::HelpRegisterDerivedString(
const string &name,
string(Service::*get)(Error *),
- void(Service::*set)(const string&, Error *)) {
+ bool(Service::*set)(const string&, Error *)) {
store_.RegisterDerivedString(
name,
StringAccessor(new CustomAccessor<Service, string>(this, get, set)));
}
-void Service::HelpRegisterDerivedRpcIdentifier(
+void Service::HelpRegisterConstDerivedRpcIdentifier(
const string &name,
- RpcIdentifier(Service::*get)(Error *),
- void(Service::*set)(const RpcIdentifier&, Error *)) {
+ RpcIdentifier(Service::*get)(Error *)) {
store_.RegisterDerivedRpcIdentifier(
name,
RpcIdentifierAccessor(new CustomAccessor<Service, RpcIdentifier>(
- this, get, set)));
+ this, get, NULL)));
}
-void Service::HelpRegisterDerivedUint16(
+void Service::HelpRegisterConstDerivedUint16(
const string &name,
- uint16(Service::*get)(Error *),
- void(Service::*set)(const uint16&, Error *)) {
+ uint16(Service::*get)(Error *)) {
store_.RegisterDerivedUint16(
name,
- Uint16Accessor(new CustomAccessor<Service, uint16>(this, get, set)));
+ Uint16Accessor(new CustomAccessor<Service, uint16>(this, get, NULL)));
}
void Service::HelpRegisterConstDerivedStrings(
@@ -1156,24 +1157,22 @@
return auto_connect();
}
-void Service::SetAutoConnectFull(const bool &connect, Error */*error*/) {
+bool Service::SetAutoConnectFull(const bool &connect, Error */*error*/) {
LOG(INFO) << "Service " << unique_name() << ": AutoConnect="
<< auto_connect() << "->" << connect;
if (auto_connect() == connect) {
- return;
+ return false;
}
SetAutoConnect(connect);
manager_->UpdateService(this);
+ return true;
}
string Service::GetCheckPortal(Error *error) {
return check_portal_;
}
-void Service::SetCheckPortal(const string &check_portal, Error *error) {
- if (check_portal == check_portal_) {
- return;
- }
+bool Service::SetCheckPortal(const string &check_portal, Error *error) {
if (check_portal != kCheckPortalFalse &&
check_portal != kCheckPortalTrue &&
check_portal != kCheckPortalAuto) {
@@ -1181,21 +1180,26 @@
base::StringPrintf(
"Invalid Service CheckPortal property value: %s",
check_portal.c_str()));
- return;
+ return false;
+ }
+ if (check_portal == check_portal_) {
+ return false;
}
check_portal_ = check_portal;
+ return true;
}
string Service::GetGuid(Error *error) {
return guid_;
}
-void Service::SetGuid(const string &guid, Error */*error*/) {
- if (guid == guid_) {
- return;
+bool Service::SetGuid(const string &guid, Error */*error*/) {
+ if (guid_ == guid) {
+ return false;
}
guid_ = guid;
adaptor_->EmitStringChanged(flimflam::kGuidProperty, guid_);
+ return true;
}
void Service::MarkAsFavorite() {
@@ -1214,25 +1218,28 @@
return friendly_name_;
}
-void Service::SetNameProperty(const string &name, Error *error) {
+bool Service::SetNameProperty(const string &name, Error *error) {
if (name != friendly_name_) {
Error::PopulateAndLog(error, Error::kInvalidArguments,
base::StringPrintf(
"Service %s Name property cannot be modified.",
unique_name_.c_str()));
+ return false;
}
+ return false;
}
int32 Service::GetPriority(Error *error) {
return priority_;
}
-void Service::SetPriority(const int32 &priority, Error *error) {
- if (priority == priority_) {
- return;
+bool Service::SetPriority(const int32 &priority, Error *error) {
+ if (priority_ == priority) {
+ return false;
}
priority_ = priority;
adaptor_->EmitIntChanged(flimflam::kPriorityProperty, priority_);
+ return true;
}
string Service::GetProfileRpcId(Error *error) {
@@ -1244,10 +1251,17 @@
return profile_->GetRpcIdentifier();
}
-void Service::SetProfileRpcId(const string &profile, Error *error) {
+bool Service::SetProfileRpcId(const string &profile, Error *error) {
+ if (profile_ && profile_->GetRpcIdentifier() == profile) {
+ return false;
+ }
+ ProfileConstRefPtr old_profile = profile_;
+ // No need to Emit afterwards, since SetProfileForService will call
+ // into SetProfile (if the profile actually changes).
manager_->SetProfileForService(this, profile, error);
- // No need to Emit here, since SetProfileForService will call into
- // SetProfile (if the profile actually changes).
+ // Can't just use error.IsSuccess(), because that also requires saving
+ // the profile to succeed. (See Profile::AdoptService)
+ return (profile_ != old_profile);
}
uint16 Service::GetHTTPProxyPort(Error */*error*/) {
@@ -1261,9 +1275,12 @@
return proxy_config_;
}
-void Service::SetProxyConfig(const string &proxy_config, Error *error) {
+bool Service::SetProxyConfig(const string &proxy_config, Error *error) {
+ if (proxy_config_ == proxy_config)
+ return false;
proxy_config_ = proxy_config;
adaptor_->EmitStringChanged(flimflam::kProxyConfigProperty, proxy_config_);
+ return true;
}
// static
diff --git a/service.h b/service.h
index ecffa8f..8dc133c 100644
--- a/service.h
+++ b/service.h
@@ -44,6 +44,7 @@
class KeyValueStore;
class Manager;
class Metrics;
+class MockManager;
class ServiceAdaptorInterface;
class ServiceMockAdaptor;
class Sockets;
@@ -326,12 +327,12 @@
void SetFriendlyName(const std::string &friendly_name);
const std::string &guid() const { return guid_; }
- void SetGuid(const std::string &guid, Error *error);
+ bool SetGuid(const std::string &guid, Error *error);
bool has_ever_connected() const { return has_ever_connected_; }
int32 priority() const { return priority_; }
- void SetPriority(const int32 &priority, Error *error);
+ bool SetPriority(const int32 &priority, Error *error);
size_t crypto_algorithm() const { return crypto_algorithm_; }
bool key_rotation() const { return key_rotation_; }
@@ -466,23 +467,21 @@
void HelpRegisterDerivedBool(
const std::string &name,
bool(Service::*get)(Error *error),
- void(Service::*set)(const bool &value, Error *error));
+ bool(Service::*set)(const bool &value, Error *error));
void HelpRegisterDerivedInt32(
const std::string &name,
int32(Service::*get)(Error *error),
- void(Service::*set)(const int32 &value, Error *error));
+ bool(Service::*set)(const int32 &value, Error *error));
void HelpRegisterDerivedString(
const std::string &name,
std::string(Service::*get)(Error *error),
- void(Service::*set)(const std::string &value, Error *error));
- void HelpRegisterDerivedUint16(
+ bool(Service::*set)(const std::string &value, Error *error));
+ void HelpRegisterConstDerivedUint16(
const std::string &name,
- uint16(Service::*get)(Error *error),
- void(Service::*set)(const uint16 &value, Error *error));
- void HelpRegisterDerivedRpcIdentifier(
+ uint16(Service::*get)(Error *error));
+ void HelpRegisterConstDerivedRpcIdentifier(
const std::string &name,
- std::string(Service::*get)(Error *),
- void(Service::*set)(const RpcIdentifier&, Error *));
+ std::string(Service::*get)(Error *));
void HelpRegisterConstDerivedStrings(
const std::string &name, Strings(Service::*get)(Error *error));
// Expose a property over RPC, with the name |name|.
@@ -513,7 +512,7 @@
// RPC setter for the the "AutoConnect" property. Updates the |manager_|.
// (cf. SetAutoConnect, which does not update the manager.)
- virtual void SetAutoConnectFull(const bool &connect, Error *error);
+ virtual bool SetAutoConnectFull(const bool &connect, Error *error);
// Property accessors reserved for subclasses
EventDispatcher *dispatcher() const { return dispatcher_; }
@@ -532,6 +531,7 @@
void SetSecurity(CryptoAlgorithm crypt, bool rotation, bool endpoint_auth);
private:
+ friend class EthernetEapServiceTest;
friend class EthernetServiceTest;
friend class MetricsTest;
friend class ManagerTest;
@@ -543,6 +543,7 @@
friend class WiMaxProviderTest;
friend class WiMaxServiceTest;
friend void TestCommonPropertyChanges(ServiceRefPtr, ServiceMockAdaptor *);
+ friend void TestCustomSetterNoopChange(ServiceRefPtr, MockManager *);
friend void TestNamePropertyChange(ServiceRefPtr, ServiceMockAdaptor *);
FRIEND_TEST(AllMockServiceTest, AutoConnectWithFailures);
FRIEND_TEST(CellularCapabilityGSMTest, SetStorageIdentifier);
@@ -561,6 +562,7 @@
FRIEND_TEST(ServiceTest, ConfigureEapStringProperty);
FRIEND_TEST(ServiceTest, ConfigureIgnoredProperty);
FRIEND_TEST(ServiceTest, Constructor);
+ FRIEND_TEST(ServiceTest, CustomSetterNoopChange);
FRIEND_TEST(ServiceTest, GetIPConfigRpcIdentifier);
FRIEND_TEST(ServiceTest, GetProperties);
FRIEND_TEST(ServiceTest, IsAutoConnectable);
@@ -617,7 +619,7 @@
bool GetAutoConnect(Error *error);
std::string GetCheckPortal(Error *error);
- void SetCheckPortal(const std::string &check_portal, Error *error);
+ bool SetCheckPortal(const std::string &check_portal, Error *error);
std::string GetGuid(Error *error);
@@ -628,18 +630,18 @@
std::string GetNameProperty(Error *error);
// The base implementation asserts that |name| matches the current Name
// property value.
- virtual void SetNameProperty(const std::string &name, Error *error);
+ virtual bool SetNameProperty(const std::string &name, Error *error);
int32 GetPriority(Error *error);
std::string GetProfileRpcId(Error *error);
- void SetProfileRpcId(const std::string &profile, Error *error);
+ bool SetProfileRpcId(const std::string &profile, Error *error);
// Returns TCP port of service's HTTP proxy in host order.
uint16 GetHTTPProxyPort(Error *error);
std::string GetProxyConfig(Error *error);
- void SetProxyConfig(const std::string &proxy_config, Error *error);
+ bool SetProxyConfig(const std::string &proxy_config, Error *error);
static Strings ExtractWallClockToStrings(
const std::deque<Timestamp> ×tamps);
@@ -676,6 +678,9 @@
// authentication) for comparison.
uint16 SecurityLevel();
+ // WeakPtrFactory comes first, so that other fields can use it.
+ base::WeakPtrFactory<Service> weak_ptr_factory_;
+
ConnectState state_;
ConnectState previous_state_;
ConnectFailure failure_;
@@ -734,7 +739,6 @@
Metrics *metrics_;
Manager *manager_;
scoped_ptr<Sockets> sockets_;
- base::WeakPtrFactory<Service> weak_ptr_factory_;
Time *time_;
DiagnosticsReporter *diagnostics_reporter_;
diff --git a/service_dbus_adaptor.cc b/service_dbus_adaptor.cc
index 9567b9c..d7068bc 100644
--- a/service_dbus_adaptor.cc
+++ b/service_dbus_adaptor.cc
@@ -86,9 +86,6 @@
::DBus::Error &error) {
SLOG(DBus, 2) << __func__ << ": " << name;
DBusAdaptor::SetProperty(service_->mutable_store(), name, value, &error);
- if (!error.is_set()) {
- service_->OnPropertyChanged(name);
- }
}
void ServiceDBusAdaptor::ClearProperty(const string &name,
diff --git a/service_dbus_adaptor.h b/service_dbus_adaptor.h
index c933b7e..80e66f2 100644
--- a/service_dbus_adaptor.h
+++ b/service_dbus_adaptor.h
@@ -29,7 +29,7 @@
public:
static const char kPath[];
- ServiceDBusAdaptor(DBus::Connection* conn, Service *service);
+ ServiceDBusAdaptor(DBus::Connection *conn, Service *service);
virtual ~ServiceDBusAdaptor();
// Implementation of ServiceAdaptorInterface.
diff --git a/service_property_change_test.cc b/service_property_change_test.cc
index 9e08e6f..c2fcfb5 100644
--- a/service_property_change_test.cc
+++ b/service_property_change_test.cc
@@ -12,12 +12,16 @@
#include "shill/error.h"
#include "shill/mock_adaptors.h"
+#include "shill/mock_manager.h"
+#include "shill/mock_profile.h"
+#include "shill/refptr_types.h"
#include "shill/service.h"
using std::string;
using testing::_;
using testing::AnyNumber;
using testing::Mock;
+using testing::NiceMock;
namespace shill {
@@ -116,4 +120,55 @@
Mock::VerifyAndClearExpectations(adaptor);
}
+void TestCustomSetterNoopChange(ServiceRefPtr service,
+ MockManager *mock_manager) {
+ // SetAutoConnectFull
+ {
+ Error error;
+ EXPECT_CALL(*mock_manager, UpdateService(_)).Times(0);
+ EXPECT_FALSE(service->SetAutoConnectFull(service->auto_connect(), &error));
+ EXPECT_TRUE(error.IsSuccess());
+ Mock::VerifyAndClearExpectations(mock_manager);
+ }
+
+ // SetCheckPortal
+ {
+ Error error;
+ EXPECT_FALSE(service->SetCheckPortal(service->check_portal_, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetNameProperty
+ {
+ Error error;
+ EXPECT_FALSE(service->SetNameProperty(service->friendly_name_, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetProfileRpcId
+ {
+ Error error;
+ scoped_refptr<MockProfile> profile(
+ new NiceMock<MockProfile>(static_cast<ControlInterface *>(NULL),
+ static_cast<Metrics *>(NULL),
+ static_cast<Manager *>(NULL)));
+ service->set_profile(profile);
+ EXPECT_FALSE(service->SetProfileRpcId(profile->GetRpcIdentifier(),
+ &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetProxyConfig
+ {
+ Error error;
+ static const string kProxyConfig = "some opaque blob";
+ // Set to known value.
+ EXPECT_TRUE(service->SetProxyConfig(kProxyConfig, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(service->SetProxyConfig(kProxyConfig, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+}
+
} // namespace shill
diff --git a/service_property_change_test.h b/service_property_change_test.h
index ecf8433..e0b4777 100644
--- a/service_property_change_test.h
+++ b/service_property_change_test.h
@@ -9,6 +9,7 @@
namespace shill {
+class MockManager;
class ServiceMockAdaptor;
// Test property change notifications that are implemented by all
@@ -23,7 +24,10 @@
// changing the name property.
void TestNamePropertyChange(ServiceRefPtr service,
ServiceMockAdaptor *adaptor);
-
+// Test that the common customer setters (for all Services) return
+// false if setting to the same as the current value.
+void TestCommonCustomSetterNoopChange(ServiceRefPtr service,
+ MockManager *mock_manager);
} // namespace shill
#endif // SHILL_TEST_COMMON_H_
diff --git a/service_unittest.cc b/service_unittest.cc
index fe78890..262ad6e 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -174,8 +174,8 @@
return service_->GetAutoConnect(error);
}
- void SetAutoConnectFull(bool connect, Error *error) {
- service_->SetAutoConnectFull(connect, error);
+ bool SetAutoConnectFull(bool connect, Error *error) {
+ return service_->SetAutoConnectFull(connect, error);
}
MockManager mock_manager_;
@@ -291,16 +291,20 @@
}
{
::DBus::Error error;
+ ::DBus::Variant priority;
+ priority.writer().append_int32(1);
EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
flimflam::kPriorityProperty,
- PropertyStoreTest::kInt32V,
+ priority,
&error));
}
{
::DBus::Error error;
+ ::DBus::Variant guid;
+ guid.writer().append_string("not default");
EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
flimflam::kGuidProperty,
- PropertyStoreTest::kStringV,
+ guid,
&error));
}
// Ensure that EAP properties cannot be set on services with no EAP
@@ -308,15 +312,19 @@
// ServiceTest::SetUp() that fiddles with service_->eap_.
{
::DBus::Error error;
+ ::DBus::Variant eap;
+ eap.writer().append_string("eap eep eip!");
EXPECT_FALSE(DBusAdaptor::SetProperty(service2_->mutable_store(),
flimflam::kEAPEAPProperty,
- PropertyStoreTest::kStringV,
+ eap,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_prop(), error.name());
+ // Now plumb in eap credentials, and try again.
service2_->SetEapCredentials(new EapCredentials());
EXPECT_TRUE(DBusAdaptor::SetProperty(service2_->mutable_store(),
flimflam::kEAPEAPProperty,
- PropertyStoreTest::kStringV,
+ eap,
&error));
}
// Ensure that an attempt to write a R/O property returns InvalidArgs error.
@@ -326,32 +334,29 @@
flimflam::kFavoriteProperty,
PropertyStoreTest::kBoolV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
{
::DBus::Error error;
+ ::DBus::Variant auto_connect;
+ auto_connect.writer().append_bool(true);
EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
flimflam::kAutoConnectProperty,
- PropertyStoreTest::kBoolV,
+ auto_connect,
&error));
}
- {
- ::DBus::Error error;
- EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
- flimflam::kAutoConnectProperty,
- PropertyStoreTest::kBoolV,
- &error));
- }
// Ensure that we can perform a trivial set of the Name property (to its
// current value) but an attempt to set the property to a different value
// fails.
{
::DBus::Error error;
- EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
- flimflam::kNameProperty,
- DBusAdaptor::StringToVariant(
- GetFriendlyName()),
- &error));
+ EXPECT_FALSE(DBusAdaptor::SetProperty(service_->mutable_store(),
+ flimflam::kNameProperty,
+ DBusAdaptor::StringToVariant(
+ GetFriendlyName()),
+ &error));
+ EXPECT_FALSE(error.is_set());
}
{
::DBus::Error error;
@@ -359,6 +364,7 @@
flimflam::kNameProperty,
PropertyStoreTest::kStringV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
}
@@ -1532,4 +1538,10 @@
TestAutoConnectPropertyChange(service_, GetAdaptor());
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(ServiceTest, CustomSetterNoopChange) {
+ TestCustomSetterNoopChange(service_, &mock_manager_);
+}
+
} // namespace shill
diff --git a/static_ip_parameters.cc b/static_ip_parameters.cc
index 90f4fa5..7842d85 100644
--- a/static_ip_parameters.cc
+++ b/static_ip_parameters.cc
@@ -293,26 +293,38 @@
return saved_args_.GetString(key);
}
-void StaticIPParameters::SetMappedInt32Property(
+bool StaticIPParameters::SetMappedInt32Property(
const size_t &index, const int32 &value, Error *error) {
CHECK(index < arraysize(kProperties));
+ if (args_.ContainsInt(kProperties[index].name) &&
+ args_.GetInt(kProperties[index].name) == value) {
+ return false;
+ }
args_.SetInt(kProperties[index].name, value);
+ return true;
}
-void StaticIPParameters::SetMappedSavedInt32Property(
+bool StaticIPParameters::SetMappedSavedInt32Property(
const size_t &index, const int32 &value, Error *error) {
error->Populate(Error::kInvalidArguments, "Property is read-only");
+ return false;
}
-void StaticIPParameters::SetMappedStringProperty(
+bool StaticIPParameters::SetMappedStringProperty(
const size_t &index, const string &value, Error *error) {
CHECK(index < arraysize(kProperties));
+ if (args_.ContainsString(kProperties[index].name) &&
+ args_.GetString(kProperties[index].name) == value) {
+ return false;
+ }
args_.SetString(kProperties[index].name, value);
+ return true;
}
-void StaticIPParameters::SetMappedSavedStringProperty(
+bool StaticIPParameters::SetMappedSavedStringProperty(
const size_t &index, const string &value, Error *error) {
error->Populate(Error::kInvalidArguments, "Property is read-only");
+ return false;
}
} // namespace shill
diff --git a/static_ip_parameters.h b/static_ip_parameters.h
index ecb9dac..e18f869 100644
--- a/static_ip_parameters.h
+++ b/static_ip_parameters.h
@@ -81,13 +81,13 @@
int32 GetMappedSavedInt32Property(const size_t &index, Error *error);
std::string GetMappedStringProperty(const size_t &index, Error *error);
std::string GetMappedSavedStringProperty(const size_t &index, Error *error);
- void SetMappedInt32Property(
+ bool SetMappedInt32Property(
const size_t &index, const int32 &value, Error *error);
- void SetMappedSavedInt32Property(
+ bool SetMappedSavedInt32Property(
const size_t &index, const int32 &value, Error *error);
- void SetMappedStringProperty(
+ bool SetMappedStringProperty(
const size_t &index, const std::string &value, Error *error);
- void SetMappedSavedStringProperty(
+ bool SetMappedSavedStringProperty(
const size_t &index, const std::string &value, Error *error);
KeyValueStore args_;
diff --git a/vpn_driver.cc b/vpn_driver.cc
index 6143980..ed7de93 100644
--- a/vpn_driver.cc
+++ b/vpn_driver.cc
@@ -130,10 +130,15 @@
return string();
}
-void VPNDriver::SetMappedProperty(
+bool VPNDriver::SetMappedProperty(
const size_t &index, const string &value, Error *error) {
CHECK(index < property_count_);
+ if (args_.ContainsString(properties_[index].property) &&
+ args_.GetString(properties_[index].property) == value) {
+ return false;
+ }
args_.SetString(properties_[index].property, value);
+ return true;
}
KeyValueStore VPNDriver::GetProvider(Error *error) {
diff --git a/vpn_driver.h b/vpn_driver.h
index f63dc07..dfccf8c 100644
--- a/vpn_driver.h
+++ b/vpn_driver.h
@@ -94,7 +94,7 @@
void ClearMappedProperty(const size_t &index, Error *error);
std::string GetMappedProperty(const size_t &index, Error *error);
- void SetMappedProperty(
+ bool SetMappedProperty(
const size_t &index, const std::string &value, Error *error);
base::WeakPtrFactory<VPNDriver> weak_ptr_factory_;
diff --git a/vpn_service.cc b/vpn_service.cc
index f893152..0e8771d 100644
--- a/vpn_service.cc
+++ b/vpn_service.cc
@@ -159,9 +159,9 @@
return true;
}
-void VPNService::SetNameProperty(const string &name, Error *error) {
+bool VPNService::SetNameProperty(const string &name, Error *error) {
if (name == friendly_name()) {
- return;
+ return false;
}
LOG(INFO) << "Renaming service " << unique_name() << ": "
<< friendly_name() << " -> " << name;
@@ -170,7 +170,7 @@
args->SetString(flimflam::kNameProperty, name);
string new_storage_id = CreateStorageIdentifier(*args, error);
if (new_storage_id.empty()) {
- return;
+ return false;
}
string old_storage_id = storage_id_;
DCHECK_NE(old_storage_id, new_storage_id);
@@ -182,6 +182,7 @@
storage_id_ = new_storage_id;
profile()->DeleteEntry(old_storage_id, NULL);
profile()->UpdateService(this);
+ return true;
}
} // namespace shill
diff --git a/vpn_service.h b/vpn_service.h
index e665917..9b37cb4 100644
--- a/vpn_service.h
+++ b/vpn_service.h
@@ -34,7 +34,7 @@
virtual bool Unload();
virtual void MakeFavorite();
virtual void SetConnection(const ConnectionRefPtr &connection);
- virtual void SetNameProperty(const std::string &name, Error *error);
+ virtual bool SetNameProperty(const std::string &name, Error *error);
virtual void InitDriverPropertyStore();
diff --git a/vpn_service_unittest.cc b/vpn_service_unittest.cc
index 28086bd..17e91fb 100644
--- a/vpn_service_unittest.cc
+++ b/vpn_service_unittest.cc
@@ -297,11 +297,13 @@
TEST_F(VPNServiceTest, SetNamePropertyTrivial) {
DBus::Error error;
- EXPECT_TRUE(DBusAdaptor::SetProperty(service_->mutable_store(),
- flimflam::kNameProperty,
- DBusAdaptor::StringToVariant(
- service_->friendly_name()),
- &error));
+ // A null change returns false, but with error set to success.
+ EXPECT_FALSE(DBusAdaptor::SetProperty(service_->mutable_store(),
+ flimflam::kNameProperty,
+ DBusAdaptor::StringToVariant(
+ service_->friendly_name()),
+ &error));
+ EXPECT_FALSE(error.is_set());
}
TEST_F(VPNServiceTest, SetNameProperty) {
@@ -335,4 +337,10 @@
TestNamePropertyChange(service_, GetAdaptor());
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(VPNServiceTest, CustomSetterNoopChange) {
+ TestCustomSetterNoopChange(service_, &manager_);
+}
+
} // namespace shill
diff --git a/wifi.cc b/wifi.cc
index 1e8b831..6b440cc 100644
--- a/wifi.cc
+++ b/wifi.cc
@@ -486,7 +486,7 @@
return bgscan_method_.empty() ? kDefaultBgscanMethod : bgscan_method_;
}
-void WiFi::SetBgscanMethod(
+bool WiFi::SetBgscanMethod(
const int &/*argument*/, const string &method, Error *error) {
if (method != WPASupplicant::kNetworkBgscanMethodSimple &&
method != WPASupplicant::kNetworkBgscanMethodLearn &&
@@ -495,30 +495,44 @@
StringPrintf("Unrecognized bgscan method %s", method.c_str());
LOG(WARNING) << error_message;
error->Populate(Error::kInvalidArguments, error_message);
- return;
+ return false;
}
-
+ if (bgscan_method_ == method) {
+ return false;
+ }
bgscan_method_ = method;
// We do not update kNetworkPropertyBgscan for |pending_service_| or
// |current_service_|, because supplicant does not allow for
// reconfiguration without disconnect and reconnect.
+ return true;
}
-void WiFi::SetBgscanShortInterval(const uint16 &seconds, Error */*error*/) {
+bool WiFi::SetBgscanShortInterval(const uint16 &seconds, Error */*error*/) {
+ if (bgscan_short_interval_seconds_ == seconds) {
+ return false;
+ }
bgscan_short_interval_seconds_ = seconds;
// We do not update kNetworkPropertyBgscan for |pending_service_| or
// |current_service_|, because supplicant does not allow for
// reconfiguration without disconnect and reconnect.
+ return true;
}
-void WiFi::SetBgscanSignalThreshold(const int32 &dbm, Error */*error*/) {
+bool WiFi::SetBgscanSignalThreshold(const int32 &dbm, Error */*error*/) {
+ if (bgscan_signal_threshold_dbm_ == dbm) {
+ return false;
+ }
bgscan_signal_threshold_dbm_ = dbm;
// We do not update kNetworkPropertyBgscan for |pending_service_| or
// |current_service_|, because supplicant does not allow for
// reconfiguration without disconnect and reconnect.
+ return true;
}
-void WiFi::SetScanInterval(const uint16 &seconds, Error */*error*/) {
+bool WiFi::SetScanInterval(const uint16 &seconds, Error */*error*/) {
+ if (scan_interval_seconds_ == seconds) {
+ return false;
+ }
scan_interval_seconds_ = seconds;
if (running()) {
StartScanTimer();
@@ -528,6 +542,7 @@
// supplicant). However, we do not update |pending_service_| or
// |current_service_|, because supplicant does not allow for
// reconfiguration without disconnect and reconnect.
+ return true;
}
void WiFi::ClearBgscanMethod(const int &/*argument*/, Error */*error*/) {
@@ -1242,7 +1257,7 @@
PropertyStore *store,
const string &name,
int32(WiFi::*get)(Error *error),
- void(WiFi::*set)(const int32 &value, Error *error)) {
+ bool(WiFi::*set)(const int32 &value, Error *error)) {
store->RegisterDerivedInt32(
name,
Int32Accessor(new CustomAccessor<WiFi, int32>(this, get, set)));
@@ -1252,7 +1267,7 @@
PropertyStore *store,
const string &name,
uint16(WiFi::*get)(Error *error),
- void(WiFi::*set)(const uint16 &value, Error *error)) {
+ bool(WiFi::*set)(const uint16 &value, Error *error)) {
store->RegisterDerivedUint16(
name,
Uint16Accessor(new CustomAccessor<WiFi, uint16>(this, get, set)));
diff --git a/wifi.h b/wifi.h
index 62c137a..8ce8828 100644
--- a/wifi.h
+++ b/wifi.h
@@ -231,11 +231,11 @@
return bgscan_signal_threshold_dbm_;
}
uint16 GetScanInterval(Error */* error */) { return scan_interval_seconds_; }
- void SetBgscanMethod(
+ bool SetBgscanMethod(
const int &argument, const std::string &method, Error *error);
- void SetBgscanShortInterval(const uint16 &seconds, Error *error);
- void SetBgscanSignalThreshold(const int32 &dbm, Error *error);
- void SetScanInterval(const uint16 &seconds, Error *error);
+ bool SetBgscanShortInterval(const uint16 &seconds, Error *error);
+ bool SetBgscanSignalThreshold(const int32 &dbm, Error *error);
+ bool SetScanInterval(const uint16 &seconds, Error *error);
void ClearBgscanMethod(const int &argument, Error *error);
void CurrentBSSChanged(const ::DBus::Path &new_bss);
@@ -267,12 +267,12 @@
PropertyStore *store,
const std::string &name,
int32(WiFi::*get)(Error *error),
- void(WiFi::*set)(const int32 &value, Error *error));
+ bool(WiFi::*set)(const int32 &value, Error *error));
void HelpRegisterDerivedUint16(
PropertyStore *store,
const std::string &name,
uint16(WiFi::*get)(Error *error),
- void(WiFi::*set)(const uint16 &value, Error *error));
+ bool(WiFi::*set)(const uint16 &value, Error *error));
// Disable a network entry in wpa_supplicant, and catch any exception
// that occurs. Returns false if an exception occurred, true otherwise.
diff --git a/wifi_service.cc b/wifi_service.cc
index 7cfee73..4f2ba2e 100644
--- a/wifi_service.cc
+++ b/wifi_service.cc
@@ -215,7 +215,7 @@
return storage_identifier_;
}
-void WiFiService::SetPassphrase(const string &passphrase, Error *error) {
+bool WiFiService::SetPassphrase(const string &passphrase, Error *error) {
if (security_ == flimflam::kSecurityWep) {
ValidateWEPPassphrase(passphrase, error);
} else if (security_ == flimflam::kSecurityPsk ||
@@ -226,13 +226,21 @@
error->Populate(Error::kNotSupported);
}
- if (!error->IsSuccess() || passphrase == passphrase_) {
- return;
+ if (!error->IsSuccess()) {
+ return false;
+ }
+ if (passphrase_ == passphrase) {
+ // After a user logs in, Chrome may reconfigure a Service with the
+ // same credentials as before login. When that occurs, we don't
+ // want to bump the user off the network. Hence, we MUST return
+ // early. (See crbug.com/231456#c17)
+ return false;
}
passphrase_ = passphrase;
ClearCachedCredentials();
UpdateConnectable();
+ return true;
}
// ClearPassphrase is separate from SetPassphrase, because the default
@@ -403,10 +411,19 @@
}
// private methods
+void WiFiService::HelpRegisterConstDerivedString(
+ const string &name,
+ string(WiFiService::*get)(Error *)) {
+ mutable_store()->RegisterDerivedString(
+ name,
+ StringAccessor(
+ new CustomAccessor<WiFiService, string>(this, get, NULL)));
+}
+
void WiFiService::HelpRegisterDerivedString(
const string &name,
- string(WiFiService::*get)(Error *),
- void(WiFiService::*set)(const string&, Error *)) {
+ string(WiFiService::*get)(Error *error),
+ bool(WiFiService::*set)(const string &, Error *)) {
mutable_store()->RegisterDerivedString(
name,
StringAccessor(new CustomAccessor<WiFiService, string>(this, get, set)));
@@ -414,7 +431,7 @@
void WiFiService::HelpRegisterWriteOnlyDerivedString(
const string &name,
- void(WiFiService::*set)(const string &, Error *),
+ bool(WiFiService::*set)(const string &, Error *),
void(WiFiService::*clear)(Error *),
const string *default_value) {
mutable_store()->RegisterDerivedString(
diff --git a/wifi_service.h b/wifi_service.h
index cbb10f5..80844f0 100644
--- a/wifi_service.h
+++ b/wifi_service.h
@@ -168,13 +168,16 @@
// Override the base clase implementation, because we need to allow
// arguments that aren't base class methods.
+ void HelpRegisterConstDerivedString(
+ const std::string &name,
+ std::string(WiFiService::*get)(Error *error));
void HelpRegisterDerivedString(
const std::string &name,
std::string(WiFiService::*get)(Error *error),
- void(WiFiService::*set)(const std::string &value, Error *error));
+ bool(WiFiService::*set)(const std::string &value, Error *error));
void HelpRegisterWriteOnlyDerivedString(
const std::string &name,
- void(WiFiService::*set)(const std::string &value, Error *error),
+ bool(WiFiService::*set)(const std::string &value, Error *error),
void(WiFiService::*clear)(Error *error),
const std::string *default_value);
@@ -218,7 +221,7 @@
KeyValueStore GetStorageProperties() const;
// Validate then apply a passphrase for this service.
- void SetPassphrase(const std::string &passphrase, Error *error);
+ bool SetPassphrase(const std::string &passphrase, Error *error);
// Select a WiFi device (e.g, for connecting a hidden service with no
// endpoints).
diff --git a/wifi_service_unittest.cc b/wifi_service_unittest.cc
index f606eaf..0d62e8e 100644
--- a/wifi_service_unittest.cc
+++ b/wifi_service_unittest.cc
@@ -164,6 +164,7 @@
return error.type();
}
scoped_refptr<MockWiFi> wifi() { return wifi_; }
+ MockManager *mock_manager() { return &mock_manager_; }
MockWiFiProvider *provider() { return &provider_; }
string GetAnyDeviceAddress() { return WiFiService::kAnyDeviceAddress; }
const vector<uint8_t> &simple_ssid() { return simple_ssid_; }
@@ -675,7 +676,6 @@
}
TEST_F(WiFiServiceTest, SetPassphraseRemovesCachedCredentials) {
- vector<uint8_t> ssid(5);
WiFiServiceRefPtr wifi_service = MakeServiceWithWiFi(flimflam::kSecurityRsn);
const string kPassphrase = "abcdefgh";
@@ -734,6 +734,85 @@
}
}
+// This test is somewhat redundant, since:
+//
+// a) we test that generic property setters return false on a null
+// change (e.g. in PropertyAccessorTest.SignedIntCorrectness)
+// b) we test that custom EAP property setters return false on a null
+// change in EapCredentialsTest.CustomSetterNoopChange
+// c) we test that the various custom accessors pass through the
+// return value of custom setters
+// (e.g. PropertyAccessorTest.CustomAccessorCorrectness)
+// d) we test that PropertyStore skips the change callback when a
+// property setter return false (PropertyStoreTypedTest.SetProperty)
+//
+// Nonetheless, I think it's worth testing the WiFi+EAP case directly.
+TEST_F(WiFiServiceTest, EapAuthPropertyChangeClearsCachedCredentials) {
+ WiFiServiceRefPtr wifi_service =
+ MakeServiceWithWiFi(flimflam::kSecurity8021x);
+ PropertyStore &property_store(*wifi_service->mutable_store());
+
+ // Property with custom accessor.
+ const string kPassword = "abcdefgh";
+ {
+ Error error;
+ // A changed passphrase should trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(wifi_service.get()));
+ EXPECT_TRUE(property_store.SetStringProperty(
+ flimflam::kEapPasswordProperty, kPassword, &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+ {
+ Error error;
+ // An unchanged passphrase should not trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(_)).Times(0);
+ EXPECT_FALSE(property_store.SetStringProperty(
+ flimflam::kEapPasswordProperty, kPassword, &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+ {
+ Error error;
+ // A modified passphrase should trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(wifi_service.get()));
+ EXPECT_TRUE(property_store.SetStringProperty(
+ flimflam::kEapPasswordProperty, kPassword + "X", &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // Property with generic accessor.
+ const string kCertId = "abcdefgh";
+ {
+ Error error;
+ // A changed cert id should trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(wifi_service.get()));
+ EXPECT_TRUE(property_store.SetStringProperty(
+ flimflam::kEapCertIdProperty, kCertId, &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+ {
+ Error error;
+ // An unchanged cert id should not trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(_)).Times(0);
+ EXPECT_FALSE(property_store.SetStringProperty(
+ flimflam::kEapCertIdProperty, kCertId, &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+ {
+ Error error;
+ // A modified cert id should trigger cache removal.
+ EXPECT_CALL(*wifi(), ClearCachedCredentials(wifi_service.get()));
+ EXPECT_TRUE(property_store.SetStringProperty(
+ flimflam::kEapCertIdProperty, kCertId + "X", &error));
+ Mock::VerifyAndClearExpectations(wifi());
+ EXPECT_TRUE(error.IsSuccess());
+ }
+}
+
TEST_F(WiFiServiceTest, LoadHidden) {
WiFiServiceRefPtr service = MakeSimpleService(flimflam::kSecurityNone);
ASSERT_FALSE(service->hidden_ssid_);
@@ -1676,4 +1755,11 @@
Mock::VerifyAndClearExpectations(adaptor);
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(WiFiServiceTest, CustomSetterNoopChange) {
+ WiFiServiceRefPtr service = MakeServiceWithMockManager();
+ TestCustomSetterNoopChange(service, mock_manager());
+}
+
} // namespace shill
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index 5bbc9f9..5077d46 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -131,6 +131,7 @@
flimflam::kScanningProperty,
PropertyStoreTest::kBoolV,
&error));
+ ASSERT_TRUE(error.is_set()); // name() may be invalid otherwise
EXPECT_EQ(invalid_args(), error.name());
}
@@ -589,8 +590,8 @@
void SetPendingService(const WiFiServiceRefPtr &service) {
wifi_->pending_service_ = service;
}
- void SetScanInterval(uint16_t interval_seconds) {
- wifi_->SetScanInterval(interval_seconds, NULL);
+ bool SetScanInterval(uint16_t interval_seconds, Error *error) {
+ return wifi_->SetScanInterval(interval_seconds, error);
}
uint16_t GetScanInterval() {
return wifi_->GetScanInterval(NULL);
@@ -666,6 +667,14 @@
wifi_->OnLinkMonitorFailure();
}
+ bool SetBgscanShortInterval(const uint16 &interval, Error *error) {
+ return wifi_->SetBgscanShortInterval(interval, error);
+ }
+
+ bool SetBgscanSignalThreshold(const int32 &threshold, Error *error) {
+ return wifi_->SetBgscanSignalThreshold(threshold, error);
+ }
+
NiceMockControl *control_interface() {
return &control_interface_;
}
@@ -1783,7 +1792,7 @@
CancelScanTimer();
EXPECT_TRUE(GetScanTimer().IsCancelled());
- SetScanInterval(1);
+ SetScanInterval(1, NULL);
EXPECT_FALSE(GetScanTimer().IsCancelled());
}
@@ -1800,7 +1809,7 @@
StartWiFi();
EXPECT_FALSE(GetScanTimer().IsCancelled());
- SetScanInterval(0);
+ SetScanInterval(0, NULL);
EXPECT_TRUE(GetScanTimer().IsCancelled());
}
@@ -2215,4 +2224,39 @@
EXPECT_EQ("[SSID=foo\\x5b\\x09\\x5dbar]", WiFi::LogSSID("foo[\t]bar"));
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(WiFiMainTest, CustomSetterNoopChange) {
+ // SetBgscanShortInterval
+ {
+ Error error;
+ static const uint16 kKnownScanInterval = 4;
+ // Set to known value.
+ EXPECT_TRUE(SetBgscanShortInterval(kKnownScanInterval, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(SetBgscanShortInterval(kKnownScanInterval, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetBgscanSignalThreshold
+ {
+ Error error;
+ static const int32 kKnownSignalThreshold = 4;
+ // Set to known value.
+ EXPECT_TRUE(SetBgscanSignalThreshold(kKnownSignalThreshold, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ // Set to same value.
+ EXPECT_FALSE(SetBgscanSignalThreshold(kKnownSignalThreshold, &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+
+ // SetScanInterval
+ {
+ Error error;
+ EXPECT_FALSE(SetScanInterval(GetScanInterval(), &error));
+ EXPECT_TRUE(error.IsSuccess());
+ }
+}
+
} // namespace shill
diff --git a/wimax_service_unittest.cc b/wimax_service_unittest.cc
index 1aa906f..920e2c6 100644
--- a/wimax_service_unittest.cc
+++ b/wimax_service_unittest.cc
@@ -327,4 +327,10 @@
Mock::VerifyAndClearExpectations(adaptor);
}
+// Custom property setters should return false, and make no changes, if
+// the new value is the same as the old value.
+TEST_F(WiMaxServiceTest, CustomSetterNoopChange) {
+ TestCustomSetterNoopChange(service_, &manager_);
+}
+
} // namespace shill