Add CommandBroker::OnTransaction to handle incoming transactions

chromeos/binder/command_broker.cc

 // Copyright 2015 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/binder/command_broker.h"
 
 #include <linux/android/binder.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "chromeos/binder/driver.h"
 #include "chromeos/binder/local_object.h"
 #include "chromeos/binder/transaction_data.h"
+#include "chromeos/binder/transaction_status.h"
 
 namespace binder {
 
 namespace {
 
 // Converts TransactionData to binder_transaction_data struct.
 binder_transaction_data ConvertTransactionDataToStruct(
     const TransactionData& data) {
   binder_transaction_data result = {};
   result.code = data.GetCode();
@@ skipping 14 matching lines @@
 
 }  // namespace
 
 CommandBroker::CommandBroker(Driver* driver)
     : command_stream_(driver, this), weak_ptr_factory_(this) {}
 
 CommandBroker::~CommandBroker() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
+bool CommandBroker::EnterLooper() {
+  command_stream_.AppendOutgoingCommand(BC_ENTER_LOOPER, nullptr, 0);
+  return command_stream_.Flush();
+}
+
+bool CommandBroker::ExitLooper() {
+  command_stream_.AppendOutgoingCommand(BC_EXIT_LOOPER, nullptr, 0);
+  return command_stream_.Flush();
+}
+
+bool CommandBroker::PollCommands() {
+  DCHECK(!command_stream_.CanProcessIncomingCommand());

[satorux1, 2016/01/13 04:33:20]

so it's not ok to call this function while CanProcessIncomingCommand() is true.
maybe we should document when to call this function in the function comment?

[hashimoto, 2016/01/13 05:46:46]

I put this DCHECK here to ensure that all previously-fetched commands were
processed in the last PollCommands() call, as leaving fetched commands
unprocessed can result in letting the transaction sender process wait forever.

However, I realized that this DCHECK is unnecessary as there is while
(command_stream_.CanProcessIncomingCommand()) loop below.
So removed this DCHECK.

+  // Fetch and process commands.
+  if (!command_stream_.Fetch()) {
+    LOG(ERROR) << "Failed to fetch commands.";
+    return false;
+  }
+  while (command_stream_.CanProcessIncomingCommand()) {
+    if (!command_stream_.ProcessIncomingCommand()) {
+      LOG(ERROR) << "Failed to process command.";
+      return false;
+    }
+  }
+  // Flush outgoing commands.
+  if (!command_stream_.Flush()) {
+    LOG(ERROR) << "Failed to flush commands.";
+    return false;
+  }
+  return true;
+}
+
 bool CommandBroker::Transact(int32_t handle,
                              const TransactionData& request,
                              scoped_ptr<TransactionData>* reply) {
   DCHECK(thread_checker_.CalledOnValidThread());
   // Send transaction.
   binder_transaction_data tr = ConvertTransactionDataToStruct(request);
   tr.target.handle = handle;
   command_stream_.AppendOutgoingCommand(BC_TRANSACTION, &tr, sizeof(tr));
   if (!command_stream_.Flush()) {
     LOG(ERROR) << "Failed to write";
@@ skipping 33 matching lines @@
   command_stream_.AppendOutgoingCommand(BC_RELEASE, &handle, sizeof(handle));
   // Decrement weak reference count.
   command_stream_.AppendOutgoingCommand(BC_DECREFS, &handle, sizeof(handle));
 }
 
 base::Closure CommandBroker::GetReleaseReferenceClosure(int32_t handle) {
   return base::Bind(&CommandBroker::ReleaseReference,
                     weak_ptr_factory_.GetWeakPtr(), handle);
 }
 
+bool CommandBroker::OnTransaction(const TransactionData& data) {
+  LocalObject* object = reinterpret_cast<LocalObject*>(data.GetCookie());
+  scoped_ptr<TransactionData> reply;
+  if (!object->Transact(this, data, &reply)) {
+    LOG(ERROR) << "Failed to transact.";
+    return false;
+  }
+  if (!data.IsOneWay()) {
+    // Send reply.
+    if (!reply) {
+      reply.reset(new TransactionStatus(Status::FAILED_TRANSACTION));
+    }
+    binder_transaction_data tr = ConvertTransactionDataToStruct(*reply);
+    tr.target.handle = -1;

[satorux1, 2016/01/13 04:33:20]

maybe document why -1? 

[hashimoto, 2016/01/13 05:46:46]

Done.

+    command_stream_.AppendOutgoingCommand(BC_REPLY, &tr, sizeof(tr));
+    if (!command_stream_.Flush()) {
+      LOG(ERROR) << "Failed to write";
+      return false;
+    }
+    scoped_ptr<TransactionData> response_data;
+    ResponseType response_type = WaitForResponse(&response_data);
+    LOG_IF(ERROR, response_type != RESPONSE_TYPE_TRANSACTION_COMPLETE)
+        << "Error on the other end when sending reply: " << response_type;

[satorux1, 2016/01/13 04:33:20]

in this case, should this return true?

[hashimoto, 2016/01/13 05:46:46]

Added a comment.

+  }
+  return true;
+}
+
 void CommandBroker::OnReply(scoped_ptr<TransactionData> data) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(response_type_, RESPONSE_TYPE_NONE);
   DCHECK(!response_data_);
   response_type_ = RESPONSE_TYPE_TRANSACTION_REPLY;
   response_data_ = std::move(data);
 }
 
 void CommandBroker::OnDeadReply() {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ skipping 47 matching lines @@
       }
     }
   }
   ResponseType response_type = response_type_;
   response_type_ = RESPONSE_TYPE_NONE;
   *data = std::move(response_data_);
   return response_type;
 }
 
 }  // namespace binder