Refactor UpstartClient and allow starting arbitrary jobs

Factor out common code in UpstartClient, and allow UpstartClient to
start arbitrary jobs instead of a fixed set of them.

BUG=b:115779632
TEST=./out/Debug/unit_tests
TEST=./out/Debug/component_unittests
TEST=Ensure dummy job can be started with UpstartClient

Change-Id: I362ad288b93615753cd778e823173d24a9ac16df
Reviewed-on: https://chromium-review.googlesource.com/c/1307238Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Commit-Queue: Christopher Morin <cmtm@google.com>
Cr-Commit-Position: refs/heads/master@{#604095}

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -72,6 +72,7 @@
 #include "chromeos/cryptohome/async_method_caller.h"
 #include "chromeos/cryptohome/cryptohome_util.h"
 #include "chromeos/dbus/cryptohome/rpc.pb.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/upstart_client.h"
 #include "chromeos/login/login_state.h"
@@ -121,6 +122,8 @@ const char kDeviceLocalAccountPendingDataRemoval[] =
 
 constexpr char kGoogleDotCom[] = "@google.com";
 
+constexpr char kBluetoothLoggingUpstartJob[] = "bluetoothlog";
+
 bool FakeOwnership() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kStubCrosSettings);
@@ -204,9 +207,8 @@ void MaybeStartBluetoothLogging(const AccountId& account_id) {
   const std::string board_name = board[0];
   if (board_name != "eve" && board_name != "nocturne")
     return;
-  chromeos::DBusThreadManager::Get()
-      ->GetUpstartClient()
-      ->StartBluetoothLogging();
+  chromeos::DBusThreadManager::Get()->GetUpstartClient()->StartJob(
+      kBluetoothLoggingUpstartJob, {}, EmptyVoidDBusMethodCallback());
 }
 
 }  // namespace

chromeos/dbus/fake_upstart_client.cc

@@ -17,6 +17,13 @@ FakeUpstartClient::~FakeUpstartClient() = default;
 
 void FakeUpstartClient::Init(dbus::Bus* bus) {}
 
+void FakeUpstartClient::StartJob(const std::string& job,
+                                 const std::vector<std::string>& upstart_env,
+                                 VoidDBusMethodCallback callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), true));
+}
+
 void FakeUpstartClient::StartAuthPolicyService() {
   static_cast<FakeAuthPolicyClient*>(
       DBusThreadManager::Get()->GetAuthPolicyClient())
@@ -73,8 +80,4 @@ void FakeUpstartClient::StopMediaAnalytics(VoidDBusMethodCallback callback) {
       FROM_HERE, base::BindOnce(std::move(callback), true));
 }
 
-void FakeUpstartClient::StartBluetoothLogging() {
-  NOTIMPLEMENTED();
-}
-
 }  // namespace chromeos

chromeos/dbus/fake_upstart_client.h

@@ -19,6 +19,9 @@ class CHROMEOS_EXPORT FakeUpstartClient : public UpstartClient {
   // DBusClient overrides.
   void Init(dbus::Bus* bus) override;
 
+  void StartJob(const std::string& job,
+                const std::vector<std::string>& upstart_env,
+                VoidDBusMethodCallback callback) override;
   // UpstartClient overrides.
   void StartAuthPolicyService() override;
   void RestartAuthPolicyService() override;
@@ -27,7 +30,6 @@ class CHROMEOS_EXPORT FakeUpstartClient : public UpstartClient {
   void RestartMediaAnalytics(VoidDBusMethodCallback callback) override;
   void StopMediaAnalytics() override;
   void StopMediaAnalytics(VoidDBusMethodCallback callback) override;
-  void StartBluetoothLogging() override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FakeUpstartClient);

chromeos/dbus/upstart_client.cc

@@ -14,17 +14,15 @@ namespace chromeos {
 
 namespace {
 
-const char kUpstartServiceName[] = "com.ubuntu.Upstart";
-const char kUpstartJobInterface[] = "com.ubuntu.Upstart0_6.Job";
-const char kUpstartStartMethod[] = "Start";
-const char kUpstartRestartMethod[] = "Restart";
-const char kUpstartStopMethod[] = "Stop";
-
-const char kUpstartAuthPolicyPath[] = "/com/ubuntu/Upstart/jobs/authpolicyd";
-const char kUpstartMediaAnalyticsPath[] =
-    "/com/ubuntu/Upstart/jobs/rtanalytics";
-const char kUpstartBluetoothLoggingPath[] =
-    "/com/ubuntu/Upstart/jobs/bluetoothlog";
+constexpr char kUpstartServiceName[] = "com.ubuntu.Upstart";
+constexpr char kUpstartJobInterface[] = "com.ubuntu.Upstart0_6.Job";
+constexpr char kStartMethod[] = "Start";
+constexpr char kRestartMethod[] = "Restart";
+constexpr char kStopMethod[] = "Stop";
+
+constexpr char kUpstartJobsPath[] = "/com/ubuntu/Upstart/jobs/";
+constexpr char kAuthPolicyJob[] = "authpolicyd";
+constexpr char kMediaAnalyticsJob[] = "rtanalytics";
 
 class UpstartClientImpl : public UpstartClient {
  public:
@@ -32,118 +30,66 @@ class UpstartClientImpl : public UpstartClient {
 
   ~UpstartClientImpl() override = default;
 
+  void StartJob(const std::string& job,
+                const std::vector<std::string>& upstart_env,
+                VoidDBusMethodCallback callback) override {
+    CallJobMethod(job, kStartMethod, upstart_env, std::move(callback));
+  }
+
   // UpstartClient override.
   void StartAuthPolicyService() override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStartMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    auth_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::HandleAuthResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+    StartJob(kAuthPolicyJob, {}, EmptyVoidDBusMethodCallback());
   }
 
   void RestartAuthPolicyService() override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartRestartMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    auth_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::HandleAuthResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+    CallJobMethod(kAuthPolicyJob, kRestartMethod, {},
+                  EmptyVoidDBusMethodCallback());
   }
 
   void StartMediaAnalytics(const std::vector<std::string>& upstart_env,
                            VoidDBusMethodCallback callback) override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStartMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(upstart_env);
-    writer.AppendBool(true /* wait for response */);
-    ma_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::OnVoidMethod,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+    StartJob(kMediaAnalyticsJob, upstart_env, std::move(callback));
   }
 
   void RestartMediaAnalytics(VoidDBusMethodCallback callback) override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartRestartMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    ma_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::OnVoidMethod,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+    CallJobMethod(kMediaAnalyticsJob, kRestartMethod, {}, std::move(callback));
   }
 
   void StopMediaAnalytics() override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStopMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    ma_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::HandleStopMediaAnalyticsResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+    CallJobMethod(kMediaAnalyticsJob, kStopMethod, {},
+                  EmptyVoidDBusMethodCallback());
   }
 
   void StopMediaAnalytics(VoidDBusMethodCallback callback) override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStopMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    ma_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::OnVoidMethod,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
-  }
-
-  void StartBluetoothLogging() override {
-    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStartMethod);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendArrayOfStrings(std::vector<std::string>());
-    writer.AppendBool(true /* wait for response */);
-    bluetooth_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&UpstartClientImpl::HandleBluetoothLoggingResponse,
-                       weak_ptr_factory_.GetWeakPtr()));
+    CallJobMethod(kMediaAnalyticsJob, kStopMethod, {}, std::move(callback));
   }
-
  protected:
   void Init(dbus::Bus* bus) override {
     bus_ = bus;
-    auth_proxy_ = bus_->GetObjectProxy(
-        kUpstartServiceName, dbus::ObjectPath(kUpstartAuthPolicyPath));
-    ma_proxy_ = bus_->GetObjectProxy(
-        kUpstartServiceName, dbus::ObjectPath(kUpstartMediaAnalyticsPath));
-    bluetooth_proxy_ = bus_->GetObjectProxy(
-        kUpstartServiceName, dbus::ObjectPath(kUpstartBluetoothLoggingPath));
   }
 
  private:
-  void HandleAuthResponse(dbus::Response* response) {
-    LOG_IF(ERROR, !response) << "Failed to signal Upstart, response is null";
+  void CallJobMethod(const std::string& job,
+                     const std::string& method,
+                     const std::vector<std::string>& upstart_env,
+                     VoidDBusMethodCallback callback) {
+    dbus::ObjectProxy* job_proxy = bus_->GetObjectProxy(
+        kUpstartServiceName, dbus::ObjectPath(kUpstartJobsPath + job));
+    dbus::MethodCall method_call(kUpstartJobInterface, method);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendArrayOfStrings(upstart_env);
+    writer.AppendBool(true /* wait for response */);
+    job_proxy->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&UpstartClientImpl::OnVoidMethod,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
   void OnVoidMethod(VoidDBusMethodCallback callback, dbus::Response* response) {
     std::move(callback).Run(response);
   }
 
-  void HandleStopMediaAnalyticsResponse(dbus::Response* response) {
-    LOG_IF(ERROR, !response) << "Failed to signal Upstart, response is null";
-  }
-
-  void HandleBluetoothLoggingResponse(dbus::Response* response) {
-    LOG_IF(ERROR, !response)
-        << "Failed to start bluetooth logging service, response is null";
-  }
-
   dbus::Bus* bus_ = nullptr;
-  dbus::ObjectProxy* auth_proxy_ = nullptr;
-  dbus::ObjectProxy* ma_proxy_ = nullptr;
-  dbus::ObjectProxy* bluetooth_proxy_ = nullptr;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.

chromeos/dbus/upstart_client.h

@@ -27,6 +27,14 @@ class CHROMEOS_EXPORT UpstartClient : public DBusClient {
   // For normal usage, access the singleton via DBusThreadManager::Get().
   static UpstartClient* Create();
 
+  // Starts an Upstart job.
+  // |job|: Name of Upstart job.
+  // |upstart_env|: List of upstart environment variables to be passed to the
+  // upstart service.
+  virtual void StartJob(const std::string& job,
+                        const std::vector<std::string>& upstart_env,
+                        VoidDBusMethodCallback callback) = 0;
+
   // Starts authpolicyd.
   virtual void StartAuthPolicyService() = 0;
 
@@ -47,10 +55,6 @@ class CHROMEOS_EXPORT UpstartClient : public DBusClient {
 
   // Provides an interface for stopping the media analytics process.
   virtual void StopMediaAnalytics(VoidDBusMethodCallback callback) = 0;
-
-  // Starts the bluetooth logging service.
-  virtual void StartBluetoothLogging() = 0;
-
  protected:
   // Create() should be used instead.
   UpstartClient();