Skip to content

Commit

Permalink
No dbus::FileDescriptor in DebugDaemonClient
Browse files Browse the repository at this point in the history 
This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841

Review-Url: https://codereview.chromium.org/2081153002
Cr-Commit-Position: refs/heads/master@{#414093}
  • Loading branch information
hashimoto authored and Commit bot committed Aug 24, 2016
1 parent 1235009 commit 8982257
Show file tree
Hide file tree
Showing 13 changed files with 123 additions and 201 deletions.
36 changes: 19 additions & 17 deletions chrome/browser/chromeos/system_logs/debug_log_writer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ void WriteDebugLogToFileCompleted(

// Stores into |file_path| debug logs in the .tgz format. Calls
// |callback| upon completion.
void WriteDebugLogToFile(base::File* file,
void WriteDebugLogToFile(std::unique_ptr<base::File> file,
const std::string& sequence_token_name,
const base::FilePath& file_path,
bool should_compress,
Expand All @@ -74,12 +74,15 @@ void WriteDebugLogToFile(base::File* file,
<< "error: " << file->error_details();
return;
}
scoped_refptr<base::TaskRunner> task_runner =
GetSequencedTaskRunner(sequence_token_name);
chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->DumpDebugLogs(
should_compress, std::move(*file), task_runner,
should_compress, file->GetPlatformFile(),
base::Bind(&WriteDebugLogToFileCompleted, file_path, sequence_token_name,
callback));

// Close the file on an IO-allowed thread.
GetSequencedTaskRunner(sequence_token_name)
->PostTask(FROM_HERE,
base::Bind(&base::DeletePointer<base::File>, file.release()));
}

// Runs command with its parameters as defined in |argv|.
Expand Down Expand Up @@ -200,9 +203,9 @@ void OnSystemLogsAdded(const DebugLogWriter::StoreLogsCallback& callback,
callback));
}

void IntializeLogFile(base::File* file,
const base::FilePath& file_path,
uint32_t flags) {
void InitializeLogFile(base::File* file,
const base::FilePath& file_path,
uint32_t flags) {
base::FilePath dir = file_path.DirName();
if (!base::DirectoryExists(dir)) {
if (!base::CreateDirectory(dir)) {
Expand All @@ -224,16 +227,15 @@ void StartLogRetrieval(const base::FilePath& file_name_template,
logging::GenerateTimestampedName(file_name_template, base::Time::Now());

int flags = base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
base::File* file = new base::File;
GetSequencedTaskRunner(sequence_token_name)->PostTaskAndReply(
FROM_HERE,
base::Bind(&IntializeLogFile, base::Unretained(file), file_path, flags),
base::Bind(&WriteDebugLogToFile,
base::Owned(file),
sequence_token_name,
file_path,
should_compress,
callback));
std::unique_ptr<base::File> file(new base::File);
base::File* file_ptr = file.get();
GetSequencedTaskRunner(sequence_token_name)
->PostTaskAndReply(
FROM_HERE, base::Bind(&InitializeLogFile, base::Unretained(file_ptr),
file_path, flags),
base::Bind(&WriteDebugLogToFile, base::Passed(&file),
sequence_token_name, file_path, should_compress,
callback));
}

const char kDefaultSequenceName[] = "DebugLogWriter";
Expand Down
13 changes: 6 additions & 7 deletions chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,13 @@ class TestDebugDaemonClient : public chromeos::FakeDebugDaemonClient {
~TestDebugDaemonClient() override {}

void DumpDebugLogs(bool is_compressed,
base::File file,
scoped_refptr<base::TaskRunner> task_runner,
int file_descriptor,
const GetDebugLogsCallback& callback) override {
base::File* file_param = new base::File(std::move(file));
task_runner->PostTaskAndReply(
FROM_HERE,
base::Bind(
&GenerateTestLogDumpFile, test_file_, base::Owned(file_param)),
// dup() is needed as the file descriptor will be closed on the client side.
base::File* file_param = new base::File(dup(file_descriptor));
content::BrowserThread::PostBlockingPoolTaskAndReply(
FROM_HERE, base::Bind(&GenerateTestLogDumpFile, test_file_,
base::Owned(file_param)),
base::Bind(callback, true));
}

Expand Down
37 changes: 4 additions & 33 deletions chrome/browser/metrics/perf/perf_output.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,26 +4,10 @@

#include "chrome/browser/metrics/perf/perf_output.h"

#include <memory>

#include "base/bind.h"
#include "base/location.h"
#include "base/task_runner_util.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/dbus/debug_daemon_client.h"

namespace {

// Create a dbus::FileDescriptor from a base::File.
dbus::ScopedFileDescriptor CreateFileDescriptor(base::File pipe_write_end) {
dbus::ScopedFileDescriptor file_descriptor(new dbus::FileDescriptor);
file_descriptor->PutValue(pipe_write_end.TakePlatformFile());
file_descriptor->CheckValidity();
return file_descriptor;
}

} // namespace

PerfOutputCall::PerfOutputCall(
scoped_refptr<base::TaskRunner> blocking_task_runner,
base::TimeDelta duration,
Expand All @@ -40,29 +24,16 @@ PerfOutputCall::PerfOutputCall(
blocking_task_runner_,
base::Bind(&PerfOutputCall::OnIOComplete, weak_factory_.GetWeakPtr())));

base::File pipe_write_end = perf_data_pipe_reader_->StartIO();
base::PostTaskAndReplyWithResult(
blocking_task_runner_.get(), FROM_HERE,
base::Bind(&CreateFileDescriptor, base::Passed(&pipe_write_end)),
base::Bind(&PerfOutputCall::OnFileDescriptorCreated,
weak_factory_.GetWeakPtr()));
}

PerfOutputCall::~PerfOutputCall() {}

void PerfOutputCall::OnFileDescriptorCreated(
dbus::ScopedFileDescriptor file_descriptor) {
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(file_descriptor);

base::ScopedFD pipe_write_end = perf_data_pipe_reader_->StartIO();
chromeos::DebugDaemonClient* client =
chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();

client->GetPerfOutput(duration_, perf_args_, std::move(file_descriptor),
client->GetPerfOutput(duration_, perf_args_, pipe_write_end.get(),
base::Bind(&PerfOutputCall::OnGetPerfOutputError,
weak_factory_.GetWeakPtr()));
}

PerfOutputCall::~PerfOutputCall() {}

void PerfOutputCall::OnIOComplete() {
DCHECK(thread_checker_.CalledOnValidThread());

Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/metrics/perf/perf_output.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "base/threading/thread_checker.h"
#include "base/time/time.h"
#include "chromeos/dbus/pipe_reader.h"
#include "dbus/file_descriptor.h"

// Class for handling getting output from perf over DBus. Manages the
// asynchronous DBus call and retrieving data from quipper over a pipe.
Expand All @@ -36,7 +35,6 @@ class PerfOutputCall {

private:
// Internal callbacks.
void OnFileDescriptorCreated(dbus::ScopedFileDescriptor file_descriptor);
void OnIOComplete();
void OnGetPerfOutputError(const std::string& error_name,
const std::string& error_message);
Expand Down
155 changes: 40 additions & 115 deletions chromeos/dbus/debug_daemon_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,33 +39,12 @@ const char kCrOSTraceLabel[] = "systemTraceEvents";
// Because the cheets logs are very huge, we set the D-Bus timeout to 2 minutes.
const int kBigLogsDBusTimeoutMS = 120 * 1000;

// The type of the callback to be invoked once the pipe reader has been
// initialized and a D-Bus file descriptor has been created for it.
using OnPipeReaderInitializedCallback =
base::Callback<void(dbus::ScopedFileDescriptor)>;

// Used in DebugDaemonClient::EmptyStopAgentTracingCallback().
void EmptyStopAgentTracingCallbackBody(
const std::string& agent_name,
const std::string& events_label,
const scoped_refptr<base::RefCountedString>& unused_result) {}

// Creates a D-Bus file descriptor from a base::File.
dbus::ScopedFileDescriptor CreateFileDescriptorForPipeWriteEnd(
base::File pipe_write_end) {
if (!pipe_write_end.IsValid()) {
VLOG(1) << "Cannot create pipe reader";
// NB: continue anyway; toss the data
pipe_write_end.Initialize(base::FilePath(FILE_PATH_LITERAL("/dev/null")),
base::File::FLAG_OPEN | base::File::FLAG_WRITE);
// TODO(afakhry): If this fails AppendFileDescriptor will abort.
}
dbus::ScopedFileDescriptor file_descriptor(new dbus::FileDescriptor);
file_descriptor->PutValue(pipe_write_end.TakePlatformFile());
file_descriptor->CheckValidity();
return file_descriptor;
}

// A self-deleting object that wraps the pipe reader operations for reading the
// big feedback logs. It will delete itself once the pipe stream has been
// terminated. Once the data has been completely read from the pipe, it invokes
Expand All @@ -80,14 +59,7 @@ class PipeReaderWrapper : public base::SupportsWeakPtr<PipeReaderWrapper> {
base::Bind(&PipeReaderWrapper::OnIOComplete, AsWeakPtr())),
callback_(callback) {}

void Initialize(const OnPipeReaderInitializedCallback& on_initialized) {
base::File pipe_write_end = pipe_reader_.StartIO();
base::PostTaskAndReplyWithResult(
task_runner_.get(), FROM_HERE,
base::Bind(&CreateFileDescriptorForPipeWriteEnd,
base::Passed(&pipe_write_end)),
on_initialized);
}
base::ScopedFD Initialize() { return pipe_reader_.StartIO(); }

void OnIOComplete() {
std::string pipe_data;
Expand Down Expand Up @@ -137,22 +109,18 @@ class DebugDaemonClientImpl : public DebugDaemonClient {

// DebugDaemonClient override.
void DumpDebugLogs(bool is_compressed,
base::File file,
scoped_refptr<base::TaskRunner> task_runner,
int file_descriptor,
const GetDebugLogsCallback& callback) override {
dbus::FileDescriptor* file_descriptor = new dbus::FileDescriptor;
file_descriptor->PutValue(file.TakePlatformFile());
// Punt descriptor validity check to a worker thread; on return we'll
// issue the D-Bus request to stop tracing and collect results.
task_runner->PostTaskAndReply(
FROM_HERE,
base::Bind(&dbus::FileDescriptor::CheckValidity,
base::Unretained(file_descriptor)),
base::Bind(&DebugDaemonClientImpl::OnCheckValidityGetDebugLogs,
weak_ptr_factory_.GetWeakPtr(),
is_compressed,
base::Owned(file_descriptor),
callback));
// Issue the dbus request to get debug logs.
dbus::MethodCall method_call(debugd::kDebugdInterface,
debugd::kDumpDebugLogs);
dbus::MessageWriter writer(&method_call);
writer.AppendBool(is_compressed);
writer.AppendFileDescriptor(file_descriptor);
debugdaemon_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&DebugDaemonClientImpl::OnGetDebugLogs,
weak_ptr_factory_.GetWeakPtr(), callback));
}

void SetDebugMode(const std::string& subsystem,
Expand Down Expand Up @@ -242,15 +210,15 @@ class DebugDaemonClientImpl : public DebugDaemonClient {

void GetPerfOutput(base::TimeDelta duration,
const std::vector<std::string>& perf_args,
dbus::ScopedFileDescriptor file_descriptor,
int file_descriptor,
const DBusMethodErrorCallback& error_callback) override {
DCHECK(file_descriptor);
dbus::MethodCall method_call(debugd::kDebugdInterface,
debugd::kGetPerfOutputFd);
dbus::MessageWriter writer(&method_call);
writer.AppendUint32(duration.InSeconds());
writer.AppendArrayOfStrings(perf_args);
writer.AppendFileDescriptor(*file_descriptor);
writer.AppendFileDescriptor(file_descriptor);

debugdaemon_proxy_->CallMethodWithErrorCallback(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
Expand All @@ -277,8 +245,17 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
// long time to be processed and a new request should never be ignored nor
// cancels the on-going one.
PipeReaderWrapper* pipe_reader = new PipeReaderWrapper(callback);
pipe_reader->Initialize(
base::Bind(&DebugDaemonClientImpl::OnBigLogsPipeReaderReady,
base::ScopedFD pipe_write_end = pipe_reader->Initialize();

dbus::MethodCall method_call(debugd::kDebugdInterface,
debugd::kGetBigFeedbackLogs);
dbus::MessageWriter writer(&method_call);
writer.AppendFileDescriptor(pipe_write_end.get());

DVLOG(1) << "Requesting big feedback logs";
debugdaemon_proxy_->CallMethod(
&method_call, kBigLogsDBusTimeoutMS,
base::Bind(&DebugDaemonClientImpl::OnBigFeedbackLogsResponse,
weak_ptr_factory_.GetWeakPtr(), pipe_reader->AsWeakPtr()));
}

Expand Down Expand Up @@ -341,16 +318,21 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
base::Bind(&DebugDaemonClientImpl::OnIOComplete,
weak_ptr_factory_.GetWeakPtr())));

base::File pipe_write_end = pipe_reader_->StartIO();
// Create dbus::FileDescriptor on the worker thread; on return we'll
// issue the D-Bus request to stop tracing and collect results.
base::PostTaskAndReplyWithResult(
stop_agent_tracing_task_runner_.get(), FROM_HERE,
base::Bind(&CreateFileDescriptorForPipeWriteEnd,
base::Passed(&pipe_write_end)),
base::Bind(
&DebugDaemonClientImpl::OnCreateFileDescriptorRequestStopSystem,
weak_ptr_factory_.GetWeakPtr(), callback));
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,
debugd::kSystraceStop);
dbus::MessageWriter writer(&method_call);
writer.AppendFileDescriptor(pipe_write_end.get());

callback_ = callback;

DVLOG(1) << "Requesting a systrace stop";
debugdaemon_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&DebugDaemonClientImpl::OnStopAgentTracing,
weak_ptr_factory_.GetWeakPtr()));
}

void SetStopAgentTracingTaskRunner(
Expand Down Expand Up @@ -468,25 +450,6 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
}

private:
// Called when a CheckValidity response is received.
void OnCheckValidityGetDebugLogs(bool is_compressed,
dbus::FileDescriptor* file_descriptor,
const GetDebugLogsCallback& callback) {
// Issue the dbus request to get debug logs.
dbus::MethodCall method_call(debugd::kDebugdInterface,
debugd::kDumpDebugLogs);
dbus::MessageWriter writer(&method_call);
writer.AppendBool(is_compressed);
writer.AppendFileDescriptor(*file_descriptor);

debugdaemon_proxy_->CallMethod(
&method_call,
dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&DebugDaemonClientImpl::OnGetDebugLogs,
weak_ptr_factory_.GetWeakPtr(),
callback));
}

// Called when a response for GetDebugLogs() is received.
void OnGetDebugLogs(const GetDebugLogsCallback& callback,
dbus::Response* response) {
Expand Down Expand Up @@ -589,22 +552,6 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
return OnGetAllLogs(callback, response);
}

void OnBigLogsPipeReaderReady(base::WeakPtr<PipeReaderWrapper> pipe_reader,
dbus::ScopedFileDescriptor file_descriptor) {
DCHECK(file_descriptor);

dbus::MethodCall method_call(debugd::kDebugdInterface,
debugd::kGetBigFeedbackLogs);
dbus::MessageWriter writer(&method_call);
writer.AppendFileDescriptor(*file_descriptor);

DVLOG(1) << "Requesting big feedback logs";
debugdaemon_proxy_->CallMethod(
&method_call, kBigLogsDBusTimeoutMS,
base::Bind(&DebugDaemonClientImpl::OnBigFeedbackLogsResponse,
weak_ptr_factory_.GetWeakPtr(), pipe_reader));
}

void OnBigFeedbackLogsResponse(base::WeakPtr<PipeReaderWrapper> pipe_reader,
dbus::Response* response) {
if (!response && pipe_reader.get()) {
Expand Down Expand Up @@ -669,28 +616,6 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
callback.Run(response != NULL);
}

// Called when a CheckValidity response is received.
void OnCreateFileDescriptorRequestStopSystem(
const StopAgentTracingCallback& callback,
dbus::ScopedFileDescriptor file_descriptor) {
DCHECK(file_descriptor);

// Issue the dbus request to stop system tracing
dbus::MethodCall method_call(
debugd::kDebugdInterface,
debugd::kSystraceStop);
dbus::MessageWriter writer(&method_call);
writer.AppendFileDescriptor(*file_descriptor);

callback_ = callback;

DVLOG(1) << "Requesting a systrace stop";
debugdaemon_proxy_->CallMethod(
&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
base::Bind(&DebugDaemonClientImpl::OnStopAgentTracing,
weak_ptr_factory_.GetWeakPtr()));
}

// Called when a response for StopAgentTracing() is received.
void OnStopAgentTracing(dbus::Response* response) {
if (!response) {
Expand Down
Loading

0 comments on commit 8982257

Please sign in to comment.