Remove D-Bus dependency from Manager
Here are the main changes:
1. Cleanup Manager::CreateService and Manager::RemoveService API
to remove D-Bus dependency.
1. Use refptr for Service since it will be maintained by both
Manager and its adaptor.
2. Move the monitoring of the service creator/owner to the adaptor,
since the DBusServiceWatcher is D-Bus specific.
Bug: 24194427
TEST=Start AP service on Brillo board, stop weaved, verify service
is destroyed and removed from apmanager.
TEST=Verify device setup works on Brillo board.
TEST=Verify apmanager does not crash on restart.
TEST=Run unittests on both Brillo and Chrome OS.
Change-Id: I33fd4eec2c1adf12830484ca087bd9dd56767288
diff --git a/dbus/dbus_control.cc b/dbus/dbus_control.cc
index d682fb8..c29421b 100644
--- a/dbus/dbus_control.cc
+++ b/dbus/dbus_control.cc
@@ -60,8 +60,6 @@
// Create and register Manager.
manager_.reset(new Manager(this));
manager_->RegisterAsync(
- object_manager_.get(),
- bus_,
sequencer->GetHandler("Manager.RegisterAsync() failed.", true));
// Take over the service ownership once the objects registration is completed.
diff --git a/dbus/manager_dbus_adaptor.cc b/dbus/manager_dbus_adaptor.cc
index a205cea..bfc4e6d 100644
--- a/dbus/manager_dbus_adaptor.cc
+++ b/dbus/manager_dbus_adaptor.cc
@@ -36,6 +36,7 @@
Manager* manager)
: adaptor_(this),
dbus_object_(object_manager, bus, ManagerAdaptor::GetObjectPath()),
+ bus_(bus),
manager_(manager) {}
ManagerDBusAdaptor::~ManagerDBusAdaptor() {}
@@ -46,18 +47,67 @@
dbus_object_.RegisterAsync(completion_callback);
}
-bool ManagerDBusAdaptor::CreateService(brillo::ErrorPtr* error,
+bool ManagerDBusAdaptor::CreateService(brillo::ErrorPtr* dbus_error,
dbus::Message* message,
dbus::ObjectPath* out_service) {
- // TODO(zqiu): to be implemented.
+ auto service = manager_->CreateService();
+ if (!service) {
+ brillo::Error::AddTo(dbus_error,
+ FROM_HERE,
+ brillo::errors::dbus::kDomain,
+ kErrorInternalError,
+ "Failed to create new service");
+ return false;
+ }
+
+ *out_service = service->adaptor()->GetRpcObjectIdentifier();
+
+ // Setup monitoring for the service's remote owner.
+ service_owner_watchers_[*out_service] =
+ ServiceOwnerWatcherContext(
+ service,
+ std::unique_ptr<DBusServiceWatcher>(
+ new DBusServiceWatcher(
+ bus_,
+ message->GetSender(),
+ base::Bind(&ManagerDBusAdaptor::OnServiceOwnerVanished,
+ base::Unretained(this),
+ *out_service))));
return true;
}
-bool ManagerDBusAdaptor::RemoveService(brillo::ErrorPtr* error,
+bool ManagerDBusAdaptor::RemoveService(brillo::ErrorPtr* dbus_error,
dbus::Message* message,
const dbus::ObjectPath& in_service) {
- // TODO(zqiu): to be implemented.
- return true;
+ auto watcher_context = service_owner_watchers_.find(in_service);
+ if (watcher_context == service_owner_watchers_.end()) {
+ brillo::Error::AddToPrintf(
+ dbus_error,
+ FROM_HERE,
+ brillo::errors::dbus::kDomain,
+ kErrorInvalidArguments,
+ "Service %s not found",
+ in_service.value().c_str());
+ return false;
+ }
+
+ Error error;
+ manager_->RemoveService(watcher_context->second.service, &error);
+ service_owner_watchers_.erase(watcher_context);
+ return !error.ToDBusError(dbus_error);
+}
+
+void ManagerDBusAdaptor::OnServiceOwnerVanished(
+ const dbus::ObjectPath& service_path) {
+ LOG(INFO) << "Owner for service " << service_path.value() << " vanished";
+ // Remove service watcher.
+ auto watcher_context = service_owner_watchers_.find(service_path);
+ CHECK(watcher_context != service_owner_watchers_.end())
+ << "Owner vanished without watcher setup.";
+
+ // Tell Manager to remove this service.
+ manager_->RemoveService(watcher_context->second.service, nullptr);
+ service_owner_watchers_.erase(watcher_context);
}
} // namespace apmanager
diff --git a/dbus/manager_dbus_adaptor.h b/dbus/manager_dbus_adaptor.h
index 1a2f4cd..a3c05ee 100644
--- a/dbus/manager_dbus_adaptor.h
+++ b/dbus/manager_dbus_adaptor.h
@@ -17,7 +17,10 @@
#ifndef APMANAGER_DBUS_MANAGER_DBUS_ADAPTOR_H_
#define APMANAGER_DBUS_MANAGER_DBUS_ADAPTOR_H_
+#include <map>
+
#include <base/macros.h>
+#include <brillo/dbus/dbus_service_watcher.h>
#include <dbus_bindings/org.chromium.apmanager.Manager.h>
#include "apmanager/manager_adaptor_interface.h"
@@ -25,6 +28,7 @@
namespace apmanager {
class Manager;
+class Service;
class ManagerDBusAdaptor : public org::chromium::apmanager::ManagerInterface,
public ManagerAdaptorInterface {
@@ -35,10 +39,10 @@
~ManagerDBusAdaptor() override;
// Implementation of org::chromium::apmanager::ManagerInterface.
- bool CreateService(brillo::ErrorPtr* error,
+ bool CreateService(brillo::ErrorPtr* dbus_error,
dbus::Message* message,
dbus::ObjectPath* out_service) override;
- bool RemoveService(brillo::ErrorPtr* error,
+ bool RemoveService(brillo::ErrorPtr* dbus_error,
dbus::Message* message,
const dbus::ObjectPath& in_service) override;
@@ -47,9 +51,28 @@
const base::Callback<void(bool)>& completion_callback) override;
private:
+ using DBusServiceWatcher = brillo::dbus_utils::DBusServiceWatcher;
+ // Context for service owner watcher.
+ struct ServiceOwnerWatcherContext {
+ ServiceOwnerWatcherContext() {}
+ ServiceOwnerWatcherContext(const scoped_refptr<Service>& in_service,
+ std::unique_ptr<DBusServiceWatcher> in_watcher)
+ : service(in_service),
+ watcher(std::move(in_watcher)) {}
+ scoped_refptr<Service> service;
+ std::unique_ptr<DBusServiceWatcher> watcher;
+ };
+
+ // Invoked when the owner of a Service vanished.
+ void OnServiceOwnerVanished(const dbus::ObjectPath& service_path);
+
org::chromium::apmanager::ManagerAdaptor adaptor_;
brillo::dbus_utils::DBusObject dbus_object_;
+ scoped_refptr<dbus::Bus> bus_;
Manager* manager_;
+ // Map of service path to owner monitor context.
+ std::map<dbus::ObjectPath, ServiceOwnerWatcherContext>
+ service_owner_watchers_;
DISALLOW_COPY_AND_ASSIGN(ManagerDBusAdaptor);
};
diff --git a/manager.cc b/manager.cc
index c5b1cb4..88c54ef 100644
--- a/manager.cc
+++ b/manager.cc
@@ -18,51 +18,29 @@
#include <base/bind.h>
-#if !defined(__ANDROID__)
-#include <chromeos/dbus/service_constants.h>
-#else
-#include "dbus/apmanager/dbus-constants.h"
-#endif // __ANDROID__
+#include "apmanager/control_interface.h"
+#include "apmanager/manager.h"
-using brillo::dbus_utils::AsyncEventSequencer;
-using brillo::dbus_utils::ExportedObjectManager;
-using brillo::dbus_utils::DBusMethodResponse;
using std::string;
namespace apmanager {
Manager::Manager(ControlInterface* control_interface)
- : org::chromium::apmanager::ManagerAdaptor(this),
- control_interface_(control_interface),
+ : control_interface_(control_interface),
service_identifier_(0),
- device_info_(this) {}
+ device_info_(this),
+ adaptor_(control_interface->CreateManagerAdaptor(this)) {}
-Manager::~Manager() {
- // Terminate all services before cleanup other resources.
- for (auto& service : services_) {
- service.reset();
- }
-}
+Manager::~Manager() {}
void Manager::RegisterAsync(
- ExportedObjectManager* object_manager,
- const scoped_refptr<dbus::Bus>& bus,
const base::Callback<void(bool)>& completion_callback) {
- CHECK(!dbus_object_) << "Already registered";
- dbus_object_.reset(
- new brillo::dbus_utils::DBusObject(
- object_manager,
- bus,
- org::chromium::apmanager::ManagerAdaptor::GetObjectPath()));
- RegisterWithDBusObject(dbus_object_.get());
- dbus_object_->RegisterAsync(completion_callback);
- bus_ = bus;
-
- shill_manager_.Init(control_interface_);
- firewall_manager_.Init(control_interface_);
+ adaptor_->RegisterAsync(completion_callback);
}
void Manager::Start() {
+ shill_manager_.Init(control_interface_);
+ firewall_manager_.Init(control_interface_);
device_info_.Start();
}
@@ -70,51 +48,26 @@
device_info_.Stop();
}
-bool Manager::CreateService(brillo::ErrorPtr* error,
- dbus::Message* message,
- dbus::ObjectPath* out_service) {
+scoped_refptr<Service> Manager::CreateService() {
LOG(INFO) << "Manager::CreateService";
- std::unique_ptr<Service> service(new Service(this, service_identifier_));
- *out_service = service->adaptor()->GetRpcObjectIdentifier();
- services_.push_back(std::move(service));
-
- base::Closure on_connection_vanish = base::Bind(
- &Manager::OnAPServiceOwnerDisappeared,
- base::Unretained(this),
- service_identifier_);
- service_watchers_[service_identifier_].reset(
- new DBusServiceWatcher{bus_, message->GetSender(), on_connection_vanish});
- service_identifier_++;
-
- return true;
+ scoped_refptr<Service> service = new Service(this, service_identifier_++);
+ services_.push_back(service);
+ return service;
}
-bool Manager::RemoveService(brillo::ErrorPtr* error,
- dbus::Message* message,
- const dbus::ObjectPath& in_service) {
+bool Manager::RemoveService(const scoped_refptr<Service>& service,
+ Error* error) {
for (auto it = services_.begin(); it != services_.end(); ++it) {
- if ((*it)->adaptor()->GetRpcObjectIdentifier() == in_service) {
- // Verify the owner.
- auto watcher = service_watchers_.find((*it)->identifier());
- CHECK(watcher != service_watchers_.end())
- << "DBus watcher not created for service: " << (*it)->identifier();
- if (watcher->second->connection_name() != message->GetSender()) {
- brillo::Error::AddToPrintf(
- error, FROM_HERE, brillo::errors::dbus::kDomain, kManagerError,
- "Service %d is owned by another local process.",
- (*it)->identifier());
- return false;
- }
- service_watchers_.erase(watcher);
-
+ if (*it == service) {
services_.erase(it);
return true;
}
}
- brillo::Error::AddTo(
- error, FROM_HERE, brillo::errors::dbus::kDomain, kManagerError,
- "Service does not exist");
+ Error::PopulateAndLog(error,
+ Error::kInvalidArguments,
+ "Service does not exist",
+ FROM_HERE);
return false;
}
@@ -138,7 +91,7 @@
return nullptr;
}
-void Manager::RegisterDevice(scoped_refptr<Device> device) {
+void Manager::RegisterDevice(const scoped_refptr<Device>& device) {
LOG(INFO) << "Manager::RegisterDevice: registering device "
<< device->GetDeviceName();
devices_.push_back(device);
@@ -171,23 +124,4 @@
firewall_manager_.ReleaseDHCPPortAccess(interface);
}
-void Manager::OnAPServiceOwnerDisappeared(int service_identifier) {
- LOG(INFO) << "Owner for service " << service_identifier << " disappeared";
- // Remove service watcher.
- auto watcher = service_watchers_.find(service_identifier);
- CHECK(watcher != service_watchers_.end())
- << "Owner disappeared without watcher setup";
- service_watchers_.erase(watcher);
-
- // Remove the service.
- for (auto it = services_.begin(); it != services_.end(); ++it) {
- if ((*it)->identifier() == service_identifier) {
- services_.erase(it);
- return;
- }
- }
- LOG(INFO) << "Owner for service " << service_identifier
- << " disappeared before it is registered";
-}
-
} // namespace apmanager
diff --git a/manager.h b/manager.h
index 06a9c3b..578a1db 100644
--- a/manager.h
+++ b/manager.h
@@ -17,52 +17,43 @@
#ifndef APMANAGER_MANAGER_H_
#define APMANAGER_MANAGER_H_
-#include <map>
#include <string>
#include <vector>
#include <base/macros.h>
-#include <brillo/dbus/dbus_service_watcher.h>
#include "apmanager/device_info.h"
#include "apmanager/firewall_manager.h"
+#include "apmanager/manager_adaptor_interface.h"
#include "apmanager/service.h"
#include "apmanager/shill_manager.h"
-#include "dbus_bindings/org.chromium.apmanager.Manager.h"
namespace apmanager {
class ControlInterface;
-class Manager : public org::chromium::apmanager::ManagerAdaptor,
- public org::chromium::apmanager::ManagerInterface {
+class Manager {
public:
- template<typename T>
- using DBusMethodResponse = brillo::dbus_utils::DBusMethodResponse<T>;
-
explicit Manager(ControlInterface* control_interface);
virtual ~Manager();
- // Implementation of ManagerInterface.
- // Handles calls to org.chromium.apmanager.Manager.CreateService().
- virtual bool CreateService(brillo::ErrorPtr* error,
- dbus::Message* message,
- dbus::ObjectPath* out_service);
- // Handles calls to org.chromium.apmanager.Manager.RemoveService().
- virtual bool RemoveService(brillo::ErrorPtr* error,
- dbus::Message* message,
- const dbus::ObjectPath& in_service);
+ // Register this object to the RPC interface asynchronously.
+ void RegisterAsync(const base::Callback<void(bool)>& completion_callback);
- // Register DBus object.
- void RegisterAsync(
- brillo::dbus_utils::ExportedObjectManager* object_manager,
- const scoped_refptr<dbus::Bus>& bus,
- const base::Callback<void(bool)>& completion_callback);
+ // Create and return a new Service instance. The newly created instance
+ // will be added to the service list, it will only get deleted via
+ // RemoveService.
+ scoped_refptr<Service> CreateService();
+
+ // Remove |service| from the service list. Return true if service is found
+ // and deleted from the list, false otherwise. |error| will be populated
+ // on failure.
+ bool RemoveService(const scoped_refptr<Service>& service, Error* error);
virtual void Start();
virtual void Stop();
- virtual void RegisterDevice(scoped_refptr<Device> device);
+ virtual void RegisterDevice(const scoped_refptr<Device>& device);
// Return an unuse device with AP interface mode support.
virtual scoped_refptr<Device> GetAvailableDevice();
@@ -94,18 +85,9 @@
private:
friend class ManagerTest;
- // This is invoked when the owner of an AP service disappeared.
- void OnAPServiceOwnerDisappeared(int service_identifier);
-
ControlInterface* control_interface_;
int service_identifier_;
- std::unique_ptr<brillo::dbus_utils::DBusObject> dbus_object_;
- scoped_refptr<dbus::Bus> bus_;
- std::vector<std::unique_ptr<Service>> services_;
std::vector<scoped_refptr<Device>> devices_;
- // DBus service watchers for the owner of AP services.
- using DBusServiceWatcher = brillo::dbus_utils::DBusServiceWatcher;
- std::map<int, std::unique_ptr<DBusServiceWatcher>> service_watchers_;
DeviceInfo device_info_;
// Manager for communicating with shill (connection manager).
@@ -113,6 +95,12 @@
// Manager for communicating with remote firewall service.
FirewallManager firewall_manager_;
+ // Put the service list after ShillManager and FirewallManager, since both
+ // are needed for tearing down an active/running Service.
+ std::vector<scoped_refptr<Service>> services_;
+
+ std::unique_ptr<ManagerAdaptorInterface> adaptor_;
+
DISALLOW_COPY_AND_ASSIGN(Manager);
};
diff --git a/mock_manager.h b/mock_manager.h
index d67d87e..f8b54b7 100644
--- a/mock_manager.h
+++ b/mock_manager.h
@@ -33,7 +33,7 @@
MOCK_METHOD0(Start, void());
MOCK_METHOD0(Stop, void());
- MOCK_METHOD1(RegisterDevice, void(scoped_refptr<Device> device));
+ MOCK_METHOD1(RegisterDevice, void(const scoped_refptr<Device>& device));
MOCK_METHOD0(GetAvailableDevice, scoped_refptr<Device>());
MOCK_METHOD1(GetDeviceFromInterfaceName,
scoped_refptr<Device>(const std::string& interface_name));
diff --git a/service.h b/service.h
index 80dccea..e1b6a05 100644
--- a/service.h
+++ b/service.h
@@ -21,6 +21,7 @@
#include <base/callback.h>
#include <base/macros.h>
+#include <base/memory/ref_counted.h>
#include <base/memory/weak_ptr.h>
#include <brillo/process.h>
@@ -40,7 +41,7 @@
class EventDispatcher;
#endif // __BRILLO__
-class Service {
+class Service : public base::RefCounted<Service> {
public:
Service(Manager* manager, int service_identifier);
virtual ~Service();
diff --git a/service_unittest.cc b/service_unittest.cc
index a23e06c..34d8721 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -76,7 +76,7 @@
.WillByDefault(ReturnNew<FakeConfigAdaptor>());
// Defer creation of Service object to allow ControlInterface to
// setup expectations for generating fake adaptors.
- service_.reset(new Service(&manager_, kServiceIdentifier));
+ service_ = new Service(&manager_, kServiceIdentifier);
}
virtual void SetUp() {
service_->dhcp_server_factory_ = &dhcp_server_factory_;
@@ -116,7 +116,7 @@
MockFileWriter file_writer_;
MockProcessFactory process_factory_;
MockHostapdMonitor* hostapd_monitor_;
- std::unique_ptr<Service> service_;
+ scoped_refptr<Service> service_;
};
TEST_F(ServiceTest, StartWhenServiceAlreadyRunning) {
