Refactored ErrorCode into it's own class

sandbox/linux/seccomp-bpf/errorcode.h

+// 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.
+
+#ifndef ERRORCODE_H__
+#define ERRORCODE_H__
+
+namespace playground2 {
+
+class ErrorCode {

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Do you mind adding a descriptive comment for the class, here ?

+public:
+  enum {
+    // Allow this system call.
+    ERR_ALLOWED   = 0x0000,
+
+    // Deny the system call with a particular "errno" value.
+    ERR_MIN_ERRNO = 1,
+    ERR_MAX_ERRNO = 4095,
+
+    // This code should never be used directly, it is used internally only.
+    ERR_INVALID   = -1,
+  };
+
+  // TrapFnc is a pointer to a function that handles Seccomp traps in
+  // user-space. The seccomp policy can request that a trap handler gets
+  // installed; it does so by returning a suitable ErrorCode() from the
+  // syscallEvaluator. See the ErrorCode() constructor for how to pass in
+  // the function pointer.
+  // Please note that TrapFnc is executed from signal context and must be
+  // async-signal safe:
+  // http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html
+  typedef intptr_t (*TrapFnc)(const struct arch_seccomp_data& args, void *aux);
+
+  enum ArgType {
+    TP_32BIT, TP_64BIT,
+  };
+
+  enum Operation {
+    OP_EQUAL, OP_GREATER, OP_GREATER_EQUAL, OP_HAS_BITS,
+    OP_NUM_OPS,
+  };
+
+  // We can either wrap a symbolic ErrorCode (i.e. ERR_XXX enum values), an
+  // errno value (in the range 1..4095), a pointer to a TrapFnc callback
+  // handling a SECCOMP_RET_TRAP trap, or a complex constraint.
+  // All of the commonly used values are stored in the "err_" field. So, code
+  // that is using the ErrorCode class typically operates on a single 32bit
+  // field.
+  // This is not only quite efficient, it also makes the API really easy to
+  // use.
+  ErrorCode(int err = ERR_INVALID);

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Style: please make a default constructor call a constructor that takes one
argument with ERR_INVALID instead of using a default argument.

Also the style guide requires constructors with one argument to use the
"explicit" keyword.

Yes, this will make using the class a tad more painful since we'll loose the
current implicit conversion, but I think it's the right thing to do, it already
confused people in the past.

+

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Please add an explicit copy constructor (since I think we need it), or otherwise
disallow it with DISALLOW_COPY_AND_ASSIGN.

+  // Destructor
+  ~ErrorCode() { }
+
+  bool isEqual(const ErrorCode& err) const;

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Since this is a new class, would you mind capitalizing the first letter of
methods?

minor: our code seem to use "Equals()" frequently instead of IsEqual().

+  bool lessThan(const ErrorCode& err) const;
+
+  struct LessThan {
+    bool operator()(const ErrorCode& a, const ErrorCode& b) const {

[jln (very slow on Chromium), 2012/07/27 19:46:30]

I think this is better declared outside of the class. The style guide says this
should be declared when you declare the container.

For instance, if you need std::sort(), you can declare a functor class in the
anonymous namespace, where this it is used.

+      return a.lessThan(b);
+    }
+  };
+
+ private:
+  friend class CodeGen;

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Nit: this class doesn't exist for now. Feel free to keep if you know it'll
exist.

+  friend class Sandbox;
+  friend class Verifier;
+
+  static void die(const char *msg) __attribute__((noreturn));
+
+  // If we are wrapping a callback, we must assign a unique id. This id is
+  // how the kernel tells us which one of our different SECCOMP_RET_TRAP
+  // cases has been triggered.
+  ErrorCode(TrapFnc fnc, const void *aux, uint16_t id);
+
+  // Some system calls require inspection of arguments. This constructor
+  // allows us to specify additional constraints.
+  ErrorCode(int argno, ArgType width, Operation op, uint64_t value,
+            const ErrorCode *passed, const ErrorCode *failed);
+
+  enum {

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Style: enum and typedefs should be at the start of the private section.

errorType_ should be rename error_type_, and be declared at the end of the
section, since it's a member.

+    ET_INVALID, ET_SIMPLE, ET_TRAP, ET_COND,
+  } errorType_;
+
+  union {
+    // Fields needed for SECCOMP_RET_TRAP callbacks
+    struct {
+      TrapFnc fnc_;
+      void    *aux_;
+    };
+
+    // Fields needed when inspecting additional arguments.
+    struct {
+      uint64_t  value_;
+      int       argno_;

[jln (very slow on Chromium), 2012/07/27 19:46:30]

Please add some documentation to those members. Maybe more explicit names as
well ?

+      ArgType   width_;
+      Operation op_;
+      const ErrorCode *passed_;
+      const ErrorCode *failed_;
+    };
+  };
+
+  // 32bit field used for all possible types of ErrorCode values. This is
+  // the value that uniquely identifies any ErrorCode and it (typically) can
+  // be emitted directly into a BPF filter program.
+  uint32_t err_;
+
+};
+
+}  // namespace
+
+#endif  // ERRORCODE_H__