Use asynchronous handler for org.chromium.Service.Start D-Bus call

This allows us to handle this D-Bus call asynchronously for Brillo
devices.  Since we want to wait for the newly created AP interface
to be enumerated before starting the service.

For now, this handler will return synchronously on both Chrome OS
and Brillo.  A separate CL (CL:178864) will update this handler to
return asynchronously on Brillo.

While there, remove the unnecessary mock function for Service.Start
and Service.Stop, since these are D-Bus method handlers.  Even though
the MockService class is now empty, we might still need it when we
remove the D-Bus dependency from the Service class (b/24194427),
so keep it for now.

Bug: 25113165
TEST=Unittest on both Chrome OS and Brillo
TEST=Manual test on both Chrome OS and Brillo

Change-Id: If0ba13ae89a25bdf9207803d7d6b008e9ad6165e

5 files changed, 26 insertions, 10 deletions

diff --git a/dbus_bindings/org.chromium.apmanager.Service.dbus-xml b/dbus_bindings/org.chromium.apmanager.Service.dbus-xml
index c17a2f9..86ce8c8 100644
--- a/dbus_bindings/org.chromium.apmanager.Service.dbus-xml
+++ b/dbus_bindings/org.chromium.apmanager.Service.dbus-xml
@@ -6,6 +6,7 @@
       <tp:docstring>
         Start the service.
       </tp:docstring>
+      <annotation name="org.chromium.DBus.Method.Kind" value="async"/>
     </method>
     <method name="Stop">
       <tp:docstring>
diff --git a/mock_service.h b/mock_service.h
index 4265486..2464e6d 100644
--- a/mock_service.h
+++ b/mock_service.h
@@ -29,9 +29,6 @@ class MockService : public Service {
   MockService();
   ~MockService() override;
 
-  MOCK_METHOD1(Start, bool(brillo::ErrorPtr *error));
-  MOCK_METHOD1(Stop, bool(brillo::ErrorPtr *error));
-
  private:
   DISALLOW_COPY_AND_ASSIGN(MockService);
 };
diff --git a/service.cc b/service.cc
index b3a6cd9..99073fb 100644
--- a/service.cc
+++ b/service.cc
@@ -30,6 +30,7 @@
 #include "apmanager/manager.h"
 
 using brillo::dbus_utils::AsyncEventSequencer;
+using brillo::dbus_utils::DBusMethodResponse;
 using brillo::dbus_utils::ExportedObjectManager;
 using org::chromium::apmanager::ManagerAdaptor;
 using std::string;
@@ -103,7 +104,7 @@ void Service::RegisterAsync(ExportedObjectManager* object_manager,
   config_->RegisterAsync(object_manager, bus, sequencer);
 }
 
-bool Service::Start(brillo::ErrorPtr* error) {
+bool Service::StartInternal(brillo::ErrorPtr* error) {
   if (IsHostapdRunning()) {
     brillo::Error::AddTo(
         error, FROM_HERE, brillo::errors::dbus::kDomain, kServiceError,
@@ -182,6 +183,16 @@ bool Service::Start(brillo::ErrorPtr* error) {
   return true;
 }
 
+void Service::Start(std::unique_ptr<DBusMethodResponse<>> response) {
+  brillo::ErrorPtr error;
+
+  if (!StartInternal(&error)) {
+    response->ReplyWithError(error.get());
+  } else {
+    response->Return();
+  }
+}
+
 bool Service::Stop(brillo::ErrorPtr* error) {
   if (!IsHostapdRunning()) {
     brillo::Error::AddTo(
diff --git a/service.h b/service.h
index b0e7976..e193943 100644
--- a/service.h
+++ b/service.h
@@ -39,9 +39,10 @@ class Service : public org::chromium::apmanager::ServiceAdaptor,
   Service(Manager* manager, int service_identifier);
   virtual ~Service();
 
-  // Implementation of ServiceInterface.
-  virtual bool Start(brillo::ErrorPtr* error);
-  virtual bool Stop(brillo::ErrorPtr* error);
+  // Implementation of org::chromium::apmanager::ServiceInterface.
+  void Start(
+      std::unique_ptr<brillo::dbus_utils::DBusMethodResponse<>> response);
+  bool Stop(brillo::ErrorPtr* error);
 
   // Register Service DBus object.
   void RegisterAsync(
@@ -65,6 +66,8 @@ class Service : public org::chromium::apmanager::ServiceAdaptor,
   static const char kStateStarted[];
   static const char kStateFailed[];
 
+  bool StartInternal(brillo::ErrorPtr* error);
+
   // Return true if hostapd process is currently running.
   bool IsHostapdRunning();
 
diff --git a/service_unittest.cc b/service_unittest.cc
index 6877bc1..313d3f9 100644
--- a/service_unittest.cc
+++ b/service_unittest.cc
@@ -73,6 +73,10 @@ class ServiceTest : public testing::Test {
     service_.hostapd_monitor_.reset(hostapd_monitor_);
   }
 
+  bool StartService(brillo::ErrorPtr* error) {
+    return service_.StartInternal(error);
+  }
+
   void StartDummyProcess() {
     service_.hostapd_process_.reset(new brillo::ProcessImpl);
     service_.hostapd_process_->AddArg(kBinSleep);
@@ -105,7 +109,7 @@ TEST_F(ServiceTest, StartWhenServiceAlreadyRunning) {
   StartDummyProcess();
 
   brillo::ErrorPtr error;
-  EXPECT_FALSE(service_.Start(&error));
+  EXPECT_FALSE(StartService(&error));
   EXPECT_THAT(error, IsServiceErrorStartingWith("Service already running"));
 }
 
@@ -115,7 +119,7 @@ TEST_F(ServiceTest, StartWhenConfigFileFailed) {
 
   brillo::ErrorPtr error;
   EXPECT_CALL(*config, GenerateConfigFile(_, _)).WillOnce(Return(false));
-  EXPECT_FALSE(service_.Start(&error));
+  EXPECT_FALSE(StartService(&error));
   EXPECT_THAT(error, IsServiceErrorStartingWith(
       "Failed to generate config file"));
 }
@@ -143,7 +147,7 @@ TEST_F(ServiceTest, StartSuccess) {
   EXPECT_CALL(*dhcp_server, Start()).WillOnce(Return(true));
   EXPECT_CALL(manager_, RequestDHCPPortAccess(_));
   EXPECT_CALL(*hostapd_monitor_, Start());
-  EXPECT_TRUE(service_.Start(&error));
+  EXPECT_TRUE(StartService(&error));
   EXPECT_EQ(nullptr, error);
 }