Pepper: Cleanup in PepperReverseInterface.

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 119 matching lines @@
     int32_t err) {
   UNREFERENCED_PARAMETER(err);
   NaClLog(4,
           "PluginReverseInterface::PostMessage_MainThreadContinuation(%s)\n",
           p->message.c_str());
   plugin_->PostMessage(std::string("DEBUG_POSTMESSAGE:") + p->message);
 }
 
 bool PluginReverseInterface::EnumerateManifestKeys(
     std::set<nacl::string>* out_keys) {
-  Manifest const* mp = manifest_;
-
-  if (!mp->GetFileKeys(out_keys)) {
-    return false;
-  }
-
-  return true;
+  return manifest_->GetFileKeys(out_keys);
 }
 
 // TODO(bsy): OpenManifestEntry should use the manifest to ResolveKey
 // and invoke StreamAsFile with a completion callback that invokes
 // GetPOSIXFileDesc.
 bool PluginReverseInterface::OpenManifestEntry(nacl::string url_key,
                                                struct NaClFileInfo* info) {
   ErrorInfo error_info;
   bool op_complete = false;  // NB: mu_ and cv_ also controls access to this!
   // The to_open object is owned by the weak ref callback. Because this function
   // waits for the callback to finish, the to_open object will be deallocated on
   // the main thread before this function can return. The pointers it contains
   // to stack variables will not leak.
   OpenManifestEntryResource* to_open =
       new OpenManifestEntryResource(url_key, info,
                                     &error_info, &op_complete);
   CHECK(to_open != NULL);
   NaClLog(4, "PluginReverseInterface::OpenManifestEntry: %s\n",
           url_key.c_str());
   // This assumes we are not on the main thread.  If false, we deadlock.
   plugin::WeakRefCallOnMainThread(
       anchor_,
       0,
       this,
       &plugin::PluginReverseInterface::OpenManifestEntry_MainThreadContinuation,
       to_open);
   NaClLog(4,
           "PluginReverseInterface::OpenManifestEntry:"
           " waiting on main thread\n");
-  bool shutting_down;
-  do {
+  {
     nacl::MutexLocker take(&mu_);
-    for (;;) {
+    while (true) {

[dmichael (off chromium), 2014/03/05 16:54:51]

Could this instead be:
while (!shutting_down_ && !op_complete)
  NaClXCondVarWait(&cv_, &mu);
NaClLog("... done!");  // <-- The "done!" log happens for either
                       // case in the old code
if (shutting_down_) {
  NaClLog("... plugin is shutting down\n");
  return false;
}

Or am I misunderstanding something?

       NaClLog(4,
               "PluginReverseInterface::OpenManifestEntry:"
               " got lock, checking shutdown and completion: (%s, %s)\n",
               shutting_down_ ? "yes" : "no",
               op_complete ? "yes" : "no");
-      shutting_down = shutting_down_;
-      if (op_complete || shutting_down) {
+      if (shutting_down_) {
+        NaClLog(4,
+                "PluginReverseInterface::OpenManifestEntry:"
+                " plugin is shutting down\n");
+        return false;
+      }
+      if (op_complete) {
         NaClLog(4,
                 "PluginReverseInterface::OpenManifestEntry:"
                 " done!\n");
         break;
       }
       NaClXCondVarWait(&cv_, &mu_);
     }
-  } while (0);
-  if (shutting_down) {
-    NaClLog(4,
-            "PluginReverseInterface::OpenManifestEntry:"
-            " plugin is shutting down\n");
-    return false;
   }
-  // out_desc has the returned descriptor if successful, else -1.
 
-  // The caller is responsible for not closing *out_desc.  If it is
+  // info->desc has the returned descriptor if successful, else -1.
+
+  // The caller is responsible for not closing info->desc.  If it is
   // closed prematurely, then another open could re-use the OS
   // descriptor, confusing the opened_ map.  If the caller is going to
   // want to make a NaClDesc object and transfer it etc., then the
   // caller should DUP the descriptor (but remember the original
   // value) for use by the NaClDesc object, which closes when the
   // object is destroyed.
   NaClLog(4,
-          "PluginReverseInterface::OpenManifestEntry:"
-          " *out_desc = %d\n",
+          "PluginReverseInterface::OpenManifestEntry: info->desc = %d\n",
           info->desc);
   if (info->desc == -1) {
     // TODO(bsy,ncbray): what else should we do with the error?  This
     // is a runtime error that may simply be a programming error in
     // the untrusted code, or it may be something else wrong w/ the
     // manifest.
     NaClLog(4,
             "OpenManifestEntry: failed for key %s, code %d (%s)\n",
             url_key.c_str(),
             error_info.error_code(),
@@ skipping 129 matching lines @@
   *p->op_complete_ptr = true;
   NaClXCondVarBroadcast(&cv_);
 }
 
 bool PluginReverseInterface::CloseManifestEntry(int32_t desc) {
   bool op_complete = false;
   bool op_result;
   CloseManifestEntryResource* to_close =
       new CloseManifestEntryResource(desc, &op_complete, &op_result);
 
-  bool shutting_down;
   plugin::WeakRefCallOnMainThread(
       anchor_,
       0,
       this,
       &plugin::PluginReverseInterface::
         CloseManifestEntry_MainThreadContinuation,
       to_close);
+
   // wait for completion or surf-away.
-  do {
+  {
     nacl::MutexLocker take(&mu_);
-    for (;;) {
-      shutting_down = shutting_down_;
-      if (op_complete || shutting_down) {
+    while (true) {

[dmichael (off chromium), 2014/03/05 16:54:51]

I think you could do the same kind of transformation here, unless I'm missing
something...

while (!shutting_down_ && !op_complete)
  NaClXCondVarWait(&cv_, &mu_);
if (shutting_down_)
  return false;

+      if (shutting_down_)
+        return false;
+      if (op_complete)
         break;
-      }
       NaClXCondVarWait(&cv_, &mu_);
     }
-  } while (0);
+  }
 
-  if (shutting_down) return false;
   // op_result true if close was successful; false otherwise (e.g., bad desc).
   return op_result;
 }
 
 void PluginReverseInterface::CloseManifestEntry_MainThreadContinuation(
     CloseManifestEntryResource* cls,
     int32_t err) {
   UNREFERENCED_PARAMETER(err);
 
   nacl::MutexLocker take(&mu_);
@@ skipping 346 matching lines @@
 
 nacl::string ServiceRuntime::GetCrashLogOutput() {
   if (NULL != subprocess_.get()) {
     return subprocess_->GetCrashLogOutput();
   } else {
     return std::string();
   }
 }
 
 }  // namespace plugin