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 <utility>
+
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_path.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+namespace {
+
+constexpr char kArcTracingAgentName[] = "arc";
+constexpr char kArcTraceLabel[] = "ArcTraceEvents";
+
+}  // namespace
+
+std::string ArcTraceAgent::GetTracingAgentName() {
+  return kArcTracingAgentName;
+}
+
+std::string ArcTraceAgent::GetTraceEventLabel() {
+  return kArcTraceLabel;
+}
+
+void ArcTraceAgent::SetArcTraceBridge(ArcTraceAgent::ArcTraceBridge* bridge) {
+  arc_trace_bridge_ = bridge;
+}
+
+void ArcTraceAgent::StartAgentTracing(
+    const base::trace_event::TraceConfig& trace_config,
+    const StartAgentTracingCallback& callback) {
+
+  // arc_trace_brigde_ may be null if ARC is not enabled on the system. In such

[Luis Héctor Chávez, 2016/12/27 19:15:59]

s/arc_trace_brigde_/arc_trace_bridge_/

[shunhsingou, 2016/12/28 09:15:16]

Done.

+  // case, simply do nothing.
+  if (arc_trace_bridge_) {
+    arc_trace_bridge_->StartTracing(trace_config);
+
+    dbus::MethodCall method_call(
+        debugd::kDebugdInterface,
+        "ArcTraceStart");
+    debugdaemon_proxy_->CallMethod(
+        &method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&ArcTraceAgent::OnStartMethod,

[Luis Héctor Chávez, 2016/12/27 19:15:59]

Shouldn't you run |callback| in OnStartMethod instead of unconditionally?

[shunhsingou, 2016/12/28 09:15:16]

Done.

+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(

[Luis Héctor Chávez, 2016/12/27 19:15:59]

Why can't you just do

callback.Run(GetTracingAgentName(), true /* success */);

? if there is a reason, please add a comment about it. Also, why are you
returning success unconditionally?

[shunhsingou, 2016/12/28 09:15:16]

I forget to change. Move it to OnStartMethod.

+      FROM_HERE,
+      base::Bind(callback, GetTracingAgentName(), true /* success */));
+}
+
+// Called when a response for a simple start is received.
+void ArcTraceAgent::OnStartMethod(dbus::Response* response) {

[Luis Héctor Chávez, 2016/12/27 19:16:00]

This can be a standalone function in the anonymous namespace.

[shunhsingou, 2016/12/28 09:15:16]

Moved callback call to here. So this function need access to
GetTracingAgentName().

+  if (!response) {
+    LOG(ERROR) << "Failed to request start";
+    return;
+  }
+}
+
+void ArcTraceAgent::StopAgentTracing(const StopAgentTracingCallback& callback) {
+  DCHECK(stop_agent_tracing_task_runner_);
+  if (pipe_reader_ != NULL) {

[Luis Héctor Chávez, 2016/12/27 19:15:59]

Maybe it's better to check if |callback_.is_null()| instead of checking for
pipe_reader_.

[shunhsingou, 2016/12/28 09:15:15]

Done.

+    LOG(ERROR) << "Busy doing StopAgentTracing";
+    return;
+  }
+
+  if (arc_trace_bridge_) {

[Luis Héctor Chávez, 2016/12/27 19:15:59]

if (!arc_trace_bridge_)
  return;

// rest of the function.

also, shouldn't you call the callback with failure if this happens?

[shunhsingou, 2016/12/28 09:15:16]

Done. Just return empty report here.

+    arc_trace_bridge_->StopTracing();
+
+    pipe_reader_.reset(

[Luis Héctor Chávez, 2016/12/27 19:15:59]

nit: pipe_reader_ = base::MakeUnique<PipeReaderForString>(... args ...)

[shunhsingou, 2016/12/28 09:15:16]

Done.

+        new PipeReaderForString(stop_agent_tracing_task_runner_,
+                                base::Bind(&ArcTraceAgent::OnIOComplete,
+                                           weak_ptr_factory_.GetWeakPtr())));
+
+    base::ScopedFD pipe_write_end = pipe_reader_->StartIO();

[Luis Héctor Chávez, 2016/12/27 19:15:59]

What thread is this supposed to be run on? Is I/O allowed in that thread?

[shunhsingou, 2016/12/28 09:15:15]

It's running on 'stop_agent_tracing_task_runner_', which is set in
tracing_controller_impl.cc as a blocking thread.

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

Can you add comments about which methods run on which threads?

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

Done.

+    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());
+
+    callback_ = callback;
+
+    debugdaemon_proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&ArcTraceAgent::OnStopAgentTracing,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
+// Called when pipe i/o completes; pass data on and delete the instance.

[Luis Héctor Chávez, 2016/12/27 19:16:00]

nit: be consistent and use I/O all across the board.

[shunhsingou, 2016/12/28 09:15:15]

Done.

+void ArcTraceAgent::OnIOComplete() {
+  std::string pipe_data;
+  pipe_reader_->GetData(&pipe_data);
+  callback_.Run(GetTracingAgentName(), GetTraceEventLabel(),

[Luis Héctor Chávez, 2016/12/27 19:15:59]

nit: base::ResetAndReturn(&callback_).Run(...);

[shunhsingou, 2016/12/28 09:15:16]

Done.

+                base::RefCountedString::TakeString(&pipe_data));
+  pipe_reader_.reset();

[Luis Héctor Chávez, 2016/12/27 19:16:00]

maybe reset the pipe_reader_ before calling the callback. 

[shunhsingou, 2016/12/28 09:15:16]

Done.

+}
+
+void ArcTraceAgent::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 NULL, see OnIOComplete().
+    if (pipe_reader_.get())
+      pipe_reader_->OnDataReady(-1);  // terminate data stream
+  }
+  // NB: requester is signaled when i/o completes
+}
+
+void ArcTraceAgent::SetStopAgentTracingTaskRunner(
+    scoped_refptr<base::TaskRunner> task_runner) {
+    stop_agent_tracing_task_runner_ = task_runner;

[Luis Héctor Chávez, 2016/12/27 19:15:59]

alignment. please run `git cl format` and `git cl lint` before uploading this
again.

[shunhsingou, 2016/12/28 09:15:16]

Done.

+}
+
+ArcTraceAgent* ArcTraceAgent::Create() {

[Luis Héctor Chávez, 2016/12/27 19:15:59]

nit: // static

[shunhsingou, 2016/12/28 09:15:16]

Done.

+  return new ArcTraceAgent();
+}
+
+void ArcTraceAgent::Init(dbus::Bus* bus) {
+  debugdaemon_proxy_ =
+      bus->GetObjectProxy(debugd::kDebugdServiceName,
+                          dbus::ObjectPath(debugd::kDebugdServicePath));
+}
+
+ArcTraceAgent::ArcTraceAgent()
+    : arc_trace_bridge_(NULL),

[Luis Héctor Chávez, 2016/12/27 19:15:59]

nit: s/NULL/nullptr/ all across the change.

Also, you can move those initializations to the header file.

[shunhsingou, 2016/12/28 09:15:16]

Done.

+      debugdaemon_proxy_(NULL),
+      weak_ptr_factory_(this) {}
+
+ArcTraceAgent::~ArcTraceAgent() {

[Luis Héctor Chávez, 2016/12/27 19:15:59]

nit: = default;

[shunhsingou, 2016/12/28 09:15:16]

Done.

+}
+
+}  // namespace chromeos