arc: enable Android tracing in verified-boot mode

chromeos/dbus/arc_trace_agent.cc

+// Copyright 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/arc_trace_agent.h"
+
+#include <string>
+#include <utility>
+
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/pipe_reader.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "dbus/object_proxy.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+constexpr char kArcTracingAgentName[] = "arc";
+constexpr char kArcTraceLabel[] = "ArcTraceEvents";
+
+// A wrapper for controlling system tracing on Chrome OS.
+class ArcTraceAgentImpl : public ArcTraceAgent {
+ public:
+  ArcTraceAgentImpl() : debugdaemon_proxy_(nullptr), weak_ptr_factory_(this) {}
+
+  ~ArcTraceAgentImpl() override = default;
+
+  // base::trace_event::TracingAgent overrides:
+  std::string GetTracingAgentName() override { return kArcTracingAgentName; }
+  std::string GetTraceEventLabel() override { return kArcTraceLabel; }
+  void StartAgentTracing(const base::trace_event::TraceConfig& trace_config,
+                         const StartAgentTracingCallback& callback) override {
+    // delegate_ may be nullptr if ARC is not enabled on the system. In such
+    // case, simply do nothing.
+    if (delegate_) {

[Luis Héctor Chávez, 2016/12/28 17:21:58]

nit:

if (!delegate_)
  return;

// rest of method.

[shunhsingou, 2016/12/29 09:40:29]

Done, and also pass false to the callback in this situation.

+      delegate_->StartTracing(trace_config);
+
+      dbus::MethodCall method_call(debugd::kDebugdInterface, "ArcTraceStart");
+      debugdaemon_proxy_->CallMethod(
+          &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+          base::Bind(&ArcTraceAgentImpl::OnStartMethod,
+                     weak_ptr_factory_.GetWeakPtr()));
+      start_callback_ = callback;

[Luis Héctor Chávez, 2016/12/28 17:21:58]

nit: You can avoid introducing |start_callback_| by binding the callback:

namespace {

void OnStarted(const StartAgentTracingCallback& callback,
               dbus::Response* response) {
  if (!response) {
    LOG(ERROR) << "Failed to request start";
  }
  callback.Run(kArcTracingAgentName, response != nullptr);
}

}  // namespace

...

base::Bind(&OnStarted, callback);

[shunhsingou, 2016/12/29 09:40:29]

Done.

+    }
+  }
+  void StopAgentTracing(const StopAgentTracingCallback& callback) override {

[Luis Héctor Chávez, 2016/12/28 17:21:58]

nit: newline before.

[shunhsingou, 2016/12/29 09:40:29]

Done.

+    DCHECK(stop_agent_tracing_task_runner_);
+    if (!stop_callback_.is_null()) {
+      LOG(ERROR) << "Busy doing StopAgentTracing";

[Luis Héctor Chávez, 2016/12/28 17:21:58]

nit: maybe word this as "Previous StopAgentTracing() invocation is still
pending."

[shunhsingou, 2016/12/29 09:40:29]

Done.

+      return;
+    }
+
+    if (!delegate_) {
+      std::string no_data;
+      callback.Run(GetTracingAgentName(), GetTraceEventLabel(),
+                   base::RefCountedString::TakeString(&no_data));
+      return;
+    }
+
+    delegate_->StopTracing();
+
+    pipe_reader_ = base::MakeUnique<PipeReaderForString>(
+        stop_agent_tracing_task_runner_,
+        base::Bind(&ArcTraceAgentImpl::OnIOComplete,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    base::ScopedFD pipe_write_end = pipe_reader_->StartIO();
+    DCHECK(pipe_write_end.is_valid());
+    // Issue the dbus request to stop system tracing
+    dbus::MethodCall method_call(debugd::kDebugdInterface, "ArcTraceStop");
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendFileDescriptor(pipe_write_end.get());
+
+    stop_callback_ = callback;
+
+    debugdaemon_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&ArcTraceAgentImpl::OnStopAgentTracing,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void SetDelegate(Delegate* delegate) { delegate_ = delegate; }

[Luis Héctor Chávez, 2016/12/28 17:21:58]

This should be marked as 'override'. Also add

// ArcTraceAgent overrides:

[shunhsingou, 2016/12/29 09:40:29]

Done.

+
+  void SetStopAgentTracingTaskRunner(
+      scoped_refptr<base::TaskRunner> task_runner) override {
+    stop_agent_tracing_task_runner_ = task_runner;
+  }
+
+ protected:
+  // DBusClient overrides:
+  void Init(dbus::Bus* bus) override {
+    debugdaemon_proxy_ =
+        bus->GetObjectProxy(debugd::kDebugdServiceName,
+                            dbus::ObjectPath(debugd::kDebugdServicePath));
+  }
+
+ private:
+  void OnStartMethod(dbus::Response* response) {
+    if (!response) {
+      LOG(ERROR) << "Failed to request start";
+    }
+    base::ResetAndReturn(&start_callback_)
+        .Run(GetTracingAgentName(), response != nullptr);
+  }

[Luis Héctor Chávez, 2016/12/28 17:21:58]

nit: newline between method definitions. Only skip newlines when you are listing
override declarations.

[shunhsingou, 2016/12/29 09:40:29]

Done.

+  // Called when pipe I/O completes; pass data on and delete the instance.
+  void OnIOComplete() {
+    std::string pipe_data;
+    pipe_reader_->GetData(&pipe_data);

[Luis Héctor Chávez, 2016/12/28 17:21:58]

Weren't you calling |pipe_reader_.reset()| before?

[shunhsingou, 2016/12/29 09:40:29]

Done. Add it back.

+    base::ResetAndReturn(&stop_callback_)
+        .Run(GetTracingAgentName(), GetTraceEventLabel(),
+             base::RefCountedString::TakeString(&pipe_data));
+  }
+  void OnStopAgentTracing(dbus::Response* response) {
+    if (!response) {
+      LOG(ERROR) << "Failed to request arc stop";
+      // If debugd crashes or completes I/O before this message is processed
+      // then pipe_reader_ can be nullptr, see OnIOComplete().
+      if (pipe_reader_.get())
+        pipe_reader_->OnDataReady(-1);  // terminate data stream
+    }
+    // NB: requester is signaled when I/O completes
+  }
+
+  Delegate* delegate_;

[Luis Héctor Chávez, 2016/12/28 17:21:58]

Who owns this?

[shunhsingou, 2016/12/29 09:40:29]

Added comment here.

+  dbus::ObjectProxy* debugdaemon_proxy_;  // Owned by dbus::Bus.
+  std::unique_ptr<PipeReaderForString> pipe_reader_;
+  scoped_refptr<base::TaskRunner> stop_agent_tracing_task_runner_;
+  StartAgentTracingCallback start_callback_;
+  StopAgentTracingCallback stop_callback_;
+  base::WeakPtrFactory<ArcTraceAgentImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ArcTraceAgentImpl);
+};
+
+}  // namespace
+
+// static
+ArcTraceAgent* ArcTraceAgent::Create() {
+  return new ArcTraceAgentImpl();
+}
+
+}  // namespace chromeos