shill: Provider: Create common superclass
Make all *Provider classes in shill that are responsible for
managing Service objects descend from a common parent class.
This will allow the Manager to refactor repeated code related
to providers. The parent class is non-virtual for now, and
implements stubs for all methods, so subclasses that do not
yet implement the full interface can safely omit these methods.
A future CL will flesh out these implementations and change
this to a pure-virtual Interface class.
BUG=chromium:265518
TEST=Unit tests
Change-Id: If4ee03a511f4732e3067ce134e501f73e421ace1
Reviewed-on: https://gerrit.chromium.org/gerrit/63604
Commit-Queue: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/Makefile b/Makefile
index c9a075f..68463db 100644
--- a/Makefile
+++ b/Makefile
@@ -275,6 +275,7 @@
profile_dbus_adaptor.o \
profile_dbus_property_exporter.o \
property_store.o \
+ provider.o \
proxy_factory.o \
routing_table.o \
resolver.o \
@@ -476,6 +477,7 @@
profile_unittest.o \
property_accessor_unittest.o \
property_store_unittest.o \
+ provider_unittest.o \
resolver_unittest.o \
result_aggregator_unittest.o \
routing_table_unittest.o \
diff --git a/ethernet_eap_provider.cc b/ethernet_eap_provider.cc
index 5661e5b..a26d15f 100644
--- a/ethernet_eap_provider.cc
+++ b/ethernet_eap_provider.cc
@@ -24,6 +24,13 @@
EthernetEapProvider::~EthernetEapProvider() {}
+ServiceRefPtr EthernetEapProvider::GetService(const KeyValueStore &args,
+ Error *error) {
+ CHECK_EQ(kTypeEthernetEap, args.LookupString(flimflam::kTypeProperty, ""))
+ << "Service type must be Ethernet EAP!";
+ return service();
+}
+
void EthernetEapProvider::Start() {
if (!service_) {
service_ = new EthernetEapService(control_interface_,
diff --git a/ethernet_eap_provider.h b/ethernet_eap_provider.h
index 8bd3601..3250438 100644
--- a/ethernet_eap_provider.h
+++ b/ethernet_eap_provider.h
@@ -9,17 +9,20 @@
#include <base/callback.h>
+#include "shill/provider.h"
#include "shill/refptr_types.h"
namespace shill {
class ControlInterface;
+class Error;
class Ethernet;
class EventDispatcher;
+class KeyValueStore;
class Manager;
class Metrics;
-class EthernetEapProvider {
+class EthernetEapProvider : public Provider {
public:
typedef base::Callback<void()> CredentialChangeCallback;
@@ -29,8 +32,11 @@
Manager *manager);
virtual ~EthernetEapProvider();
- virtual void Start();
- virtual void Stop();
+ // Called by Manager as a part of the Provider interface.
+ virtual ServiceRefPtr GetService(const KeyValueStore &args,
+ Error *error) override;
+ virtual void Start() override;
+ virtual void Stop() override;
virtual const ServiceRefPtr &service() const { return service_; }
diff --git a/manager.cc b/manager.cc
index b202777..9812b39 100644
--- a/manager.cc
+++ b/manager.cc
@@ -1659,7 +1659,7 @@
string type = args.GetString(flimflam::kTypeProperty);
if (type == kTypeEthernetEap) {
SLOG(Manager, 2) << __func__ << ": getting Ethernet EAP Service";
- return ethernet_eap_provider_->service();
+ return ethernet_eap_provider_->GetService(args, error);
}
if (type == flimflam::kTypeVPN) {
SLOG(Manager, 2) << __func__ << ": getting VPN Service";
diff --git a/mock_wifi_provider.h b/mock_wifi_provider.h
index 79bd579..880ed2f 100644
--- a/mock_wifi_provider.h
+++ b/mock_wifi_provider.h
@@ -21,12 +21,12 @@
MOCK_METHOD0(Stop, void());
MOCK_METHOD1(CreateServicesFromProfile, void(const ProfileRefPtr &profile));
MOCK_CONST_METHOD2(FindSimilarService,
- WiFiServiceRefPtr(const KeyValueStore &args,
- Error *error));
+ ServiceRefPtr(const KeyValueStore &args,
+ Error *error));
MOCK_METHOD2(CreateTemporaryService,
- WiFiServiceRefPtr(const KeyValueStore &args, Error *error));
- MOCK_METHOD2(GetService, WiFiServiceRefPtr(const KeyValueStore &args,
- Error *error));
+ ServiceRefPtr(const KeyValueStore &args, Error *error));
+ MOCK_METHOD2(GetService, ServiceRefPtr(const KeyValueStore &args,
+ Error *error));
MOCK_METHOD1(FindServiceForEndpoint,
WiFiServiceRefPtr(const WiFiEndpointConstRefPtr &endpoint));
MOCK_METHOD1(OnEndpointAdded, void(const WiFiEndpointConstRefPtr &endpoint));
diff --git a/provider.cc b/provider.cc
new file mode 100644
index 0000000..973555e
--- /dev/null
+++ b/provider.cc
@@ -0,0 +1,40 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/provider.h"
+
+#include "shill/error.h"
+#include "shill/service.h"
+
+namespace shill {
+
+Provider::Provider() {}
+
+void Provider::CreateServicesFromProfile(const ProfileRefPtr &/*profile*/) {}
+
+ServiceRefPtr Provider::FindSimilarService(
+ const KeyValueStore &/*args*/, Error *error) const {
+ error->Populate(Error::kInternalError,
+ "FindSimilarService is not implemented");
+ return NULL;
+}
+
+ServiceRefPtr Provider::GetService(const KeyValueStore &/*args*/,
+ Error *error) {
+ error->Populate(Error::kInternalError, "GetService is not implemented");
+ return NULL;
+}
+
+ServiceRefPtr Provider::CreateTemporaryService(
+ const KeyValueStore &/*args*/, Error *error) {
+ error->Populate(Error::kInternalError,
+ "CreateTemporaryService is not implemented");
+ return NULL;
+}
+
+void Provider::Start() {}
+
+void Provider::Stop() {}
+
+} // namespace shill
diff --git a/provider.h b/provider.h
new file mode 100644
index 0000000..dad9ffa
--- /dev/null
+++ b/provider.h
@@ -0,0 +1,62 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SHILL_PROVIDER_INTERFACE_H_
+#define SHILL_PROVIDER_INTERFACE_H_
+
+#include "shill/refptr_types.h"
+
+namespace shill {
+
+class Error;
+class KeyValueStore;
+
+// This is an object that creates and manages service objects. It provides
+// default implementations for each provider method so sublcasses do not
+// need to implement boilerplate unimplemented methods.
+class Provider {
+ public:
+ virtual ~Provider() {}
+
+ // Creates services from the entries within |profile|.
+ virtual void CreateServicesFromProfile(const ProfileRefPtr &profile);
+
+ // Find a Service with similar properties to |args|. The criteria
+ // used are specific to the provider subclass. Returns a reference
+ // to a matching service if one exists. Otherwise it returns a NULL
+ // reference and populates |error|.
+ virtual ServiceRefPtr FindSimilarService(
+ const KeyValueStore &args, Error *error) const;
+
+ // Retrieves (see FindSimilarService) or creates a service with the
+ // unique attributes in |args|. The remaining attributes will be
+ // populated (by Manager) via a later call to Service::Configure().
+ // Returns a NULL reference and populates |error| on failure.
+ virtual ServiceRefPtr GetService(const KeyValueStore &args, Error *error);
+
+ // Create a temporary service with the identifying properties populated
+ // from |args|. Callers outside of the Provider must never register
+ // this service with the Manager or connect it since it was never added
+ // to the provider's service list.
+ virtual ServiceRefPtr CreateTemporaryService(
+ const KeyValueStore &args, Error *error);
+
+ // Start the provider.
+ virtual void Start();
+
+ // Stop the provider (will de-register all services).
+ virtual void Stop();
+
+ protected:
+ Provider();
+
+ private:
+ friend class ProviderTest;
+
+ DISALLOW_COPY_AND_ASSIGN(Provider);
+};
+
+} // namespace shill
+
+#endif // SHILL_PROVIDER_INTERFACE_H_
diff --git a/provider_unittest.cc b/provider_unittest.cc
new file mode 100644
index 0000000..ef59c86
--- /dev/null
+++ b/provider_unittest.cc
@@ -0,0 +1,40 @@
+// Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "shill/provider.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include "shill/error.h"
+#include "shill/key_value_store.h"
+#include "shill/profile.h"
+#include "shill/service.h"
+
+namespace shill {
+
+class ProviderTest : public testing::Test {
+ protected:
+ Provider provider_;
+};
+
+TEST_F(ProviderTest, Stubs) {
+ // Expect that we do not crash.
+ provider_.CreateServicesFromProfile(ProfileRefPtr());
+
+ KeyValueStore args;
+
+ {
+ Error error;
+ EXPECT_EQ(NULL, provider_.FindSimilarService(args, &error).get());
+ EXPECT_EQ(Error::kInternalError, error.type());
+ }
+ {
+ Error error;
+ EXPECT_EQ(NULL, provider_.GetService(args, &error).get());
+ EXPECT_EQ(Error::kInternalError, error.type());
+ }
+}
+
+} // namespace shill
diff --git a/vpn_provider.cc b/vpn_provider.cc
index 77f066f..0e2474b 100644
--- a/vpn_provider.cc
+++ b/vpn_provider.cc
@@ -39,8 +39,8 @@
void VPNProvider::Stop() {}
-VPNServiceRefPtr VPNProvider::GetService(const KeyValueStore &args,
- Error *error) {
+ServiceRefPtr VPNProvider::GetService(const KeyValueStore &args,
+ Error *error) {
SLOG(VPN, 2) << __func__;
string type = args.LookupString(flimflam::kProviderTypeProperty, "");
if (type.empty()) {
@@ -92,7 +92,7 @@
}
}
-void VPNProvider::CreateServicesFromProfile(ProfileRefPtr profile) {
+void VPNProvider::CreateServicesFromProfile(const ProfileRefPtr &profile) {
SLOG(VPN, 2) << __func__;
const StoreInterface *storage = profile->GetConstStorage();
set<string> groups =
diff --git a/vpn_provider.h b/vpn_provider.h
index cf9d95f..3fbb847 100644
--- a/vpn_provider.h
+++ b/vpn_provider.h
@@ -11,6 +11,7 @@
#include <base/basictypes.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
+#include "shill/provider.h"
#include "shill/refptr_types.h"
namespace shill {
@@ -22,7 +23,7 @@
class Manager;
class Metrics;
-class VPNProvider {
+class VPNProvider : public Provider {
public:
VPNProvider(ControlInterface *control_interface,
EventDispatcher *dispatcher,
@@ -30,10 +31,14 @@
Manager *manager);
virtual ~VPNProvider();
- virtual void Start();
- virtual void Stop();
-
- VPNServiceRefPtr GetService(const KeyValueStore &args, Error *error);
+ // Called by Manager as a part of the Provider interface. The attributes
+ // used for matching services for the VPN provider are the ProviderType,
+ // ProviderHost mode and Name parameters.
+ virtual void CreateServicesFromProfile(const ProfileRefPtr &profile) override;
+ virtual ServiceRefPtr GetService(const KeyValueStore &args,
+ Error *error) override;
+ virtual void Start() override;
+ virtual void Stop() override;
// Offers an unclaimed interface to VPN services. Returns true if this
// device has been accepted by a service.
@@ -45,8 +50,6 @@
// services_ vector.
void RemoveService(VPNServiceRefPtr service);
- void CreateServicesFromProfile(ProfileRefPtr profile);
-
// Returns true if any of the managed VPN services is connecting or connected.
virtual bool HasActiveService() const;
diff --git a/vpn_provider_unittest.cc b/vpn_provider_unittest.cc
index aa23a2f..7d8a583 100644
--- a/vpn_provider_unittest.cc
+++ b/vpn_provider_unittest.cc
@@ -72,7 +72,7 @@
KeyValueStore args;
Error e;
args.SetString(flimflam::kTypeProperty, flimflam::kTypeVPN);
- VPNServiceRefPtr service = provider_.GetService(args, &e);
+ ServiceRefPtr service = provider_.GetService(args, &e);
EXPECT_EQ(Error::kNotSupported, e.type());
EXPECT_FALSE(service);
}
@@ -84,7 +84,7 @@
args.SetString(flimflam::kProviderTypeProperty, "unknown-vpn-type");
args.SetString(flimflam::kProviderHostProperty, kHost);
args.SetString(flimflam::kNameProperty, kName);
- VPNServiceRefPtr service = provider_.GetService(args, &e);
+ ServiceRefPtr service = provider_.GetService(args, &e);
EXPECT_EQ(Error::kNotSupported, e.type());
EXPECT_FALSE(service);
}
@@ -98,7 +98,7 @@
args.SetString(flimflam::kNameProperty, kName);
EXPECT_CALL(manager_, device_info()).WillOnce(Return(&device_info_));
EXPECT_CALL(manager_, RegisterService(_));
- VPNServiceRefPtr service0 = provider_.GetService(args, &e);
+ ServiceRefPtr service0 = provider_.GetService(args, &e);
EXPECT_TRUE(e.IsSuccess());
ASSERT_TRUE(service0);
EXPECT_EQ("vpn_10_8_0_1_vpn_name", service0->GetStorageIdentifier());
@@ -109,7 +109,7 @@
EXPECT_TRUE(e.IsSuccess());
// A second call should return the same service.
- VPNServiceRefPtr service1 = provider_.GetService(args, &e);
+ ServiceRefPtr service1 = provider_.GetService(args, &e);
EXPECT_TRUE(e.IsSuccess());
ASSERT_EQ(service0.get(), service1.get());
}
diff --git a/wifi_provider.cc b/wifi_provider.cc
index 2fda3b4..36b1e09 100644
--- a/wifi_provider.cc
+++ b/wifi_provider.cc
@@ -158,7 +158,7 @@
}
}
-WiFiServiceRefPtr WiFiProvider::FindSimilarService(
+ServiceRefPtr WiFiProvider::FindSimilarService(
const KeyValueStore &args, Error *error) const {
vector<uint8_t> ssid;
string mode;
@@ -178,7 +178,7 @@
return service;
}
-WiFiServiceRefPtr WiFiProvider::CreateTemporaryService(
+ServiceRefPtr WiFiProvider::CreateTemporaryService(
const KeyValueStore &args, Error *error) {
vector<uint8_t> ssid;
string mode;
@@ -201,7 +201,12 @@
hidden_ssid);
}
-WiFiServiceRefPtr WiFiProvider::GetService(
+ServiceRefPtr WiFiProvider::GetService(
+ const KeyValueStore &args, Error *error) {
+ return GetWiFiService(args, error);
+}
+
+WiFiServiceRefPtr WiFiProvider::GetWiFiService(
const KeyValueStore &args, Error *error) {
vector<uint8_t> ssid_bytes;
string mode;
diff --git a/wifi_provider.h b/wifi_provider.h
index b77ed96..e968d86 100644
--- a/wifi_provider.h
+++ b/wifi_provider.h
@@ -13,6 +13,7 @@
#include <gtest/gtest_prod.h> // for FRIEND_TEST
#include "shill/accessor_interface.h" // for ByteArrays
+#include "shill/provider.h"
#include "shill/refptr_types.h"
namespace shill {
@@ -31,7 +32,7 @@
// The WiFi Provider is the holder of all WiFi Services. It holds both
// visible (created due to an Endpoint becoming visible) and invisible
// (created due to user or storage configuration) Services.
-class WiFiProvider {
+class WiFiProvider : public Provider {
public:
static const char kStorageFrequencies[];
static const int kMaxStorageFrequencies;
@@ -55,25 +56,18 @@
Manager *manager);
virtual ~WiFiProvider();
- virtual void Start();
- virtual void Stop();
-
- // Called by Manager.
- virtual void CreateServicesFromProfile(const ProfileRefPtr &profile);
- virtual WiFiServiceRefPtr GetService(const KeyValueStore &args, Error *error);
-
- // Find a Service with the same SSID, mode and security as provided
- // in |args|. Returns a reference to a matching service if one
- // exists. Otherwise it returns a NULL reference and populates |error|.
- virtual WiFiServiceRefPtr FindSimilarService(
- const KeyValueStore &args, Error *error) const;
-
- // Create a temporary WiFiService with the mode, ssid, security and
- // hidden properties populated from |args|. Callers outside of the
- // WiFiProvider must must never register this service with the Manager
- // or connect it since it was never added to the provider's service list.
- virtual WiFiServiceRefPtr CreateTemporaryService(
- const KeyValueStore &args, Error *error);
+ // Called by Manager as a part of the Provider interface. The attributes
+ // used for matching services for the WiFi provider are the SSID, mode and
+ // security parameters.
+ virtual void CreateServicesFromProfile(const ProfileRefPtr &profile) override;
+ virtual ServiceRefPtr GetService(const KeyValueStore &args,
+ Error *error) override;
+ virtual ServiceRefPtr FindSimilarService(
+ const KeyValueStore &args, Error *error) const override;
+ virtual ServiceRefPtr CreateTemporaryService(
+ const KeyValueStore &args, Error *error) override;
+ virtual void Start() override;
+ virtual void Stop() override;
// Find a Service this Endpoint should be associated with.
virtual WiFiServiceRefPtr FindServiceForEndpoint(
@@ -155,6 +149,10 @@
const std::string &mode,
const std::string &security) const;
+ // Returns a WiFiServiceRefPtr for unit tests and for down-casting to a
+ // ServiceRefPtr in GetService().
+ WiFiServiceRefPtr GetWiFiService(const KeyValueStore &args, Error *error);
+
// Disassociate the service from its WiFi device and remove it from the
// services_ vector.
void ForgetService(const WiFiServiceRefPtr &service);
diff --git a/wifi_provider_unittest.cc b/wifi_provider_unittest.cc
index 64aaf13..9eef99f 100644
--- a/wifi_provider_unittest.cc
+++ b/wifi_provider_unittest.cc
@@ -211,12 +211,12 @@
}
}
- WiFiServiceRefPtr CreateTemporaryService(const char *ssid,
- const char *mode,
- const char *security,
- bool is_hidden,
- bool provide_hidden,
- Error *error) {
+ ServiceRefPtr CreateTemporaryService(const char *ssid,
+ const char *mode,
+ const char *security,
+ bool is_hidden,
+ bool provide_hidden,
+ Error *error) {
KeyValueStore args;
SetServiceParameters(
ssid, mode, security, is_hidden, provide_hidden, &args);
@@ -232,7 +232,11 @@
KeyValueStore args;
SetServiceParameters(
ssid, mode, security, is_hidden, provide_hidden, &args);
- return provider_.GetService(args, error);
+ return provider_.GetWiFiService(args, error);
+ }
+
+ WiFiServiceRefPtr GetWiFiService(const KeyValueStore &args, Error *error) {
+ return provider_.GetWiFiService(args, error);
}
WiFiServiceRefPtr FindService(const vector<uint8_t> &ssid,
@@ -716,12 +720,12 @@
true, true, &args);
EXPECT_CALL(manager_, RegisterService(_)).Times(1);
Error get_service_error;
- WiFiServiceRefPtr service = provider_.GetService(args, &get_service_error);
+ WiFiServiceRefPtr service = GetWiFiService(args, &get_service_error);
EXPECT_EQ(1, GetServices().size());
{
Error error;
- WiFiServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
+ ServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
EXPECT_EQ(service.get(), find_service.get());
EXPECT_TRUE(error.IsSuccess());
}
@@ -730,7 +734,7 @@
{
Error error;
- WiFiServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
+ ServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
EXPECT_EQ(service.get(), find_service.get());
EXPECT_TRUE(error.IsSuccess());
}
@@ -739,7 +743,7 @@
{
Error error;
- WiFiServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
+ ServiceRefPtr find_service = provider_.FindSimilarService(args, &error);
EXPECT_EQ(NULL, find_service.get());
EXPECT_EQ(Error::kNotFound, error.type());
}
@@ -758,7 +762,7 @@
Mock::VerifyAndClearExpectations(&manager_);
EXPECT_CALL(manager_, RegisterService(_)).Times(0);
- WiFiServiceRefPtr service1 =
+ ServiceRefPtr service1 =
CreateTemporaryService(kSSID.c_str(), flimflam::kModeManaged,
flimflam::kSecurityNone, true, true, &error);
diff --git a/wimax_provider.cc b/wimax_provider.cc
index 3d0e536..cbe9ca6 100644
--- a/wimax_provider.cc
+++ b/wimax_provider.cc
@@ -147,8 +147,8 @@
return true;
}
-WiMaxServiceRefPtr WiMaxProvider::GetService(const KeyValueStore &args,
- Error *error) {
+ServiceRefPtr WiMaxProvider::GetService(const KeyValueStore &args,
+ Error *error) {
SLOG(WiMax, 2) << __func__;
CHECK_EQ(args.GetString(flimflam::kTypeProperty), flimflam::kTypeWimax);
WiMaxNetworkId id = args.LookupString(WiMaxService::kNetworkIdProperty, "");
diff --git a/wimax_provider.h b/wimax_provider.h
index ca18448..56838f3 100644
--- a/wimax_provider.h
+++ b/wimax_provider.h
@@ -13,6 +13,7 @@
#include "shill/accessor_interface.h"
#include "shill/dbus_manager.h"
+#include "shill/provider.h"
#include "shill/refptr_types.h"
#include "shill/wimax_network_proxy_interface.h"
@@ -26,7 +27,7 @@
class ProxyFactory;
class WiMaxManagerProxyInterface;
-class WiMaxProvider {
+class WiMaxProvider : public Provider {
public:
WiMaxProvider(ControlInterface *control,
EventDispatcher *dispatcher,
@@ -34,8 +35,14 @@
Manager *manager);
virtual ~WiMaxProvider();
- void Start();
- void Stop();
+ // Called by Manager as a part of the Provider interface. The attributes
+ // used for matching services for the WiMax provider are the NetworkId,
+ // mode and Name parameters.
+ virtual void CreateServicesFromProfile(const ProfileRefPtr &profile) override;
+ virtual ServiceRefPtr GetService(const KeyValueStore &args,
+ Error *error) override;
+ void Start() override;
+ void Stop() override;
// Signaled by DeviceInfo when a new WiMAX device becomes available.
virtual void OnDeviceInfoAvailable(const std::string &link_name);
@@ -47,14 +54,6 @@
// this provider has released ownership of the service, and false otherwise.
virtual bool OnServiceUnloaded(const WiMaxServiceRefPtr &service);
- // Creates if necessary and configures a WiMAX service with the given
- // parameters. Used by Manager::GetService.
- WiMaxServiceRefPtr GetService(const KeyValueStore &args, Error *error);
-
- // Creates and registers all WiMAX services available in |profile|. Used by
- // Manager::PushProfile.
- void CreateServicesFromProfile(const ProfileRefPtr &profile);
-
// Selects and returns a WiMAX device to connect |service| through.
virtual WiMaxRefPtr SelectCarrier(const WiMaxServiceConstRefPtr &service);
diff --git a/wimax_provider_unittest.cc b/wimax_provider_unittest.cc
index 01c768b..a1bcf78 100644
--- a/wimax_provider_unittest.cc
+++ b/wimax_provider_unittest.cc
@@ -491,7 +491,7 @@
args.SetString(flimflam::kTypeProperty, flimflam::kTypeWimax);
// No network id property.
- WiMaxServiceRefPtr service = provider_.GetService(args, &e);
+ ServiceRefPtr service = provider_.GetService(args, &e);
EXPECT_EQ(Error::kInvalidArguments, e.type());
EXPECT_FALSE(service);