shill: Start assigning service state
Service state is usually controlled by the associated Device. Devices
select a single Service to reflect current connection state. All other
Services remain in an idle or whatever Failure state they ended up with
at the end of their last attempt.
When Service state changes, the service notifies the Manager of its new
state. This will be used by the Manager to update service priority and
selection of default routes.
For unit-tests, add a "State" test for service_unittest which which
tests for state changes and up-calls to the Manager. Add
"SelectedDevice" test to device_unittest to ensure proper down-calls
to the service.
BUG=chromium-os:19523
TEST=New unit tests
Change-Id: Ic253cc1dd77821a74176346521aff5948ad59660
Reviewed-on: http://gerrit.chromium.org/gerrit/6495
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/device.cc b/device.cc
index 69e6551..fb1f5cb 100644
--- a/device.cc
+++ b/device.cc
@@ -214,9 +214,11 @@
if (success) {
CreateConnection();
connection_->UpdateFromIPConfig(ipconfig);
+ SetServiceState(Service::kStateConnected);
} else {
// TODO(pstew): This logic gets more complex when multiple IPConfig types
// are run in parallel (e.g. DHCP and DHCP6)
+ SetServiceState(Service::kStateDisconnected);
DestroyConnection();
}
}
@@ -233,6 +235,27 @@
connection_ = NULL;
}
+void Device::SelectService(const ServiceRefPtr &service) {
+ VLOG(2) << __func__ << ": " << service->UniqueName();
+ if (selected_service_.get() &&
+ selected_service_->state() != Service::kStateFailure) {
+ selected_service_->SetState(Service::kStateIdle);
+ }
+ selected_service_ = service;
+}
+
+void Device::SetServiceState(Service::ConnectState state) {
+ if (selected_service_.get()) {
+ selected_service_->SetState(state);
+ }
+}
+
+void Device::SetServiceFailure(Service::ConnectFailure failure_state) {
+ if (selected_service_.get()) {
+ selected_service_->SetFailure(failure_state);
+ }
+}
+
string Device::SerializeIPConfigs(const string &suffix) {
return StringPrintf("%s:%s", suffix.c_str(), ipconfig_->type().c_str());
}
diff --git a/device.h b/device.h
index 008d370..cd3aeeb 100644
--- a/device.h
+++ b/device.h
@@ -16,6 +16,7 @@
#include "shill/ipconfig.h"
#include "shill/property_store.h"
#include "shill/refptr_types.h"
+#include "shill/service.h"
#include "shill/shill_event.h"
namespace shill {
@@ -87,6 +88,7 @@
FRIEND_TEST(DeviceTest, DestroyIPConfigNULL);
FRIEND_TEST(DeviceTest, GetProperties);
FRIEND_TEST(DeviceTest, Save);
+ FRIEND_TEST(DeviceTest, SelectedService);
// If there's an IP configuration in |ipconfig_|, releases the IP address and
// destroys the configuration instance.
@@ -104,6 +106,17 @@
// Remove connection state
void DestroyConnection();
+ // Selects a service to be "current" -- i.e. link-state or configuration
+ // events that happen to the device are attributed to this service.
+ void SelectService(const ServiceRefPtr &service);
+
+ // Set the state of the selected service
+ void SetServiceState(Service::ConnectState state);
+
+ // Set the failure of the selected service (implicitly sets the state to
+ // "failure")
+ void SetServiceFailure(Service::ConnectFailure failure_state);
+
void HelpRegisterDerivedStrings(const std::string &name,
Strings(Device::*get)(void),
bool(Device::*set)(const Strings&));
@@ -146,6 +159,9 @@
scoped_ptr<DeviceAdaptorInterface> adaptor_;
+ // Maintain a reference to the connected / connecting service
+ ServiceRefPtr selected_service_;
+
// Cache singleton pointer for performance and test purposes.
DHCPProvider *dhcp_provider_;
diff --git a/device_unittest.cc b/device_unittest.cc
index 74256a7..a2f2a68 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -23,6 +23,7 @@
#include "shill/mock_device.h"
#include "shill/mock_glib.h"
#include "shill/mock_ipconfig.h"
+#include "shill/mock_service.h"
#include "shill/mock_store.h"
#include "shill/property_store_unittest.h"
#include "shill/shill_event.h"
@@ -34,6 +35,7 @@
using ::testing::AtLeast;
using ::testing::NiceMock;
using ::testing::Return;
+using ::testing::StrictMock;
using ::testing::Test;
using ::testing::Values;
@@ -169,4 +171,32 @@
EXPECT_EQ(string::npos, to_process.find('/'));
}
+TEST_F(DeviceTest, SelectedService) {
+ EXPECT_FALSE(device_->selected_service_.get());
+ device_->SetServiceState(Service::kStateAssociating);
+ scoped_refptr<MockService> service(
+ new StrictMock<MockService>(&control_interface_,
+ &dispatcher_,
+ &manager_));
+ device_->SelectService(service);
+ EXPECT_TRUE(device_->selected_service_.get() == service.get());
+
+ EXPECT_CALL(*service.get(), SetState(Service::kStateConfiguring));
+ device_->SetServiceState(Service::kStateConfiguring);
+ EXPECT_CALL(*service.get(), SetFailure(Service::kFailureOutOfRange));
+ device_->SetServiceFailure(Service::kFailureOutOfRange);
+
+ // Service should be returned to "Idle" state
+ EXPECT_CALL(*service.get(), state())
+ .WillOnce(Return(Service::kStateUnknown));
+ EXPECT_CALL(*service.get(), SetState(Service::kStateIdle));
+ device_->SelectService(NULL);
+
+ // A service in the "Failure" state should not be reset to "Idle"
+ device_->SelectService(service);
+ EXPECT_CALL(*service.get(), state())
+ .WillOnce(Return(Service::kStateFailure));
+ device_->SelectService(NULL);
+}
+
} // namespace shill
diff --git a/ethernet.cc b/ethernet.cc
index 9b0b3f4..f1149f7 100644
--- a/ethernet.cc
+++ b/ethernet.cc
@@ -75,11 +75,17 @@
link_up_ = true;
manager_->RegisterService(service_);
if (service_->auto_connect()) {
- LOG_IF(ERROR, !AcquireDHCPConfig()) << "Unable to acquire DHCP config.";
+ if (AcquireDHCPConfig()) {
+ SelectService(service_);
+ SetServiceState(Service::kStateConfiguring);
+ } else {
+ LOG(ERROR) << "Unable to acquire DHCP config.";
+ }
}
} else if ((flags & IFF_LOWER_UP) == 0 && link_up_) {
link_up_ = false;
manager_->DeregisterService(service_);
+ SelectService(NULL);
DestroyIPConfig();
}
}
diff --git a/manager.cc b/manager.cc
index 1d04521..034f40e 100644
--- a/manager.cc
+++ b/manager.cc
@@ -190,6 +190,13 @@
}
}
+void Manager::UpdateService(const ServiceConstRefPtr &to_update) {
+ LOG(INFO) << "Service " << to_update->UniqueName() << " updated;"
+ << " state: " << to_update->state() << " failure: "
+ << to_update->failure();
+ // TODO(pstew): This should trigger re-sorting of services, autoconnect, etc.
+}
+
void Manager::FilterByTechnology(Device::Technology tech,
vector<DeviceRefPtr> *found) {
CHECK(found);
diff --git a/manager.h b/manager.h
index 4b6fcba..6fd4553 100644
--- a/manager.h
+++ b/manager.h
@@ -60,6 +60,7 @@
void RegisterService(const ServiceRefPtr &to_manage);
void DeregisterService(const ServiceConstRefPtr &to_forget);
+ virtual void UpdateService(const ServiceConstRefPtr &to_update);
void FilterByTechnology(Device::Technology tech,
std::vector<DeviceRefPtr> *found);
diff --git a/mock_manager.h b/mock_manager.h
index 815bcfa..e8f709d 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -29,6 +29,7 @@
MOCK_METHOD0(device_info, DeviceInfo*(void));
MOCK_METHOD0(store, PropertyStore*(void));
+ MOCK_METHOD1(UpdateService, void(const ServiceConstRefPtr &to_update));
private:
DISALLOW_COPY_AND_ASSIGN(MockManager);
diff --git a/mock_service.h b/mock_service.h
index e82987c..371ced4 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -28,6 +28,10 @@
MOCK_METHOD1(Connect, void(Error *error));
MOCK_METHOD0(Disconnect, void());
MOCK_METHOD0(CalculateState, std::string());
+ MOCK_METHOD1(SetState, void(ConnectState state));
+ MOCK_CONST_METHOD0(state, ConnectState());
+ MOCK_METHOD1(SetFailure, void(ConnectFailure failure));
+ MOCK_CONST_METHOD0(failure, ConnectFailure());
MOCK_METHOD0(GetDeviceRpcId, std::string());
MOCK_CONST_METHOD0(GetRpcIdentifier, std::string());
MOCK_METHOD1(GetStorageIdentifier, std::string(const std::string &));
diff --git a/service.cc b/service.cc
index 491a24c..1a3c6f7 100644
--- a/service.cc
+++ b/service.cc
@@ -64,7 +64,9 @@
Service::Service(ControlInterface *control_interface,
EventDispatcher *dispatcher,
Manager *manager)
- : auto_connect_(false),
+ : state_(kStateUnknown),
+ failure_(kFailureUnknown),
+ auto_connect_(false),
check_portal_(kCheckPortalAuto),
connectable_(false),
favorite_(false),
@@ -153,6 +155,30 @@
NOTREACHED() << "Attempt to activate a non-cellular service?";
}
+bool Service::IsActive() {
+ return state_ != kStateUnknown &&
+ state_ != kStateIdle &&
+ state_ != kStateFailure;
+}
+
+void Service::SetState(ConnectState state) {
+ if (state == state_) {
+ return;
+ }
+
+ state_ = state;
+ if (state != kStateFailure) {
+ failure_ = kFailureUnknown;
+ }
+ manager_->UpdateService(this);
+ // TODO(pstew): Broadcast property change notification via control interface
+}
+
+void Service::SetFailure(ConnectFailure failure) {
+ failure_ = failure;
+ SetState(kStateFailure);
+}
+
string Service::GetRpcIdentifier() const {
return adaptor_->GetRpcIdentifier();
}
diff --git a/service.h b/service.h
index a44d856..3edf054 100644
--- a/service.h
+++ b/service.h
@@ -42,25 +42,25 @@
static const char kCheckPortalTrue[];
enum ConnectFailure {
- kServiceFailureUnknown,
- kServiceFailureActivationFailure,
- kServiceFailureOutOfRange,
- kServiceFailurePinMissing,
- kServiceFailureConfigurationFailed,
- kServiceFailureBadCredentials,
- kServiceFailureNeedEVDO,
- kServiceFailureNeedHomeNetwork,
- kServiceFailureOTASPFailure,
- kServiceFailureAAAFailure
+ kFailureUnknown,
+ kFailureActivationFailure,
+ kFailureOutOfRange,
+ kFailurePinMissing,
+ kFailureConfigurationFailed,
+ kFailureBadCredentials,
+ kFailureNeedEVDO,
+ kFailureNeedHomeNetwork,
+ kFailureOTASPFailure,
+ kFailureAAAFailure
};
enum ConnectState {
- kServiceStateUnknown,
- kServiceStateIdle,
- kServiceStateAssociating,
- kServiceStateConfiguring,
- kServiceStateConnected,
- kServiceStateDisconnected,
- kServiceStateFailure
+ kStateUnknown,
+ kStateIdle,
+ kStateAssociating,
+ kStateConfiguring,
+ kStateConnected,
+ kStateDisconnected,
+ kStateFailure
};
struct EapCredentials {
EapCredentials() : use_system_cas(false) {}
@@ -93,7 +93,16 @@
// The default implementation asserts.
virtual void ActivateCellularModem(const std::string &carrier);
- virtual bool IsActive() { return false; }
+ virtual bool IsActive();
+
+ virtual ConnectState state() const { return state_; }
+ // Updates the state of the Service and alerts the manager. Also
+ // clears |failure_| if the new state isn't a failure.
+ virtual void SetState(ConnectState state);
+
+ virtual ConnectFailure failure() const { return failure_; }
+ // Records the failure mode, and sets the Service state to "Failure".
+ virtual void SetFailure(ConnectFailure failure);
// Returns a string that is guaranteed to uniquely identify this Service
// instance.
@@ -153,6 +162,8 @@
void SaveEapCredentials(StoreInterface *storage, const std::string &id);
// Properties
+ ConnectState state_;
+ ConnectFailure failure_;
bool auto_connect_;
std::string check_portal_;
bool connectable_;
@@ -170,6 +181,7 @@
private:
friend class ServiceAdaptorInterface;
+ FRIEND_TEST(DeviceTest, SelectedService);
FRIEND_TEST(ServiceTest, Constructor);
FRIEND_TEST(ServiceTest, Save);
FRIEND_TEST(ServiceTest, SaveString);
diff --git a/service_unittest.cc b/service_unittest.cc
index 269379f..f549750 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -18,9 +18,10 @@
#include "shill/manager.h"
#include "shill/mock_adaptors.h"
#include "shill/mock_control.h"
-#include "shill/mock_service.h"
+#include "shill/mock_manager.h"
#include "shill/mock_store.h"
#include "shill/property_store_unittest.h"
+#include "shill/service.h"
#include "shill/shill_event.h"
using std::map;
@@ -35,42 +36,55 @@
namespace shill {
+// This is a simple Service subclass with all the pure-virutal methods stubbed
+class ServiceUnderTest : public Service {
+ public:
+ static const char kRpcId[];
+ static const char kStorageId[];
+
+ ServiceUnderTest(ControlInterface *control_interface,
+ EventDispatcher *dispatcher,
+ Manager *manager)
+ : Service(control_interface, dispatcher, manager) {}
+ virtual ~ServiceUnderTest() {}
+
+ virtual void Connect(Error *error) {}
+ virtual void Disconnect() {}
+ virtual string CalculateState() { return ""; }
+ virtual string GetRpcIdentifier() const { return ServiceMockAdaptor::kRpcId; }
+ virtual string GetDeviceRpcId() { return kRpcId; }
+ virtual string GetStorageIdentifier(const string &mac) { return kStorageId; }
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(ServiceUnderTest);
+};
+
+const char ServiceUnderTest::kRpcId[] = "mock-device-rpc";
+const char ServiceUnderTest::kStorageId[] = "service";
+
class ServiceTest : public PropertyStoreTest {
public:
- static const char kMockDeviceRpcId[];
- static const char kProfileName[];
-
ServiceTest()
- : service_(new MockService(&control_interface_,
- &dispatcher_,
- &manager_)),
- storage_id_("service") {
- EXPECT_CALL(*service_.get(), GetStorageIdentifier(_))
- .WillRepeatedly(Return(storage_id_));
- EXPECT_CALL(*service_.get(), GetRpcIdentifier())
- .WillRepeatedly(Return(ServiceMockAdaptor::kRpcId));
- }
+ : mock_manager_(&control_interface_, &dispatcher_, &glib_),
+ service_(new ServiceUnderTest(&control_interface_,
+ &dispatcher_,
+ &mock_manager_)),
+ storage_id_(ServiceUnderTest::kStorageId) {}
virtual ~ServiceTest() {}
protected:
- scoped_refptr<MockService> service_;
+ MockManager mock_manager_;
+ scoped_refptr<ServiceUnderTest> service_;
string storage_id_;
};
-const char ServiceTest::kMockDeviceRpcId[] = "mock-device-rpc";
-
-const char ServiceTest::kProfileName[] = "profile";
-
TEST_F(ServiceTest, Constructor) {
EXPECT_TRUE(service_->save_credentials_);
EXPECT_EQ(Service::kCheckPortalAuto, service_->check_portal_);
}
TEST_F(ServiceTest, GetProperties) {
- EXPECT_CALL(*service_.get(), CalculateState()).WillRepeatedly(Return(""));
- EXPECT_CALL(*service_.get(), GetDeviceRpcId())
- .WillRepeatedly(Return(ServiceTest::kMockDeviceRpcId));
map<string, ::DBus::Variant> props;
Error error(Error::kInvalidProperty, "");
{
@@ -117,7 +131,7 @@
DBusAdaptor::GetProperties(service_->store(), &props, &dbus_error);
ASSERT_FALSE(props.find(flimflam::kDeviceProperty) == props.end());
EXPECT_EQ(props[flimflam::kDeviceProperty].reader().get_string(),
- string(ServiceTest::kMockDeviceRpcId));
+ string(ServiceUnderTest::kRpcId));
}
}
@@ -214,4 +228,26 @@
EXPECT_TRUE(service_->Save(&storage, ""));
}
+TEST_F(ServiceTest, State) {
+ EXPECT_EQ(Service::kStateUnknown, service_->state());
+ EXPECT_EQ(Service::kFailureUnknown, service_->failure());
+
+ ServiceConstRefPtr service_ref(service_);
+ EXPECT_CALL(mock_manager_, UpdateService(service_ref));
+ service_->SetState(Service::kStateConnected);
+ // A second state change shouldn't cause another update
+ service_->SetState(Service::kStateConnected);
+
+ EXPECT_EQ(Service::kStateConnected, service_->state());
+ EXPECT_EQ(Service::kFailureUnknown, service_->failure());
+ EXPECT_CALL(mock_manager_, UpdateService(service_ref));
+ service_->SetState(Service::kStateDisconnected);
+
+ EXPECT_CALL(mock_manager_, UpdateService(service_ref));
+ service_->SetFailure(Service::kFailureOutOfRange);
+
+ EXPECT_EQ(Service::kStateFailure, service_->state());
+ EXPECT_EQ(Service::kFailureOutOfRange, service_->failure());
+}
+
} // namespace shill