sandbox: style cleanup

Based on readability review by Dean Berris at Google.

R=jln@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#292698}

sandbox/linux/bpf_dsl/bpf_dsl.cc

@@ -6,67 +6,75 @@
 
 #include <errno.h>
 
+#include <limits>
+
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "sandbox/linux/seccomp-bpf/errorcode.h"
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf.h"
 
-using namespace sandbox::bpf_dsl::internal;
-typedef ::sandbox::Trap::TrapFnc TrapFnc;
-
 namespace sandbox {
 namespace bpf_dsl {
 namespace {
 
-class AllowResultExprImpl : public ResultExprImpl {
+class AllowResultExprImpl : public internal::ResultExprImpl {
  public:
   AllowResultExprImpl() {}
+
   virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE {
     return ErrorCode(ErrorCode::ERR_ALLOWED);
   }
 
  private:
   virtual ~AllowResultExprImpl() {}
+
   DISALLOW_COPY_AND_ASSIGN(AllowResultExprImpl);
 };
 
-class ErrorResultExprImpl : public ResultExprImpl {
+class ErrorResultExprImpl : public internal::ResultExprImpl {
  public:
   explicit ErrorResultExprImpl(int err) : err_(err) {
     CHECK(err_ >= ErrorCode::ERR_MIN_ERRNO && err_ <= ErrorCode::ERR_MAX_ERRNO);
   }
+
   virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE {
     return ErrorCode(err_);
   }
 
  private:
   virtual ~ErrorResultExprImpl() {}
+
   int err_;
+
   DISALLOW_COPY_AND_ASSIGN(ErrorResultExprImpl);
 };
 
-class TrapResultExprImpl : public ResultExprImpl {
+class TrapResultExprImpl : public internal::ResultExprImpl {
  public:
-  TrapResultExprImpl(TrapFnc func, void* arg) : func_(func), arg_(arg) {
+  TrapResultExprImpl(Trap::TrapFnc func, void* arg) : func_(func), arg_(arg) {
     DCHECK(func_);
   }
+
   virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE {
     return sb->Trap(func_, arg_);
   }
 
  private:
   virtual ~TrapResultExprImpl() {}
-  TrapFnc func_;
+
+  Trap::TrapFnc func_;
   void* arg_;
+
   DISALLOW_COPY_AND_ASSIGN(TrapResultExprImpl);
 };
 
-class IfThenResultExprImpl : public ResultExprImpl {
+class IfThenResultExprImpl : public internal::ResultExprImpl {
  public:
-  IfThenResultExprImpl(BoolExpr cond,
-                       ResultExpr then_result,
-                       ResultExpr else_result)
+  IfThenResultExprImpl(const BoolExpr& cond,
+                       const ResultExpr& then_result,
+                       const ResultExpr& else_result)
       : cond_(cond), then_result_(then_result), else_result_(else_result) {}
+
   virtual ErrorCode Compile(SandboxBPF* sb) const OVERRIDE {
     return cond_->Compile(
         sb, then_result_->Compile(sb), else_result_->Compile(sb));
@@ -74,19 +82,22 @@ class IfThenResultExprImpl : public ResultExprImpl {
 
  private:
   virtual ~IfThenResultExprImpl() {}
+
   BoolExpr cond_;
   ResultExpr then_result_;
   ResultExpr else_result_;
+
   DISALLOW_COPY_AND_ASSIGN(IfThenResultExprImpl);
 };
 
-class PrimitiveBoolExprImpl : public BoolExprImpl {
+class PrimitiveBoolExprImpl : public internal::BoolExprImpl {
  public:
   PrimitiveBoolExprImpl(int argno,
                         ErrorCode::ArgType is_32bit,
                         ErrorCode::Operation op,
                         uint64_t value)
       : argno_(argno), is_32bit_(is_32bit), op_(op), value_(value) {}
+
   virtual ErrorCode Compile(SandboxBPF* sb,
                             ErrorCode true_ec,
                             ErrorCode false_ec) const OVERRIDE {
@@ -95,16 +106,19 @@ class PrimitiveBoolExprImpl : public BoolExprImpl {
 
  private:
   virtual ~PrimitiveBoolExprImpl() {}
+
   int argno_;
   ErrorCode::ArgType is_32bit_;
   ErrorCode::Operation op_;
   uint64_t value_;
+
   DISALLOW_COPY_AND_ASSIGN(PrimitiveBoolExprImpl);
 };
 
-class NegateBoolExprImpl : public BoolExprImpl {
+class NegateBoolExprImpl : public internal::BoolExprImpl {
  public:
-  explicit NegateBoolExprImpl(BoolExpr cond) : cond_(cond) {}
+  explicit NegateBoolExprImpl(const BoolExpr& cond) : cond_(cond) {}
+
   virtual ErrorCode Compile(SandboxBPF* sb,
                             ErrorCode true_ec,
                             ErrorCode false_ec) const OVERRIDE {
@@ -113,13 +127,17 @@ class NegateBoolExprImpl : public BoolExprImpl {
 
  private:
   virtual ~NegateBoolExprImpl() {}
+
   BoolExpr cond_;
+
   DISALLOW_COPY_AND_ASSIGN(NegateBoolExprImpl);
 };
 
-class AndBoolExprImpl : public BoolExprImpl {
+class AndBoolExprImpl : public internal::BoolExprImpl {
  public:
-  AndBoolExprImpl(BoolExpr lhs, BoolExpr rhs) : lhs_(lhs), rhs_(rhs) {}
+  AndBoolExprImpl(const BoolExpr& lhs, const BoolExpr& rhs)
+      : lhs_(lhs), rhs_(rhs) {}
+
   virtual ErrorCode Compile(SandboxBPF* sb,
                             ErrorCode true_ec,
                             ErrorCode false_ec) const OVERRIDE {
@@ -128,13 +146,18 @@ class AndBoolExprImpl : public BoolExprImpl {
 
  private:
   virtual ~AndBoolExprImpl() {}
-  BoolExpr lhs_, rhs_;
+
+  BoolExpr lhs_;
+  BoolExpr rhs_;
+
   DISALLOW_COPY_AND_ASSIGN(AndBoolExprImpl);
 };
 
-class OrBoolExprImpl : public BoolExprImpl {
+class OrBoolExprImpl : public internal::BoolExprImpl {
  public:
-  OrBoolExprImpl(BoolExpr lhs, BoolExpr rhs) : lhs_(lhs), rhs_(rhs) {}
+  OrBoolExprImpl(const BoolExpr& lhs, const BoolExpr& rhs)
+      : lhs_(lhs), rhs_(rhs) {}
+
   virtual ErrorCode Compile(SandboxBPF* sb,
                             ErrorCode true_ec,
                             ErrorCode false_ec) const OVERRIDE {
@@ -143,7 +166,10 @@ class OrBoolExprImpl : public BoolExprImpl {
 
  private:
   virtual ~OrBoolExprImpl() {}
-  BoolExpr lhs_, rhs_;
+
+  BoolExpr lhs_;
+  BoolExpr rhs_;
+
   DISALLOW_COPY_AND_ASSIGN(OrBoolExprImpl);
 };
 
@@ -161,23 +187,27 @@ BoolExpr ArgEq(int num, size_t size, uint64_t mask, uint64_t val) {
   const ErrorCode::ArgType arg_type =
       (size <= 4) ? ErrorCode::TP_32BIT : ErrorCode::TP_64BIT;
 
-  if (mask == static_cast<uint64_t>(-1)) {
+  if (mask == std::numeric_limits<uint64_t>::max()) {
     // Arg == Val
     return BoolExpr(new const PrimitiveBoolExprImpl(
         num, arg_type, ErrorCode::OP_EQUAL, val));
-  } else if (mask == val) {
+  }
+
+  if (mask == val) {
     // (Arg & Mask) == Mask
     return BoolExpr(new const PrimitiveBoolExprImpl(
         num, arg_type, ErrorCode::OP_HAS_ALL_BITS, mask));
-  } else if (val == 0) {
+  }
+
+  if (val == 0) {
     // (Arg & Mask) == 0, which is semantically equivalent to !((arg & mask) !=
     // 0).
     return !BoolExpr(new const PrimitiveBoolExprImpl(
         num, arg_type, ErrorCode::OP_HAS_ANY_BITS, mask));
-  } else {
-    CHECK(false) << "Unimplemented ArgEq case";
-    return BoolExpr();
   }
+
+  CHECK(false) << "Unimplemented ArgEq case";
+  return BoolExpr();
 }
 
 }  // namespace internal
@@ -190,23 +220,23 @@ ResultExpr Error(int err) {
   return ResultExpr(new const ErrorResultExprImpl(err));
 }
 
-ResultExpr Trap(TrapFnc trap_func, void* aux) {
+ResultExpr Trap(Trap::TrapFnc trap_func, void* aux) {
   return ResultExpr(new const TrapResultExprImpl(trap_func, aux));
 }
 
-BoolExpr operator!(BoolExpr cond) {
+BoolExpr operator!(const BoolExpr& cond) {
   return BoolExpr(new const NegateBoolExprImpl(cond));
 }
 
-BoolExpr operator&&(BoolExpr lhs, BoolExpr rhs) {
+BoolExpr operator&&(const BoolExpr& lhs, const BoolExpr& rhs) {
   return BoolExpr(new const AndBoolExprImpl(lhs, rhs));
 }
 
-BoolExpr operator||(BoolExpr lhs, BoolExpr rhs) {
+BoolExpr operator||(const BoolExpr& lhs, const BoolExpr& rhs) {
   return BoolExpr(new const OrBoolExprImpl(lhs, rhs));
 }
 
-Elser If(BoolExpr cond, ResultExpr then_result) {
+Elser If(const BoolExpr& cond, const ResultExpr& then_result) {
   return Elser(Cons<Elser::Clause>::List()).ElseIf(cond, then_result);
 }
 
@@ -219,12 +249,12 @@ Elser::Elser(const Elser& elser) : clause_list_(elser.clause_list_) {
 Elser::~Elser() {
 }
 
-Elser Elser::ElseIf(BoolExpr cond, ResultExpr then_result) const {
+Elser Elser::ElseIf(const BoolExpr& cond, const ResultExpr& then_result) const {
   return Elser(
       Cons<Clause>::Make(std::make_pair(cond, then_result), clause_list_));
 }
 
-ResultExpr Elser::Else(ResultExpr else_result) const {
+ResultExpr Elser::Else(const ResultExpr& else_result) const {
   // We finally have the default result expression for this
   // if/then/else sequence.  Also, we've already accumulated all
   // if/then pairs into a list of reverse order (i.e., lower priority
@@ -269,8 +299,7 @@ ErrorCode SandboxBPFDSLPolicy::InvalidSyscall(SandboxBPF* sb) const {
   return InvalidSyscall()->Compile(sb);
 }
 
-ResultExpr SandboxBPFDSLPolicy::Trap(::sandbox::Trap::TrapFnc trap_func,
-                                     void* aux) {
+ResultExpr SandboxBPFDSLPolicy::Trap(Trap::TrapFnc trap_func, void* aux) {
   return bpf_dsl::Trap(trap_func, aux);
 }
 

sandbox/linux/bpf_dsl/bpf_dsl.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <limits>
 #include <utility>
 
 #include "base/macros.h"
@@ -108,7 +109,7 @@ class SANDBOX_EXPORT SandboxBPFDSLPolicy : public SandboxBPFPolicy {
   virtual ErrorCode InvalidSyscall(SandboxBPF* sb) const OVERRIDE FINAL;
 
   // Helper method so policies can just write Trap(func, aux).
-  static ResultExpr Trap(::sandbox::Trap::TrapFnc trap_func, void* aux);
+  static ResultExpr Trap(Trap::TrapFnc trap_func, void* aux);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SandboxBPFDSLPolicy);
@@ -127,41 +128,48 @@ SANDBOX_EXPORT ResultExpr Error(int err);
 // Trap specifies a result that the system call should be handled by
 // trapping back into userspace and invoking |trap_func|, passing
 // |aux| as the second parameter.
-SANDBOX_EXPORT ResultExpr Trap(::sandbox::Trap::TrapFnc trap_func, void* aux);
+SANDBOX_EXPORT ResultExpr Trap(Trap::TrapFnc trap_func, void* aux);
 
 template <typename T>
 class SANDBOX_EXPORT Arg {
  public:
   // Initializes the Arg to represent the |num|th system call
   // argument (indexed from 0), which is of type |T|.
-  explicit Arg(int num) : num_(num), mask_(-1) {}
+  explicit Arg(int num)
+      : num_(num), mask_(std::numeric_limits<uint64_t>::max()) {}
 
   Arg(const Arg& arg) : num_(arg.num_), mask_(arg.mask_) {}
 
   // Returns an Arg representing the current argument, but after
   // bitwise-and'ing it with |rhs|.
-  Arg operator&(uint64_t rhs) const { return Arg(num_, mask_ & rhs); }
+  friend Arg operator&(const Arg& lhs, uint64_t rhs) {
+    return Arg(lhs.num_, lhs.mask_ & rhs);
+  }
 
   // Returns a boolean expression comparing whether the system call
   // argument (after applying any bitmasks, if appropriate) equals |rhs|.
-  BoolExpr operator==(T rhs) const;
+  friend BoolExpr operator==(const Arg& lhs, T rhs) { return lhs.EqualTo(rhs); }
 
  private:
   Arg(int num, uint64_t mask) : num_(num), mask_(mask) {}
+
+  BoolExpr EqualTo(T val) const;
+
   int num_;
   uint64_t mask_;
+
   DISALLOW_ASSIGN(Arg);
 };
 
 // Various ways to combine boolean expressions into more complex expressions.
 // They follow standard boolean algebra laws.
-SANDBOX_EXPORT BoolExpr operator!(BoolExpr cond);
-SANDBOX_EXPORT BoolExpr operator&&(BoolExpr lhs, BoolExpr rhs);
-SANDBOX_EXPORT BoolExpr operator||(BoolExpr lhs, BoolExpr rhs);
+SANDBOX_EXPORT BoolExpr operator!(const BoolExpr& cond);
+SANDBOX_EXPORT BoolExpr operator&&(const BoolExpr& lhs, const BoolExpr& rhs);
+SANDBOX_EXPORT BoolExpr operator||(const BoolExpr& lhs, const BoolExpr& rhs);
 
 // If begins a conditional result expression predicated on the
 // specified boolean expression.
-SANDBOX_EXPORT Elser If(BoolExpr cond, ResultExpr then_result);
+SANDBOX_EXPORT Elser If(const BoolExpr& cond, const ResultExpr& then_result);
 
 class SANDBOX_EXPORT Elser {
  public:
@@ -170,17 +178,20 @@ class SANDBOX_EXPORT Elser {
 
   // ElseIf extends the conditional result expression with another
   // "if then" clause, predicated on the specified boolean expression.
-  Elser ElseIf(BoolExpr cond, ResultExpr then_result) const;
+  Elser ElseIf(const BoolExpr& cond, const ResultExpr& then_result) const;
 
   // Else terminates a conditional result expression using |else_result| as
   // the default fallback result expression.
-  ResultExpr Else(ResultExpr else_result) const;
+  ResultExpr Else(const ResultExpr& else_result) const;
 
  private:
   typedef std::pair<BoolExpr, ResultExpr> Clause;
+
   explicit Elser(Cons<Clause>::List clause_list);
+
   Cons<Clause>::List clause_list_;
-  friend Elser If(BoolExpr, ResultExpr);
+
+  friend Elser If(const BoolExpr&, const ResultExpr&);
   DISALLOW_ASSIGN(Elser);
 };
 
@@ -235,9 +246,13 @@ class SANDBOX_EXPORT ResultExprImpl : public base::RefCounted<ResultExprImpl> {
 // Definition requires ArgEq to have been declared.  Moved out-of-line
 // to minimize how much internal clutter users have to ignore while
 // reading the header documentation.
+//
+// Additionally, we use this helper member function to avoid linker errors
+// caused by defining operator== out-of-line.  For a more detailed explanation,
+// see http://www.parashift.com/c++-faq-lite/template-friends.html.
 template <typename T>
-BoolExpr Arg<T>::operator==(T rhs) const {
-  return internal::ArgEq(num_, sizeof(T), mask_, static_cast<uint64_t>(rhs));
+BoolExpr Arg<T>::EqualTo(T val) const {
+  return internal::ArgEq(num_, sizeof(T), mask_, static_cast<uint64_t>(val));
 }
 
 }  // namespace bpf_dsl

sandbox/linux/bpf_dsl/bpf_dsl_unittest.cc

@@ -16,8 +16,6 @@
 #include "sandbox/linux/seccomp-bpf/sandbox_bpf_policy.h"
 #include "sandbox/linux/seccomp-bpf/syscall.h"
 
-using namespace sandbox::bpf_dsl;
-
 // Helper macro to assert that invoking system call |sys| directly via
 // Syscall::Call with arguments |...| returns |res|.
 // Errors can be asserted by specifying a value like "-EINVAL".
@@ -25,6 +23,7 @@ using namespace sandbox::bpf_dsl;
   BPF_ASSERT_EQ(res, Stubs::sys(__VA_ARGS__))
 
 namespace sandbox {
+namespace bpf_dsl {
 namespace {
 
 // Type safe stubs for tested system calls.
@@ -265,4 +264,5 @@ BPF_TEST_C(BPFDSL, ElseIfTest, ElseIfPolicy) {
 }
 
 }  // namespace
+}  // namespace bpf_dsl
 }  // namespace sandbox