shill: IPConfig: Don't reset StaticIP on failure
If the IP address associated with an IPConfig is provided by
StaticIP, the entire configuration should be retained even if
the automatic configuration method fails. Otherwise,
configuration parameters provided outside of the automatic
configuration mechanism will be lost.
To accomplish this cleanly, we need to change the contract
between IPConfig and Device objects. The failure and success
events are now split into separate callbacks. Intead of
passing a bool parameter to OnIPConfigUpdated, failure cases
are handled by a separate method. Further, there is no
expectation in the failure case that the IPConfig resets
itself, so the IPConfig provides a public method for the
Device to call if it chooses.
BUG=chromium:318290
TEST=Unittest, new autotest network_DhcpFailureWithStaticIP (CL:179783)
All other network_Dhcp* unit tests run.
Change-Id: I55ed5a7adfc5d97e45ce832e25caab97ff39cea6
Reviewed-on: https://chromium-review.googlesource.com/179786
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
diff --git a/connection_unittest.cc b/connection_unittest.cc
index d4189c4..fb3f18e 100644
--- a/connection_unittest.cc
+++ b/connection_unittest.cc
@@ -100,7 +100,7 @@
}
void UpdateProperties() {
- ipconfig_->UpdateProperties(properties_, true);
+ ipconfig_->UpdateProperties(properties_);
}
bool PinHostRoute(ConnectionRefPtr connection,
diff --git a/device.cc b/device.cc
index 30f2e66..b711b29 100644
--- a/device.cc
+++ b/device.cc
@@ -417,6 +417,8 @@
ShouldUseMinimalDHCPConfig());
ipconfig_->RegisterUpdateCallback(Bind(&Device::OnIPConfigUpdated,
weak_ptr_factory_.GetWeakPtr()));
+ ipconfig_->RegisterFailureCallback(Bind(&Device::OnIPConfigFailed,
+ weak_ptr_factory_.GetWeakPtr()));
ipconfig_->RegisterRefreshCallback(Bind(&Device::OnIPConfigRefreshed,
weak_ptr_factory_.GetWeakPtr()));
dispatcher_->PostTask(Bind(&Device::ConfigureStaticIPTask,
@@ -469,7 +471,7 @@
// If the parameters contain an IP address, apply them now and bring
// the interface up. When DHCP information arrives, it will supplement
// the static information.
- OnIPConfigUpdated(ipconfig_, true);
+ OnIPConfigUpdated(ipconfig_);
} else {
// Either |ipconfig_| has just been created in AcquireIPConfig() or
// we're being called by OnIPConfigRefreshed(). In either case a
@@ -479,81 +481,83 @@
}
}
-void Device::OnIPConfigUpdated(const IPConfigRefPtr &ipconfig, bool success) {
- SLOG(Device, 2) << __func__ << " " << " success: " << success;
- if (success) {
- CreateConnection();
- if (selected_service_) {
- ipconfig->ApplyStaticIPParameters(
- selected_service_->mutable_static_ip_parameters());
- if (selected_service_->static_ip_parameters().ContainsAddress()) {
- // If we are using a statically configured IP address instead
- // of a leased IP address, release any acquired lease so it may
- // be used by others. This allows us to merge other non-leased
- // parameters (like DNS) when they're available from a DHCP server
- // and not overridden by static parameters, but at the same time
- // we avoid taking up a dynamic IP address the DHCP server could
- // assign to someone else who might actually use it.
- ipconfig->ReleaseIP(IPConfig::kReleaseReasonStaticIP);
- }
+void Device::OnIPConfigUpdated(const IPConfigRefPtr &ipconfig) {
+ SLOG(Device, 2) << __func__;
+ CreateConnection();
+ if (selected_service_) {
+ ipconfig->ApplyStaticIPParameters(
+ selected_service_->mutable_static_ip_parameters());
+ if (selected_service_->static_ip_parameters().ContainsAddress()) {
+ // If we are using a statically configured IP address instead
+ // of a leased IP address, release any acquired lease so it may
+ // be used by others. This allows us to merge other non-leased
+ // parameters (like DNS) when they're available from a DHCP server
+ // and not overridden by static parameters, but at the same time
+ // we avoid taking up a dynamic IP address the DHCP server could
+ // assign to someone else who might actually use it.
+ ipconfig->ReleaseIP(IPConfig::kReleaseReasonStaticIP);
}
- connection_->UpdateFromIPConfig(ipconfig);
- // SetConnection must occur after the UpdateFromIPConfig so the
- // service can use the values derived from the connection.
- if (selected_service_) {
- selected_service_->OnDHCPSuccess();
- selected_service_->SetConnection(connection_);
- }
- // The service state change needs to happen last, so that at the
- // time we report the state change to the manager, the service
- // has its connection.
- SetServiceState(Service::kStateConnected);
- OnConnected();
- portal_attempts_to_online_ = 0;
- // Subtle: Start portal detection after transitioning the service
- // to the Connected state because this call may immediately transition
- // to the Online state.
- if (selected_service_) {
- StartPortalDetection();
- }
- StartLinkMonitor();
- } else {
- // TODO(pstew): This logic gets yet more complex when multiple
- // IPConfig types are run in parallel (e.g. DHCP and DHCP6)
- if (selected_service_) {
- selected_service_->OnDHCPFailure();
-
- if (selected_service_->static_ip_parameters().ContainsAddress()) {
- // Consider three cases:
- //
- // 1. We're here because DHCP failed while starting up. There
- // are two subcases:
- // a. DHCP has failed, and Static IP config has _not yet_
- // completed. It's fine to do nothing, because we'll
- // apply the static config shortly.
- // b. DHCP has failed, and Static IP config has _already_
- // completed. It's fine to do nothing, because we can
- // continue to use the static config that's already
- // been applied.
- //
- // 2. We're here because a previously valid DHCP configuration
- // is no longer valid. There's still a static IP config,
- // because the condition in the if clause evaluated to true.
- // Furthermore, the static config includes an IP address for
- // us to use.
- //
- // The current configuration may include some DHCP
- // parameters, overriden by any static parameters
- // provided. We continue to use this configuration, because
- // the only configuration element that is leased to us (IP
- // address) will be overriden by a static parameter.
- return;
- }
- }
-
- OnIPConfigFailure();
- DestroyConnection();
}
+ connection_->UpdateFromIPConfig(ipconfig);
+ // SetConnection must occur after the UpdateFromIPConfig so the
+ // service can use the values derived from the connection.
+ if (selected_service_) {
+ selected_service_->OnDHCPSuccess();
+ selected_service_->SetConnection(connection_);
+ }
+ // The service state change needs to happen last, so that at the
+ // time we report the state change to the manager, the service
+ // has its connection.
+ SetServiceState(Service::kStateConnected);
+ OnConnected();
+ portal_attempts_to_online_ = 0;
+ // Subtle: Start portal detection after transitioning the service
+ // to the Connected state because this call may immediately transition
+ // to the Online state.
+ if (selected_service_) {
+ StartPortalDetection();
+ }
+ StartLinkMonitor();
+}
+
+void Device::OnIPConfigFailed(const IPConfigRefPtr &ipconfig) {
+ SLOG(Device, 2) << __func__;
+ // TODO(pstew): This logic gets yet more complex when multiple
+ // IPConfig types are run in parallel (e.g. DHCP and DHCP6)
+ if (selected_service_) {
+ selected_service_->OnDHCPFailure();
+
+ if (selected_service_->static_ip_parameters().ContainsAddress()) {
+ // Consider three cases:
+ //
+ // 1. We're here because DHCP failed while starting up. There
+ // are two subcases:
+ // a. DHCP has failed, and Static IP config has _not yet_
+ // completed. It's fine to do nothing, because we'll
+ // apply the static config shortly.
+ // b. DHCP has failed, and Static IP config has _already_
+ // completed. It's fine to do nothing, because we can
+ // continue to use the static config that's already
+ // been applied.
+ //
+ // 2. We're here because a previously valid DHCP configuration
+ // is no longer valid. There's still a static IP config,
+ // because the condition in the if clause evaluated to true.
+ // Furthermore, the static config includes an IP address for
+ // us to use.
+ //
+ // The current configuration may include some DHCP
+ // parameters, overriden by any static parameters
+ // provided. We continue to use this configuration, because
+ // the only configuration element that is leased to us (IP
+ // address) will be overriden by a static parameter.
+ return;
+ }
+ }
+
+ ipconfig->ResetProperties();
+ OnIPConfigFailure();
+ DestroyConnection();
}
void Device::OnIPConfigRefreshed(const IPConfigRefPtr &ipconfig) {
diff --git a/device.h b/device.h
index 0d47aaf..51d366b 100644
--- a/device.h
+++ b/device.h
@@ -360,8 +360,11 @@
// request was successfully sent.
bool AcquireIPConfigWithLeaseName(const std::string &lease_name);
- // Callback invoked on every IP configuration update.
- void OnIPConfigUpdated(const IPConfigRefPtr &ipconfig, bool success);
+ // Callback invoked on successful IP configuration updates.
+ void OnIPConfigUpdated(const IPConfigRefPtr &ipconfig);
+
+ // Callback invoked on IP configuration failures.
+ void OnIPConfigFailed(const IPConfigRefPtr &ipconfig);
// Callback invoked when "Refresh" is invoked on an IPConfig. This usually
// signals a change in static IP parameters.
diff --git a/device_unittest.cc b/device_unittest.cc
index 6f95634..fe2df87 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -147,8 +147,12 @@
static const char kDeviceAddress[];
static const int kDeviceInterfaceIndex;
- void OnIPConfigUpdated(const IPConfigRefPtr &ipconfig, bool success) {
- device_->OnIPConfigUpdated(ipconfig, success);
+ void OnIPConfigUpdated(const IPConfigRefPtr &ipconfig) {
+ device_->OnIPConfigUpdated(ipconfig);
+ }
+
+ void OnIPConfigFailed(const IPConfigRefPtr &ipconfig) {
+ device_->OnIPConfigFailed(ipconfig);
}
void SelectService(const ServiceRefPtr service) {
@@ -360,6 +364,8 @@
}
TEST_F(DeviceTest, IPConfigUpdatedFailure) {
+ scoped_refptr<MockIPConfig> ipconfig = new MockIPConfig(control_interface(),
+ kDeviceName);
scoped_refptr<MockService> service(
new StrictMock<MockService>(control_interface(),
dispatcher(),
@@ -369,10 +375,13 @@
EXPECT_CALL(*service, OnDHCPFailure());
EXPECT_CALL(*service, DisconnectWithFailure(Service::kFailureDHCP, _));
EXPECT_CALL(*service, SetConnection(IsNullRefPtr()));
- OnIPConfigUpdated(NULL, false);
+ EXPECT_CALL(*ipconfig, ResetProperties());
+ OnIPConfigFailed(ipconfig.get());
}
TEST_F(DeviceTest, IPConfigUpdatedFailureWithStatic) {
+ scoped_refptr<MockIPConfig> ipconfig = new MockIPConfig(control_interface(),
+ kDeviceName);
scoped_refptr<MockService> service(
new StrictMock<MockService>(control_interface(),
dispatcher(),
@@ -386,7 +395,9 @@
EXPECT_CALL(*service, OnDHCPFailure());
EXPECT_CALL(*service, DisconnectWithFailure(_, _)).Times(0);
EXPECT_CALL(*service, SetConnection(_)).Times(0);
- OnIPConfigUpdated(NULL, false);
+ // The IPConfig should retain the previous values.
+ EXPECT_CALL(*ipconfig, ResetProperties()).Times(0);
+ OnIPConfigFailed(ipconfig.get());
}
TEST_F(DeviceTest, IPConfigUpdatedSuccess) {
@@ -406,7 +417,7 @@
EXPECT_CALL(*service, SetState(Service::kStateOnline));
EXPECT_CALL(*service, OnDHCPSuccess());
EXPECT_CALL(*service, SetConnection(NotNullRefPtr()));
- OnIPConfigUpdated(ipconfig.get(), true);
+ OnIPConfigUpdated(ipconfig.get());
}
TEST_F(DeviceTest, IPConfigUpdatedSuccessNoSelectedService) {
@@ -415,7 +426,7 @@
scoped_refptr<MockIPConfig> ipconfig = new MockIPConfig(control_interface(),
kDeviceName);
SelectService(NULL);
- OnIPConfigUpdated(ipconfig.get(), true);
+ OnIPConfigUpdated(ipconfig.get());
}
TEST_F(DeviceTest, SetEnabledNonPersistent) {
@@ -706,11 +717,11 @@
IPConfig::Properties properties;
properties.vendor_encapsulated_options =
Tethering::kAndroidVendorEncapsulatedOptions;
- device_->ipconfig_->UpdateProperties(properties, true);
+ device_->ipconfig_->UpdateProperties(properties);
EXPECT_TRUE(device_->IsConnectedViaTether());
properties.vendor_encapsulated_options = "Some other non-empty value";
- device_->ipconfig_->UpdateProperties(properties, true);
+ device_->ipconfig_->UpdateProperties(properties);
EXPECT_FALSE(device_->IsConnectedViaTether());
}
diff --git a/dhcp_config.cc b/dhcp_config.cc
index be81f8e..1ac3522 100644
--- a/dhcp_config.cc
+++ b/dhcp_config.cc
@@ -163,7 +163,7 @@
LOG(INFO) << "Event reason: " << reason;
if (reason == kReasonFail) {
LOG(ERROR) << "Received failure event from DHCP client.";
- UpdateProperties(IPConfig::Properties(), false);
+ NotifyFailure();
return;
} else if (reason == kReasonNak) {
// If we got a NAK, this means the DHCP server is active, and any
@@ -194,17 +194,22 @@
// client is still running, so we should not cancel the timeout
// until that completes. In the meantime, however, we can tentatively
// configure our network in anticipation of successful completion.
- IPConfig::UpdateProperties(properties, true);
+ IPConfig::UpdateProperties(properties);
is_gateway_arp_active_ = true;
} else {
- UpdateProperties(properties, true);
+ UpdateProperties(properties);
is_gateway_arp_active_ = false;
}
}
-void DHCPConfig::UpdateProperties(const Properties &properties, bool success) {
+void DHCPConfig::UpdateProperties(const Properties &properties) {
StopDHCPTimeout();
- IPConfig::UpdateProperties(properties, success);
+ IPConfig::UpdateProperties(properties);
+}
+
+void DHCPConfig::NotifyFailure() {
+ StopDHCPTimeout();
+ IPConfig::NotifyFailure();
}
bool DHCPConfig::Start() {
@@ -507,7 +512,7 @@
if (is_gateway_arp_active_) {
LOG(INFO) << "Continuing to use our previous lease, due to gateway-ARP.";
} else {
- UpdateProperties(IPConfig::Properties(), false);
+ NotifyFailure();
}
}
diff --git a/dhcp_config.h b/dhcp_config.h
index d471800..501dc92 100644
--- a/dhcp_config.h
+++ b/dhcp_config.h
@@ -67,30 +67,31 @@
protected:
// Overrides base clase implementation.
- virtual void UpdateProperties(const Properties &properties, bool success);
+ virtual void UpdateProperties(const Properties &properties);
+ virtual void NotifyFailure();
private:
friend class DHCPConfigTest;
+ FRIEND_TEST(DHCPConfigCallbackTest, ProcessEventSignalFail);
+ FRIEND_TEST(DHCPConfigCallbackTest, ProcessEventSignalGatewayArp);
+ FRIEND_TEST(DHCPConfigCallbackTest, ProcessEventSignalGatewayArpNak);
+ FRIEND_TEST(DHCPConfigCallbackTest, ProcessEventSignalSuccess);
+ FRIEND_TEST(DHCPConfigCallbackTest, ProcessEventSignalUnknown);
+ FRIEND_TEST(DHCPConfigCallbackTest, RequestIPTimeout);
+ FRIEND_TEST(DHCPConfigCallbackTest, StartTimeout);
FRIEND_TEST(DHCPConfigTest, GetIPv4AddressString);
FRIEND_TEST(DHCPConfigTest, InitProxy);
FRIEND_TEST(DHCPConfigTest, ParseClasslessStaticRoutes);
FRIEND_TEST(DHCPConfigTest, ParseConfiguration);
- FRIEND_TEST(DHCPConfigTest, ProcessEventSignalFail);
- FRIEND_TEST(DHCPConfigTest, ProcessEventSignalGatewayArp);
- FRIEND_TEST(DHCPConfigTest, ProcessEventSignalGatewayArpNak);
- FRIEND_TEST(DHCPConfigTest, ProcessEventSignalSuccess);
- FRIEND_TEST(DHCPConfigTest, ProcessEventSignalUnknown);
FRIEND_TEST(DHCPConfigTest, ReleaseIP);
FRIEND_TEST(DHCPConfigTest, ReleaseIPArpGW);
FRIEND_TEST(DHCPConfigTest, ReleaseIPStaticIPWithLease);
FRIEND_TEST(DHCPConfigTest, ReleaseIPStaticIPWithoutLease);
FRIEND_TEST(DHCPConfigTest, RenewIP);
FRIEND_TEST(DHCPConfigTest, RequestIP);
- FRIEND_TEST(DHCPConfigTest, RequestIPTimeout);
FRIEND_TEST(DHCPConfigTest, Restart);
FRIEND_TEST(DHCPConfigTest, RestartNoClient);
FRIEND_TEST(DHCPConfigTest, StartFail);
- FRIEND_TEST(DHCPConfigTest, StartTimeout);
FRIEND_TEST(DHCPConfigTest, StartWithHostname);
FRIEND_TEST(DHCPConfigTest, StartWithMinimalConfig);
FRIEND_TEST(DHCPConfigTest, StartWithoutArpGateway);
@@ -174,6 +175,7 @@
// Informs upper layers of the failure.
void ProcessDHCPTimeout();
+ // Kills DHCP client process.
void KillClient();
// Store cached copies of singletons for speed/ease of testing.
diff --git a/dhcp_config_unittest.cc b/dhcp_config_unittest.cc
index 88adfd2..a48e371 100644
--- a/dhcp_config_unittest.cc
+++ b/dhcp_config_unittest.cc
@@ -416,50 +416,47 @@
namespace {
-class UpdateCallbackTest {
+class DHCPConfigCallbackTest : public DHCPConfigTest {
public:
- UpdateCallbackTest(const string &message,
- const IPConfigRefPtr &ipconfig,
- bool success)
- : message_(message),
- ipconfig_(ipconfig),
- success_(success),
- called_(false) {}
-
- void Callback(const IPConfigRefPtr &ipconfig, bool success) {
- called_ = true;
- EXPECT_EQ(ipconfig_.get(), ipconfig.get()) << message_;
- EXPECT_EQ(success_, success) << message_;
+ virtual void SetUp() {
+ DHCPConfigTest::SetUp();
+ config_->RegisterUpdateCallback(
+ Bind(&DHCPConfigCallbackTest::SuccessCallback, Unretained(this)));
+ config_->RegisterFailureCallback(
+ Bind(&DHCPConfigCallbackTest::FailureCallback, Unretained(this)));
+ ip_config_ = config_;
}
- bool called() const { return called_; }
+ MOCK_METHOD1(SuccessCallback, void(const IPConfigRefPtr &ipconfig));
+ MOCK_METHOD1(FailureCallback, void(const IPConfigRefPtr &ipconfig));
+
+ // The mock methods above take IPConfigRefPtr because this is the type
+ // that the registered callbacks take. This conversion of the DHCP
+ // config ref pointer eases our work in setting up expectations.
+ const IPConfigRefPtr &ConfigRef() { return ip_config_; }
private:
- const string message_;
- IPConfigRefPtr ipconfig_;
- bool success_;
- bool called_;
+ IPConfigRefPtr ip_config_;
};
void DoNothing() {}
} // namespace {}
-TEST_F(DHCPConfigTest, ProcessEventSignalFail) {
+TEST_F(DHCPConfigCallbackTest, ProcessEventSignalFail) {
DHCPConfig::Configuration conf;
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
0x01020304);
- UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(ConfigRef()));
config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(DHCPConfig::kReasonFail, conf);
- EXPECT_TRUE(callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
EXPECT_TRUE(config_->properties().address.empty());
EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
-TEST_F(DHCPConfigTest, ProcessEventSignalSuccess) {
+TEST_F(DHCPConfigCallbackTest, ProcessEventSignalSuccess) {
static const char * const kReasons[] = {
DHCPConfig::kReasonBound,
DHCPConfig::kReasonRebind,
@@ -470,45 +467,41 @@
DHCPConfig::Configuration conf;
string message = string(kReasons[r]) + " failed";
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(r);
- UpdateCallbackTest callback_test(message, config_, true);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(ConfigRef()));
+ EXPECT_CALL(*this, FailureCallback(_)).Times(0);
config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(kReasons[r], conf);
- EXPECT_TRUE(callback_test.called()) << message;
+ EXPECT_TRUE(Mock::VerifyAndClearExpectations(this)) << message;
EXPECT_EQ(base::StringPrintf("%zu.0.0.0", r), config_->properties().address)
<< message;
EXPECT_TRUE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
}
-TEST_F(DHCPConfigTest, ProcessEventSignalUnknown) {
+TEST_F(DHCPConfigCallbackTest, ProcessEventSignalUnknown) {
DHCPConfig::Configuration conf;
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
0x01020304);
static const char kReasonUnknown[] = "UNKNOWN_REASON";
- UpdateCallbackTest callback_test(kReasonUnknown, config_, false);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(_)).Times(0);
config_->lease_acquisition_timeout_callback_.Reset(base::Bind(&DoNothing));
config_->ProcessEventSignal(kReasonUnknown, conf);
- EXPECT_FALSE(callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
EXPECT_TRUE(config_->properties().address.empty());
EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
}
-TEST_F(DHCPConfigTest, ProcessEventSignalGatewayArp) {
+TEST_F(DHCPConfigCallbackTest, ProcessEventSignalGatewayArp) {
DHCPConfig::Configuration conf;
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
0x01020304);
- UpdateCallbackTest success_callback_test(DHCPConfig::kReasonGatewayArp,
- config_, true);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&success_callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(ConfigRef()));
+ EXPECT_CALL(*this, FailureCallback(_)).Times(0);
EXPECT_CALL(*minijail_, RunAndDestroy(_, _, _)).WillOnce(Return(true));
config_->Start();
config_->ProcessEventSignal(DHCPConfig::kReasonGatewayArp, conf);
- EXPECT_TRUE(success_callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
EXPECT_EQ("4.3.2.1", config_->properties().address);
EXPECT_FALSE(config_->lease_acquisition_timeout_callback_.IsCancelled());
EXPECT_TRUE(config_->is_gateway_arp_active_);
@@ -519,31 +512,24 @@
EXPECT_CALL(log, Log(_, _, _)).Times(AnyNumber());
EXPECT_CALL(log, Log(_, _, ::testing::EndsWith(
"Continuing to use our previous lease, due to gateway-ARP.")));
- UpdateCallbackTest failure_callback_test("Timeout", config_, true);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&failure_callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(_)).Times(0);
config_->lease_acquisition_timeout_callback_.callback().Run();
- EXPECT_FALSE(failure_callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
EXPECT_TRUE(config_->is_gateway_arp_active_);
// An official reply from a DHCP server should reset our GatewayArp state.
- UpdateCallbackTest renew_callback_test(DHCPConfig::kReasonRenew,
- config_, true);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&renew_callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(ConfigRef()));
+ EXPECT_CALL(*this, FailureCallback(_)).Times(0);
config_->ProcessEventSignal(DHCPConfig::kReasonRenew, conf);
- EXPECT_TRUE(renew_callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
EXPECT_FALSE(config_->is_gateway_arp_active_);
}
-TEST_F(DHCPConfigTest, ProcessEventSignalGatewayArpNak) {
+TEST_F(DHCPConfigCallbackTest, ProcessEventSignalGatewayArpNak) {
DHCPConfig::Configuration conf;
conf[DHCPConfig::kConfigurationKeyIPAddress].writer().append_uint32(
0x01020304);
- UpdateCallbackTest success_callback_test(DHCPConfig::kReasonGatewayArp,
- config_, true);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&success_callback_test)));
EXPECT_CALL(*minijail_, RunAndDestroy(_, _, _)).WillOnce(Return(true));
config_->Start();
config_->ProcessEventSignal(DHCPConfig::kReasonGatewayArp, conf);
@@ -552,14 +538,14 @@
// Sending a NAK should clear is_gateway_arp_active_.
config_->ProcessEventSignal(DHCPConfig::kReasonNak, conf);
EXPECT_FALSE(config_->is_gateway_arp_active_);
+ Mock::VerifyAndClearExpectations(this);
// If the timeout gets called, we should lose the lease since GatewayArp
// is not active any more.
- UpdateCallbackTest failure_callback_test("Timeout", config_, false);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&failure_callback_test)));
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(ConfigRef()));
config_->lease_acquisition_timeout_callback_.callback().Run();
- EXPECT_TRUE(failure_callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
}
TEST_F(DHCPConfigTest, ReleaseIP) {
@@ -630,17 +616,16 @@
config_->pid_ = 0;
}
-TEST_F(DHCPConfigTest, RequestIPTimeout) {
- UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+TEST_F(DHCPConfigCallbackTest, RequestIPTimeout) {
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(ConfigRef()));
config_->lease_acquisition_timeout_seconds_ = 0;
config_->pid_ = 567;
EXPECT_CALL(*proxy_, Rebind(kDeviceName)).Times(1);
config_->proxy_.reset(proxy_.release());
config_->RenewIP();
config_->dispatcher_->DispatchPendingEvents();
- EXPECT_TRUE(callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
config_->pid_ = 0;
}
@@ -694,16 +679,15 @@
StopRunningConfigAndExpect(config, true);
}
-TEST_F(DHCPConfigTest, StartTimeout) {
- UpdateCallbackTest callback_test(DHCPConfig::kReasonFail, config_, false);
- config_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
+TEST_F(DHCPConfigCallbackTest, StartTimeout) {
+ EXPECT_CALL(*this, SuccessCallback(_)).Times(0);
+ EXPECT_CALL(*this, FailureCallback(ConfigRef()));
config_->lease_acquisition_timeout_seconds_ = 0;
config_->proxy_.reset(proxy_.release());
EXPECT_CALL(*minijail_, RunAndDestroy(_, _, _)).WillOnce(Return(true));
config_->Start();
config_->dispatcher_->DispatchPendingEvents();
- EXPECT_TRUE(callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
}
TEST_F(DHCPConfigTest, Stop) {
diff --git a/ipconfig.cc b/ipconfig.cc
index 90962db..cc8172e 100644
--- a/ipconfig.cc
+++ b/ipconfig.cc
@@ -129,27 +129,48 @@
return true;
}
-void IPConfig::UpdateProperties(const Properties &properties, bool success) {
+void IPConfig::UpdateProperties(const Properties &properties) {
// Take a reference of this instance to make sure we don't get destroyed in
// the middle of this call. (The |update_callback_| may cause a reference
// to be dropped. See, e.g., EthernetService::Disconnect and
// Ethernet::DropConnection.)
IPConfigRefPtr me = this;
+
properties_ = properties;
+
if (!update_callback_.is_null()) {
- update_callback_.Run(this, success);
+ update_callback_.Run(this);
}
EmitChanges();
}
-void IPConfig::RegisterRefreshCallback(
- const Callback<void(const IPConfigRefPtr&)> &callback) {
+void IPConfig::NotifyFailure() {
+ // Take a reference of this instance to make sure we don't get destroyed in
+ // the middle of this call. (The |update_callback_| may cause a reference
+ // to be dropped. See, e.g., EthernetService::Disconnect and
+ // Ethernet::DropConnection.)
+ IPConfigRefPtr me = this;
+
+ if (!failure_callback_.is_null()) {
+ failure_callback_.Run(this);
+ }
+}
+
+void IPConfig::RegisterUpdateCallback(const Callback &callback) {
+ update_callback_ = callback;
+}
+
+void IPConfig::RegisterFailureCallback(const Callback &callback) {
+ failure_callback_ = callback;
+}
+
+void IPConfig::RegisterRefreshCallback(const Callback &callback) {
refresh_callback_ = callback;
}
-void IPConfig::RegisterUpdateCallback(
- const Callback<void(const IPConfigRefPtr&, bool)> &callback) {
- update_callback_ = callback;
+void IPConfig::ResetProperties() {
+ properties_ = Properties();
+ EmitChanges();
}
void IPConfig::EmitChanges() {
diff --git a/ipconfig.h b/ipconfig.h
index 94f2963..cf8aba3 100644
--- a/ipconfig.h
+++ b/ipconfig.h
@@ -69,6 +69,8 @@
kReleaseReasonStaticIP
};
+ typedef base::Callback<void(const IPConfigRefPtr&)> Callback;
+
IPConfig(ControlInterface *control_interface, const std::string &device_name);
IPConfig(ControlInterface *control_interface,
const std::string &device_name,
@@ -82,25 +84,32 @@
std::string GetRpcIdentifier();
// Registers a callback that's executed every time the configuration
- // properties change. Takes ownership of |callback|. Pass NULL to remove a
- // callback. The callback's first argument is a pointer to this IP
+ // properties are acquired. Takes ownership of |callback|. Pass NULL
+ // to remove a callback. The callback's argument is a pointer to this IP
// configuration instance allowing clients to more easily manage multiple IP
- // configurations. The callback's second argument is set to false if IP
- // configuration failed.
- void RegisterUpdateCallback(
- const base::Callback<void(const IPConfigRefPtr&, bool)> &callback);
+ // configurations.
+ void RegisterUpdateCallback(const Callback &callback);
+
+ // Registers a callback that's executed every time the configuration
+ // properties fail to be acquired. Takes ownership of |callback|. Pass NULL
+ // to remove a callback. The callback's argument is a pointer to this IP
+ // configuration instance allowing clients to more easily manage multiple IP
+ // configurations.
+ void RegisterFailureCallback(const Callback &callback);
// Registers a callback that's executed every time the Refresh method
// on the ipconfig is called. Takes ownership of |callback|. Pass NULL
// to remove a callback. The callback's argument is a pointer to this IP
// configuration instance allowing clients to more easily manage multiple IP
// configurations.
- void RegisterRefreshCallback(
- const base::Callback<void(const IPConfigRefPtr&)> &callback);
+ void RegisterRefreshCallback(const Callback &callback);
void set_properties(const Properties &props) { properties_ = props; }
virtual const Properties &properties() const { return properties_; }
+ // Reset the IPConfig properties to their default values.
+ virtual void ResetProperties();
+
// Request, renew and release IP configuration. Return true on success, false
// otherwise. The default implementation always returns false indicating a
// failure. ReleaseIP is advisory: if we are no longer connected, it is not
@@ -134,8 +143,11 @@
virtual void EmitChanges();
// Updates the IP configuration properties and notifies registered listeners
- // about the event. |success| is set to false if the IP configuration failed.
- virtual void UpdateProperties(const Properties &properties, bool success);
+ // about the event.
+ virtual void UpdateProperties(const Properties &properties);
+
+ // Notifies registered listeners that the configuration process has failed.
+ virtual void NotifyFailure();
// |id_suffix| is appended to the storage id, intended to bind this instance
// to its associated device.
@@ -169,8 +181,9 @@
const uint serial_;
scoped_ptr<IPConfigAdaptorInterface> adaptor_;
Properties properties_;
- base::Callback<void(const IPConfigRefPtr&, bool)> update_callback_;
- base::Callback<void(const IPConfigRefPtr&)> refresh_callback_;
+ Callback update_callback_;
+ Callback failure_callback_;
+ Callback refresh_callback_;
DISALLOW_COPY_AND_ASSIGN(IPConfig);
};
diff --git a/ipconfig_unittest.cc b/ipconfig_unittest.cc
index 915f568..7b6cf70 100644
--- a/ipconfig_unittest.cc
+++ b/ipconfig_unittest.cc
@@ -34,17 +34,54 @@
class IPConfigTest : public Test {
public:
IPConfigTest() : ipconfig_(new IPConfig(&control_, kDeviceName)) {}
- void DropRef(const IPConfigRefPtr &/*ipconfig*/, bool /*config_success*/) {
+ void DropRef(const IPConfigRefPtr &/*ipconfig*/) {
ipconfig_ = NULL;
}
+ MOCK_METHOD1(OnIPConfigUpdated, void(const IPConfigRefPtr &ipconfig));
+ MOCK_METHOD1(OnIPConfigFailed, void(const IPConfigRefPtr &ipconfig));
+ MOCK_METHOD1(OnIPConfigRefreshed, void(const IPConfigRefPtr &ipconfig));
+
protected:
IPConfigMockAdaptor *GetAdaptor() {
return dynamic_cast<IPConfigMockAdaptor *>(ipconfig_->adaptor_.get());
}
- void UpdateProperties(const IPConfig::Properties &properties, bool success) {
- ipconfig_->UpdateProperties(properties, success);
+ void UpdateProperties(const IPConfig::Properties &properties) {
+ ipconfig_->UpdateProperties(properties);
+ }
+
+ void NotifyFailure() {
+ ipconfig_->NotifyFailure();
+ }
+
+ void ExpectPropertiesEqual(const IPConfig::Properties &properties) {
+ EXPECT_EQ(properties.address, ipconfig_->properties().address);
+ EXPECT_EQ(properties.subnet_prefix, ipconfig_->properties().subnet_prefix);
+ EXPECT_EQ(properties.broadcast_address,
+ ipconfig_->properties().broadcast_address);
+ EXPECT_EQ(properties.dns_servers.size(),
+ ipconfig_->properties().dns_servers.size());
+ if (properties.dns_servers.size() ==
+ ipconfig_->properties().dns_servers.size()) {
+ for (size_t i = 0; i < properties.dns_servers.size(); ++i) {
+ EXPECT_EQ(properties.dns_servers[i],
+ ipconfig_->properties().dns_servers[i]);
+ }
+ }
+ EXPECT_EQ(properties.domain_search.size(),
+ ipconfig_->properties().domain_search.size());
+ if (properties.domain_search.size() ==
+ ipconfig_->properties().domain_search.size()) {
+ for (size_t i = 0; i < properties.domain_search.size(); ++i) {
+ EXPECT_EQ(properties.domain_search[i],
+ ipconfig_->properties().domain_search[i]);
+ }
+ }
+ EXPECT_EQ(properties.gateway, ipconfig_->properties().gateway);
+ EXPECT_EQ(properties.blackhole_ipv6,
+ ipconfig_->properties().blackhole_ipv6);
+ EXPECT_EQ(properties.mtu, ipconfig_->properties().mtu);
}
MockControl control_;
@@ -89,95 +126,51 @@
properties.address = "1.2.3.4";
properties.subnet_prefix = 24;
properties.broadcast_address = "11.22.33.44";
- properties.gateway = "5.6.7.8";
properties.dns_servers.push_back("10.20.30.40");
properties.dns_servers.push_back("20.30.40.50");
properties.domain_name = "foo.org";
properties.domain_search.push_back("zoo.org");
properties.domain_search.push_back("zoo.com");
+ properties.gateway = "5.6.7.8";
properties.blackhole_ipv6 = true;
properties.mtu = 700;
- UpdateProperties(properties, true);
- EXPECT_EQ("1.2.3.4", ipconfig_->properties().address);
- EXPECT_EQ(24, ipconfig_->properties().subnet_prefix);
- EXPECT_EQ("11.22.33.44", ipconfig_->properties().broadcast_address);
- EXPECT_EQ("5.6.7.8", ipconfig_->properties().gateway);
- ASSERT_EQ(2, ipconfig_->properties().dns_servers.size());
- EXPECT_EQ("10.20.30.40", ipconfig_->properties().dns_servers[0]);
- EXPECT_EQ("20.30.40.50", ipconfig_->properties().dns_servers[1]);
- ASSERT_EQ(2, ipconfig_->properties().domain_search.size());
- EXPECT_EQ("zoo.org", ipconfig_->properties().domain_search[0]);
- EXPECT_EQ("zoo.com", ipconfig_->properties().domain_search[1]);
- EXPECT_EQ("foo.org", ipconfig_->properties().domain_name);
- EXPECT_TRUE(ipconfig_->properties().blackhole_ipv6);
- EXPECT_EQ(700, ipconfig_->properties().mtu);
+ UpdateProperties(properties);
+ ExpectPropertiesEqual(properties);
+
+ // We should not reset on NotifyFailure.
+ NotifyFailure();
+ ExpectPropertiesEqual(properties);
+
+ // We should reset if ResetProperties is called.
+ ipconfig_->ResetProperties();
+ ExpectPropertiesEqual(IPConfig::Properties());
}
-namespace {
-
-class UpdateCallbackTest {
- public:
- UpdateCallbackTest(const IPConfigRefPtr &ipconfig, bool success)
- : ipconfig_(ipconfig),
- success_(success),
- called_(false) {}
-
- void Callback(const IPConfigRefPtr &ipconfig, bool success) {
- called_ = true;
- EXPECT_EQ(ipconfig_.get(), ipconfig.get());
- EXPECT_EQ(success_, success);
- }
-
- bool called() const { return called_; }
-
- private:
- IPConfigRefPtr ipconfig_;
- bool success_;
- bool called_;
-};
-
-} // namespace {}
-
-TEST_F(IPConfigTest, UpdateCallback) {
- for (int success = 0; success < 2; success++) {
- UpdateCallbackTest callback_test(ipconfig_, success);
- ASSERT_FALSE(callback_test.called());
- ipconfig_->RegisterUpdateCallback(
- Bind(&UpdateCallbackTest::Callback, Unretained(&callback_test)));
- UpdateProperties(IPConfig::Properties(), success);
- EXPECT_TRUE(callback_test.called());
- }
-}
-
-namespace {
-
-class RefreshCallbackTest {
- public:
- RefreshCallbackTest(const IPConfigRefPtr &ipconfig)
- : ipconfig_(ipconfig),
- called_(false) {}
-
- void Callback(const IPConfigRefPtr &ipconfig) {
- called_ = true;
- EXPECT_EQ(ipconfig_.get(), ipconfig.get());
- }
-
- bool called() const { return called_; }
-
- private:
- IPConfigRefPtr ipconfig_;
- bool called_;
-};
-
-} // namespace {}
-
-TEST_F(IPConfigTest, RefreshCallback) {
- RefreshCallbackTest callback_test(ipconfig_);
- ASSERT_FALSE(callback_test.called());
+TEST_F(IPConfigTest, Callbacks) {
+ ipconfig_->RegisterUpdateCallback(
+ Bind(&IPConfigTest::OnIPConfigUpdated, Unretained(this)));
+ ipconfig_->RegisterFailureCallback(
+ Bind(&IPConfigTest::OnIPConfigFailed, Unretained(this)));
ipconfig_->RegisterRefreshCallback(
- Bind(&RefreshCallbackTest::Callback, Unretained(&callback_test)));
+ Bind(&IPConfigTest::OnIPConfigRefreshed, Unretained(this)));
+
+ EXPECT_CALL(*this, OnIPConfigUpdated(ipconfig_));
+ EXPECT_CALL(*this, OnIPConfigFailed(ipconfig_)).Times(0);
+ EXPECT_CALL(*this, OnIPConfigRefreshed(ipconfig_)).Times(0);
+ UpdateProperties(IPConfig::Properties());
+ Mock::VerifyAndClearExpectations(this);
+
+ EXPECT_CALL(*this, OnIPConfigUpdated(ipconfig_)).Times(0);
+ EXPECT_CALL(*this, OnIPConfigFailed(ipconfig_));
+ EXPECT_CALL(*this, OnIPConfigRefreshed(ipconfig_)).Times(0);
+ NotifyFailure();
+ Mock::VerifyAndClearExpectations(this);
+
+ EXPECT_CALL(*this, OnIPConfigUpdated(ipconfig_)).Times(0);
+ EXPECT_CALL(*this, OnIPConfigFailed(ipconfig_)).Times(0);
+ EXPECT_CALL(*this, OnIPConfigRefreshed(ipconfig_));
ipconfig_->Refresh(NULL);
- EXPECT_TRUE(callback_test.called());
+ Mock::VerifyAndClearExpectations(this);
}
TEST_F(IPConfigTest, UpdatePropertiesWithDropRef) {
@@ -185,7 +178,7 @@
// IPConfig object without crashing.
ipconfig_->RegisterUpdateCallback(
Bind(&IPConfigTest::DropRef, Unretained(this)));
- UpdateProperties(IPConfig::Properties(), true);
+ UpdateProperties(IPConfig::Properties());
}
TEST_F(IPConfigTest, PropertyChanges) {
@@ -205,12 +198,21 @@
IPConfig::Properties ip_properties;
EXPECT_CALL(*adaptor, EmitStringChanged(kAddressProperty, _));
EXPECT_CALL(*adaptor, EmitStringsChanged(kNameServersProperty, _));
- UpdateProperties(ip_properties, true);
+ UpdateProperties(ip_properties);
+ Mock::VerifyAndClearExpectations(adaptor);
+
+ // It is the callback's responsibility for resetting the IPConfig
+ // properties (via IPConfig::ResetProperties()). Since NotifyFailure
+ // by itself doesn't change any properties, it should not emit any
+ // property change events either.
+ EXPECT_CALL(*adaptor, EmitStringChanged(_, _)).Times(0);
+ EXPECT_CALL(*adaptor, EmitStringsChanged(_, _)).Times(0);
+ NotifyFailure();
Mock::VerifyAndClearExpectations(adaptor);
EXPECT_CALL(*adaptor, EmitStringChanged(kAddressProperty, _));
EXPECT_CALL(*adaptor, EmitStringsChanged(kNameServersProperty, _));
- UpdateProperties(ip_properties, false);
+ ipconfig_->ResetProperties();
Mock::VerifyAndClearExpectations(adaptor);
}
diff --git a/mock_ipconfig.h b/mock_ipconfig.h
index aed364f..7e23305 100644
--- a/mock_ipconfig.h
+++ b/mock_ipconfig.h
@@ -24,6 +24,7 @@
MOCK_METHOD0(RequestIP, bool(void));
MOCK_METHOD0(RenewIP, bool(void));
MOCK_METHOD1(ReleaseIP, bool(ReleaseReason reason));
+ MOCK_METHOD0(ResetProperties, void(void));
MOCK_METHOD2(Load, bool(StoreInterface *storage,
const std::string &id_suffix));
diff --git a/routing_table_unittest.cc b/routing_table_unittest.cc
index c38f960..d9391de 100644
--- a/routing_table_unittest.cc
+++ b/routing_table_unittest.cc
@@ -481,7 +481,7 @@
IPConfig::Properties properties;
properties.address_family = IPAddress::kFamilyIPv4;
vector<IPConfig::Route> &routes = properties.routes;
- ipconfig->UpdateProperties(properties, true);
+ ipconfig->UpdateProperties(properties);
const int kMetric = 10;
EXPECT_TRUE(routing_table_->ConfigureRoutes(kTestDeviceIndex0,
@@ -493,7 +493,7 @@
route.netmask = kTestRemoteNetmask4;
route.gateway = kTestGatewayAddress4;
routes.push_back(route);
- ipconfig->UpdateProperties(properties, true);
+ ipconfig->UpdateProperties(properties);
IPAddress destination_address(IPAddress::kFamilyIPv4);
IPAddress source_address(IPAddress::kFamilyIPv4);
@@ -526,7 +526,7 @@
routes.push_back(route);
route.host = kTestRemoteNetwork4;
routes.push_back(route);
- ipconfig->UpdateProperties(properties, true);
+ ipconfig->UpdateProperties(properties);
EXPECT_CALL(rtnl_handler_,
SendMessage(IsRoutingPacket(RTNLMessage::kModeAdd,
diff --git a/virtual_device.cc b/virtual_device.cc
index cab8416..7de67a3 100644
--- a/virtual_device.cc
+++ b/virtual_device.cc
@@ -61,7 +61,7 @@
set_ipconfig(new IPConfig(control_interface(), link_name()));
}
ipconfig()->set_properties(properties);
- OnIPConfigUpdated(ipconfig(), true);
+ OnIPConfigUpdated(ipconfig());
}
void VirtualDevice::DropConnection() {
diff --git a/wifi_unittest.cc b/wifi_unittest.cc
index d1c4d9f..db2e78a 100644
--- a/wifi_unittest.cc
+++ b/wifi_unittest.cc
@@ -682,7 +682,7 @@
uint16 frequency,
const char *mode);
void ReportIPConfigComplete() {
- wifi_->OnIPConfigUpdated(dhcp_config_, true);
+ wifi_->OnIPConfigUpdated(dhcp_config_);
}
void ReportIPConfigFailure() {
wifi_->OnIPConfigFailure();