Linux sandbox: cleanup BrokerProcess class.

A few style guide cleanups and more usage of scopers.

BUG=316486
R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/84183002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236883 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 36 insertions, 33 deletions

diff --git a/sandbox/linux/services/broker_process.cc b/sandbox/linux/services/broker_process.cc
index 81d78b0..0e91c20 100644
--- a/sandbox/linux/services/broker_process.cc
+++ b/sandbox/linux/services/broker_process.cc
@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/posix/eintr_wrapper.h"
@@ -147,13 +148,13 @@ bool BrokerProcess::Init(bool (*sandbox_callback)(void)) {
 
   int child_pid = fork();
   if (child_pid == -1) {
-    (void) HANDLE_EINTR(close(socket_pair[0]));
-    (void) HANDLE_EINTR(close(socket_pair[1]));
+    ignore_result(HANDLE_EINTR(close(socket_pair[0])));
+    ignore_result(HANDLE_EINTR(close(socket_pair[1])));
     return false;
   }
   if (child_pid) {
     // We are the parent and we have just forked our broker process.
-    (void) HANDLE_EINTR(close(socket_pair[0]));
+    ignore_result(HANDLE_EINTR(close(socket_pair[0])));
     // We should only be able to write to the IPC channel. We'll always send
     // a new file descriptor to receive the reply on.
     shutdown(socket_pair[1], SHUT_RD);
@@ -164,7 +165,7 @@ bool BrokerProcess::Init(bool (*sandbox_callback)(void)) {
     return true;
   } else {
     // We are the broker.
-    (void) HANDLE_EINTR(close(socket_pair[1]));
+    ignore_result(HANDLE_EINTR(close(socket_pair[1])));
     // We should only be able to read from this IPC channel. We will send our
     // replies on a new file descriptor attached to the requests.
     shutdown(socket_pair[0], SHUT_WR);
diff --git a/sandbox/linux/services/broker_process.h b/sandbox/linux/services/broker_process.h
index 00218a0..6b13b33 100644
--- a/sandbox/linux/services/broker_process.h
+++ b/sandbox/linux/services/broker_process.h
@@ -66,26 +66,31 @@ class BrokerProcess {
     kCommandAccess,
   };
   int PathAndFlagsSyscall(enum IPCCommands command_type,
-                          const char* pathname, int flags) const;
+                          const char* pathname,
+                          int flags) const;
   bool HandleRequest() const;
-  bool HandleRemoteCommand(IPCCommands command_type, int reply_ipc,
-      const Pickle& read_pickle, PickleIterator iter) const;
+  bool HandleRemoteCommand(IPCCommands command_type,
+                           int reply_ipc,
+                           const Pickle& read_pickle,
+                           PickleIterator iter) const;
 
   void AccessFileForIPC(const std::string& requested_filename,
-                        int mode, Pickle* write_pickle) const;
+                        int mode,
+                        Pickle* write_pickle) const;
   void OpenFileForIPC(const std::string& requested_filename,
-                      int flags, Pickle* write_pickle,
+                      int flags,
+                      Pickle* write_pickle,
                       std::vector<int>* opened_files) const;
   bool GetFileNameIfAllowedToAccess(const char*, int, const char**) const;
   bool GetFileNameIfAllowedToOpen(const char*, int, const char**) const;
   const int denied_errno_;
-  bool initialized_;  // Whether we've been through Init() yet.
-  bool is_child_;  // Whether we're the child (broker process).
-  bool fast_check_in_client_;  // Whether to forward a request that we know
-                               // will be denied to the broker.
+  bool initialized_;               // Whether we've been through Init() yet.
+  bool is_child_;                  // Whether we're the child (broker process).
+  bool fast_check_in_client_;      // Whether to forward a request that we know
+                                   // will be denied to the broker.
   bool quiet_failures_for_tests_;  // Disable certain error message when
                                    // testing for failures.
-  pid_t broker_pid_;  // The PID of the broker (child).
+  pid_t broker_pid_;               // The PID of the broker (child).
   const std::vector<std::string> allowed_r_files_;  // Files allowed for read.
   const std::vector<std::string> allowed_w_files_;  // Files allowed for write.
   int ipc_socketpair_;  // Our communication channel to parent or child.
diff --git a/sandbox/linux/services/broker_process_unittest.cc b/sandbox/linux/services/broker_process_unittest.cc
index 6ce294e..4cb9c6f 100644
--- a/sandbox/linux/services/broker_process_unittest.cc
+++ b/sandbox/linux/services/broker_process_unittest.cc
@@ -15,11 +15,15 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/file_util.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/posix/eintr_wrapper.h"
 #include "sandbox/linux/tests/unit_tests.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using file_util::ScopedFD;
+
 namespace sandbox {
 
 namespace {
@@ -68,14 +72,13 @@ TEST(BrokerProcess, CreateAndDestroy) {
   std::vector<std::string> read_whitelist;
   read_whitelist.push_back("/proc/cpuinfo");
 
-  BrokerProcess* open_broker = new BrokerProcess(EPERM,
-                                                 read_whitelist,
-                                                 std::vector<std::string>());
+  scoped_ptr<BrokerProcess> open_broker(
+      new BrokerProcess(EPERM, read_whitelist, std::vector<std::string>()));
   ASSERT_TRUE(open_broker->Init(NULL));
   pid_t broker_pid = open_broker->broker_pid();
-  delete(open_broker);
 
-  // Now we check that the broker has exited properly.
+  // Destroy the broker and check it has exited properly.
+  open_broker.reset();
   int status = 0;
   ASSERT_EQ(waitpid(broker_pid, &status, 0), broker_pid);
   ASSERT_TRUE(WIFEXITED(status));
@@ -224,12 +227,12 @@ void TestOpenFilePerms(bool fast_check_in_client, int denied_errno) {
   ASSERT_EQ(ret, -denied_errno);
 
   // We have some extra sanity check for clearly wrong values.
-  fd = open_broker.Open(kRW_WhiteListed, O_RDONLY|O_WRONLY|O_RDWR);
+  fd = open_broker.Open(kRW_WhiteListed, O_RDONLY | O_WRONLY | O_RDWR);
   ASSERT_EQ(fd, -denied_errno);
 
   // It makes no sense to allow O_CREAT in a 2-parameters open. Ensure this
   // is denied.
-  fd = open_broker.Open(kRW_WhiteListed, O_RDWR|O_CREAT);
+  fd = open_broker.Open(kRW_WhiteListed, O_RDWR | O_CREAT);
   ASSERT_EQ(fd, -denied_errno);
 }
 
@@ -265,15 +268,14 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
   std::vector<std::string> read_whitelist;
   read_whitelist.push_back(kFileCpuInfo);
 
-  BrokerProcess* open_broker = new BrokerProcess(EPERM,
-                                                 read_whitelist,
-                                                 std::vector<std::string>(),
-                                                 fast_check_in_client);
+  scoped_ptr<BrokerProcess> open_broker(new BrokerProcess(
+      EPERM, read_whitelist, std::vector<std::string>(), fast_check_in_client));
   ASSERT_TRUE(open_broker->Init(NULL));
   pid_t broker_pid = open_broker->broker_pid();
 
   int fd = -1;
   fd = open_broker->Open(kFileCpuInfo, O_RDWR);
+  ScopedFD fd_closer(&fd);
   ASSERT_EQ(fd, -EPERM);
 
   // Check we can read /proc/cpuinfo.
@@ -285,6 +287,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
 
   // Open cpuinfo via the broker.
   int cpuinfo_fd = open_broker->Open(kFileCpuInfo, O_RDONLY);
+  ScopedFD cpuinfo_fd_closer(&cpuinfo_fd);
   ASSERT_GE(cpuinfo_fd, 0);
   char buf[3];
   memset(buf, 0, sizeof(buf));
@@ -293,6 +296,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
 
   // Open cpuinfo directly.
   int cpuinfo_fd2 = open(kFileCpuInfo, O_RDONLY);
+  ScopedFD cpuinfo_fd2_closer(&cpuinfo_fd2);
   ASSERT_GE(cpuinfo_fd2, 0);
   char buf2[3];
   memset(buf2, 1, sizeof(buf2));
@@ -305,14 +309,7 @@ void TestOpenCpuinfo(bool fast_check_in_client) {
   // ourselves.
   ASSERT_EQ(memcmp(buf, buf2, read_len1), 0);
 
-  if (fd >= 0)
-    close(fd);
-  if (cpuinfo_fd >= 0)
-    close(cpuinfo_fd);
-  if (cpuinfo_fd2 >= 0)
-    close(cpuinfo_fd);
-
-  delete(open_broker);
+  open_broker.reset();
 
   // Now we check that the broker has exited properly.
   int status = 0;