Add UpstartClient

chromeos/dbus/upstart_client.cc

+// Copyright (c) 2016 The Chromium 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 "chromeos/dbus/upstart_client.h"
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+
+namespace chromeos {
+
+namespace {
+
+const char kUpstartServiceName[] = "com.ubuntu.Upstart";
+const char kUpstartJobInterface[] = "com.ubuntu.Upstart0_6.Job";
+const char kUpstartStartMethod[] = "Start";
+
+const char kUpstartAuthPolicyPath[] = "/com/ubuntu/Upstart/jobs/authpolicyd";
+
+class UpstartClientImpl : public UpstartClient {
+ public:
+  UpstartClientImpl() : weak_ptr_factory_(this) {}
+
+  ~UpstartClientImpl() override {}
+
+  // UpstartClient override.
+  void StartAuthPolicyService() override {
+    dbus::ObjectPath objectPath(kUpstartAuthPolicyPath);
+    dbus::ObjectProxy* proxy =

[hashimoto, 2016/11/08 20:23:58]

Please make proxy_ a member of this class to be consistent with other  D-Bus
client classes.

[Roman Sorokin (ftl), 2016/11/09 14:03:30]

Hmm, what is the right way to do that? Since this proxy is specific to
AuthPolicydPath. Or is it OK to save that as authpolicy_proxy_ member?

[hashimoto, 2016/11/11 03:46:57]

Ah, it seems this method is tricky.
Where can I look at the documentation of this API? (or this is an undocumented
API which shouldn't be used?)
I tried to access http://upstart.ubuntu.com/wiki/DBusInterface, but it gave me
503.

[hashimoto, 2016/11/11 05:30:20]

FYI, another possible approach is to let the session manager emit an upstart
event, and write an upstart conf file to handle the event and start the auth
policy service.
https://chromium.googlesource.com/chromiumos/platform2/+/master/login_manager...

[Roman Sorokin (ftl), 2016/11/14 13:54:19]

The problem is we need to start it from Chrome anyway (at least for the
enrollment case - which would be triggered by custom shortcut)

[Roman Sorokin (ftl), 2016/11/14 13:54:19]

I looked here: http://upstart.ubuntu.com/cookbook/#to-start-a-job-via-d-bus

[hashimoto, 2016/11/14 22:09:40]

Thank you for giving me a pointer!

+        bus_->GetObjectProxy(kUpstartServiceName, objectPath);
+    dbus::MethodCall method_call(kUpstartJobInterface, kUpstartStartMethod);
+    dbus::MessageWriter writer(&method_call);
+    dbus::MessageWriter sub_writer(nullptr);
+    writer.OpenArray("s", &sub_writer);
+    writer.CloseContainer(&sub_writer);

[hashimoto, 2016/11/08 20:23:59]

suggestion: How about writer.AppendArrayOfStrings(std::vector<std::string>()); ?

[Roman Sorokin (ftl), 2016/11/09 14:03:30]

Done.

+    writer.AppendBool(false);  // No waiting for response.

[hashimoto, 2016/11/08 20:23:58]

IIUC this parameter changes the timing when the response is sent (i.e. when
HandleUpstartCallback() is called), so "true" here would let us see the actual
Start() result in the callback.

Why "false" is preferred?

[Roman Sorokin (ftl), 2016/11/09 14:03:30]

I checked both options, but response is null in both cases (if startup failed
for some reason). So not sure what is the advantage of waiting it.

[hashimoto, 2016/11/11 03:46:58]

IIUC, without waiting, it results in ignoring errors which occurred after the
response is sent back.
So I thought using "true" here should allow us to be aware of more errors, thus
it makes easy to debug by reading log files.

[Roman Sorokin (ftl), 2016/11/14 13:53:15]

Done.

+    proxy->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                      base::Bind(&UpstartClientImpl::HandleUpstartCallback,
+                                 weak_ptr_factory_.GetWeakPtr()));
+  }
+
+ protected:
+  void Init(dbus::Bus* bus) override { bus_ = bus; }
+
+ private:
+  void HandleUpstartCallback(dbus::Response* response) {

[hashimoto, 2016/11/08 20:23:58]

HandleUpstartCallback sounds ambiguous as basically everything in this class
should be related to Upstart :)

Maybe something like "HanldeStartResponse" is better?

[Roman Sorokin (ftl), 2016/11/09 14:03:30]

Done.

+    if (!response) {
+      LOG(ERROR) << "Failed to signal Upstart, response is null";
+      return;

[hashimoto, 2016/11/08 20:23:58]

nit: This line is redundant.

Probably you should just use LOG_IF?

[Roman Sorokin (ftl), 2016/11/09 14:03:30]

Done.

+    }
+  }
+
+  dbus::Bus* bus_ = nullptr;
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<UpstartClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(UpstartClientImpl);
+};
+
+}  // namespace
+
+UpstartClient::UpstartClient() {}
+
+UpstartClient::~UpstartClient() {}
+
+// static
+UpstartClient* UpstartClient::Create() {
+  return new UpstartClientImpl();
+}
+
+}  // namespace chromeos