Trusted plugin: Break up InitCommunication().

ppapi/native_client/src/trusted/plugin/service_runtime.cc

 /*
  * Copyright (c) 2012 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.
  */
 
 #define NACL_LOG_MODULE_NAME "Plugin::ServiceRuntime"
 
 #include "ppapi/native_client/src/trusted/plugin/service_runtime.h"
 
@@ skipping 472 matching lines @@
                                                 manifest,
                                                 this,
                                                 init_done_cb, crash_cb)),
       exit_status_(-1),
       start_sel_ldr_done_(false) {
   NaClSrpcChannelInitialize(&command_channel_);
   NaClXMutexCtor(&mu_);
   NaClXCondVarCtor(&cond_);
 }
 
-bool ServiceRuntime::InitCommunication(nacl::DescWrapper* nacl_desc,
-                                       ErrorInfo* error_info) {
-  NaClLog(4, "ServiceRuntime::InitCommunication"
+bool ServiceRuntime::LoadModule(nacl::DescWrapper* nacl_desc,
+                                ErrorInfo* error_info) {
+  NaClLog(4, "ServiceRuntime::LoadModule"
           " (this=%p, subprocess=%p)\n",
           static_cast<void*>(this),
           static_cast<void*>(subprocess_.get()));
   // Create the command channel to the sel_ldr and load the nexe from nacl_desc.
-  if (!subprocess_->SetupCommandAndLoad(&command_channel_, nacl_desc)) {
+  if (!subprocess_->SetupCommand(&command_channel_)) {
     error_info->SetReport(ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
                           "ServiceRuntime: command channel creation failed");
     return false;
   }
+
+  if (nacl_desc && !subprocess_->LoadModule(&command_channel_, nacl_desc)) {

[Mark Seaborn, 2014/01/16 22:02:41]

You might want to remove the NULL check for nacl_desc here for clarity.  I know
you're preserving the check that SetupCommandAndLoad() does, but I suspect that
nacl_desc should never be NULL here (though I could be wrong).

+    error_info->SetReport(ERROR_SEL_LDR_COMMUNICATION_CMD_CHANNEL,
+                          "ServiceRuntime: load module failed");
+    return false;
+  }
+  return true;
+}
+
+bool ServiceRuntime::InitReverseService(ErrorInfo* error_info) {
   // Hook up the reverse service channel.  We are the IMC client, but
   // provide SRPC service.
   NaClDesc* out_conn_cap;
   NaClSrpcResultCodes rpc_result =
       NaClSrpcInvokeBySignature(&command_channel_,
                                 "reverse_setup::h",
                                 &out_conn_cap);
 
   if (NACL_SRPC_RESULT_OK != rpc_result) {
     error_info->SetReport(ERROR_SEL_LDR_COMMUNICATION_REV_SETUP,
                           "ServiceRuntime: reverse setup rpc failed");
     return false;
   }
   //  Get connection capability to service runtime where the IMC
   //  server/SRPC client is waiting for a rendezvous.
   NaClLog(4, "ServiceRuntime: got 0x%" NACL_PRIxPTR "\n",
           (uintptr_t) out_conn_cap);
   nacl::DescWrapper* conn_cap = plugin_->wrapper_factory()->MakeGenericCleanup(
       out_conn_cap);
   if (conn_cap == NULL) {
     error_info->SetReport(ERROR_SEL_LDR_COMMUNICATION_WRAPPER,
                           "ServiceRuntime: wrapper allocation failure");
     return false;
   }
   out_conn_cap = NULL;  // ownership passed
-  NaClLog(4, "ServiceRuntime::InitCommunication: starting reverse service\n");
+  NaClLog(4, "ServiceRuntime::InitReverseService: starting reverse service\n");
   reverse_service_ = new nacl::ReverseService(conn_cap, rev_interface_->Ref());
   if (!reverse_service_->Start()) {
     error_info->SetReport(ERROR_SEL_LDR_COMMUNICATION_REV_SERVICE,
                           "ServiceRuntime: starting reverse services failed");
     return false;
   }
+  return true;
+}
 
+bool ServiceRuntime::StartModule(ErrorInfo* error_info) {
   // start the module.  otherwise we cannot connect for multimedia
   // subsystem since that is handled by user-level code (not secure!)
   // in libsrpc.
   int load_status = -1;
-  rpc_result =
+  NaClSrpcResultCodes rpc_result =
       NaClSrpcInvokeBySignature(&command_channel_,
                                 "start_module::i",
                                 &load_status);
 
   if (NACL_SRPC_RESULT_OK != rpc_result) {
     error_info->SetReport(ERROR_SEL_LDR_START_MODULE,
                           "ServiceRuntime: could not start nacl module");
     return false;
   }
-  NaClLog(4, "ServiceRuntime::InitCommunication (load_status=%d)\n",
+  NaClLog(4, "ServiceRuntime::StartModule (load_status=%d)\n",
           load_status);
   if (main_service_runtime_) {
     plugin_->ReportSelLdrLoadStatus(load_status);
   }
   if (LOAD_OK != load_status) {
     error_info->SetReport(
         ERROR_SEL_LDR_START_STATUS,
         NaClErrorString(static_cast<NaClErrorCode>(load_status)));
     return false;
   }
@@ skipping 47 matching lines @@
   nacl::MutexLocker take(&mu_);
   start_sel_ldr_done_ = true;
   NaClXCondVarSignal(&cond_);
 }
 
 bool ServiceRuntime::LoadNexeAndStart(nacl::DescWrapper* nacl_desc,
                                       ErrorInfo* error_info,
                                       const pp::CompletionCallback& crash_cb) {
   NaClLog(4, "ServiceRuntime::LoadNexeAndStart (nacl_desc=%p)\n",
           reinterpret_cast<void*>(nacl_desc));
-  if (!InitCommunication(nacl_desc, error_info)) {
+  bool ok = LoadModule(nacl_desc, error_info) &&
+            InitReverseService(error_info) &&
+            StartModule(error_info);
+  if (!ok) {
     // On a load failure the service runtime does not crash itself to
     // avoid a race where the no-more-senders error on the reverse
     // channel esrvice thread might cause the crash-detection logic to
     // kick in before the start_module RPC reply has been received. So
     // we induce a service runtime crash here. We do not release
     // subprocess_ since it's needed to collect crash log output after
     // the error is reported.
     Log(LOG_FATAL, "reap logs");
     if (NULL == reverse_service_) {
       // No crash detector thread.
@@ skipping 88 matching lines @@
 
 nacl::string ServiceRuntime::GetCrashLogOutput() {
   if (NULL != subprocess_.get()) {
     return subprocess_->GetCrashLogOutput();
   } else {
     return std::string();
   }
 }
 
 }  // namespace plugin