Refactored ErrorCode into it's own class

Refactored ErrorCode into it's own class. Removed operators and made them
explicit functions. Added code that allows testing of the sandbox infrastructure
with Valgrind (this currently only works when building with the Makefile; we
still need to decide if and how we want to expose this to our unittests).

Added a way to unittest the ErrorCode class.

BUG=n/a
TEST=sandbox_linux_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154397

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

Here is a first pass, mostly style guide nit picking.

As general remarks:

- accessor and mutators for accessing a friends' class members make everything a
lot easier to follow. For instance it's nice to be able to assert that a friend
class will never modify a private member because there is no mutator.

- Please, add unit tests!

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/errorcode.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.cc:35: errorType_(ET_TRAP),
Initializer lists should be indented 4 spaces, the colon should be on the first
initializer line.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.cc:43: errorType_(ET_COND),
Same here.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.cc:77: bool ErrorCode::lessThan(const
ErrorCode& err) const {
This is quit "artificial". Do you mind adding a comment explaining why this is
needed?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.cc:96: return
passed_->lessThan(*err.passed_);
Do you want to add a CHECK here, to check that &err != passed_ && &err !=
failed_ or something along those lines ?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/errorcode.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:10: class ErrorCode {
Do you mind adding a descriptive comment for the class, here ?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:51: ErrorCode(int err = ERR_INVALID);
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.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:52: 
Please add an explicit copy constructor (since I think we need it), or otherwise
disallow it with DISALLOW_COPY_AND_ASSIGN.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:56: bool isEqual(const ErrorCode& err)
const;
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().

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:60: bool operator()(const ErrorCode& a,
const ErrorCode& b) const {
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.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:66: friend class CodeGen;
Nit: this class doesn't exist for now. Feel free to keep if you know it'll
exist.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:82: enum {
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.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/errorcode.h:96: int       argno_;
Please add some documentation to those members. Maybe more explicit names as
well ?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:261: bool Sandbox::isDenied(ErrorCode
code) {
Shouldn't this be a const reference ?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:264: code.err_ <= (SECCOMP_RET_ERRNO +
ErrorCode::ERR_MAX_ERRNO));
Please create an accessor err() in the ErrorCode class.

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:142: #include
"sandbox/linux/seccomp-bpf/errorcode.h"
Can this be moved up towards the other includes?

I suspect not, maybe we should have another include file for all those #define
here ?

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:290: static int getProcFd() { return
proc_fd_; }
Style: accessor should be named proc_fd()

https://chromiumcodereview.appspot.com/10833044/diff/2002/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:337: static TrapIds       trapIds_;
This probably should not be part of this CL, but we should rename all those
members with "unix_hacker_style_" names as per the style guide.

[Markus (顧孟勤), 2012-07-30 18:30:27]

PTAL

[jln (very slow on Chromium), 2012-07-30 18:50:39]

A few minor nits remaining.

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/errorcode.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.cc:35: error_type_(ET_TRAP),
See below for nit on initializer.

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.cc:43: error_type_(ET_COND),
The colon should be here. Four spaces, colon, space, initializer:

    : some_var_(var),             // 4 space indent
      some_other_var_(var + 1) {  // lined up

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/errorcode.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:11: 
The class documentation should be right above the class declaration, here. My
bad for pointing the the wrong line previously.

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:107: void    *aux_;             //  
triggered by the kernel's BPF filter.
Nit: C++ style "void*  aux;"

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:117: const ErrorCode *failed_;  //   or if
it failed.
Nit: C++ style "X* ptr"

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/13001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:48: #ifdef SECCOMP_BPF_VALGRIND_HACKS
In theory this should be #if defined(SECCOMP....). For the sake of consistency
it's fine to keep it as #ifdef, unless you want to change them all.

[jln (very slow on Chromium), 2012-07-30 19:01:45]

Please don't forget the unit tests, and perhaps to edit the commit message, I
believe line breaks have been lost for some reason.

[Markus (顧孟勤), 2012-08-14 00:31:49]

Based on the feedback that you gave me offline, I added code to
unit_tests.{cc,h} and refactored our die() method into a new class (die.{cc,h}).

This allowed adding proper unittests for ErrorCode. The unittests themselves are
nice and small -- the rest of the infrastructure to make them work is
unfortunately a little more complicated. But it should be usable for other
unittests as well.

[jln (very slow on Chromium), 2012-08-14 01:48:13]

Could you split the CL for unit_tests.cc into its own CL ?

Here are the first comments.

The most important is that we really don't want this to depend on the
seccomp-bpf defined Die class.

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
File sandbox/linux/tests/unit_tests.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.cc:84: void UnitTests::AssertionFailure(const
char *expr, const char *file, int line){
Style Missing space before "{"

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.cc:90: int main(int argc, char *argv[]) {
Please split this out to main.cc

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
File sandbox/linux/tests/unit_tests.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:8: #include <setjmp.h>
:(

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:16: 
Please add namespace sandbox {

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:33: ? static_cast<void>(0)                     
                               \
Style: I think it would be nicer to have the ternary operators at the end of the
previous line ?

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:43: playground2::Die
custom_handler(UnitTests::CaptureError);               \
We really shouldn't have a dependency on seccomp-bpf here.

Can you abstract this?

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:47: } while (false)
Note: you can also create macro blocks with ({ }). I'm happy with do { } while
as well though since it's generic and generally well understood.

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:49: class UnitTests {
Please add a comment explaining that this is not thread-safe.

Also I would really prefer if you made this a singleton instance.

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:57: static jmp_buf& JumpEnv() { return
jmp_env_; }
Style: the accessor should be called jmp_env().
It should also be a const method.

But really, what we need is a SetJmp() method instead, since jmp_env_ will
obviously be modified.

https://chromiumcodereview.appspot.com/10833044/diff/11002/sandbox/linux/test...
sandbox/linux/tests/unit_tests.h:59: static const std::string& LastError() {
return last_error_; }
Accessor should be const and called last_error().

[Markus (顧孟勤), 2012-08-31 00:37:22]

PTAL

[jln (very slow on Chromium), 2012-08-31 01:16:00]

Looks good overall, with some nits. I didn't point out all the "type* blah"
ones.

We really need to clean-up the SandboxBpf class though!

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/Makefile (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/Makefile:2: DEF_CPPFLAGS = -D_GNU_SOURCE
-DSECCOMP_BPF_STANDALONE -DSECCOMP_BPF_VALGRIND_HACKS -include
valgrind/valgrind.h -iquote ../../..
80 columns in this file would be nice. But up to you.

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/die.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/die.h:7: 
I'm pretty sure that comes from a rebase :)

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/errorcode.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.cc:25:
ErrorCode::ErrorCode(ErrorCode::TrapFnc fnc, const void *aux, uint16_t id)
nit: void* aux

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.cc:33: const ErrorCode *passed, const
ErrorCode *failed)
nit: type* ptr

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/errorcode.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:21: public:
nit: public should be indented one space

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:42: typedef intptr_t (*TrapFnc)(const
struct arch_seccomp_data& args, void *aux);
nit: "void* aux"

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:96: ErrorCode(TrapFnc fnc, const void
*aux, uint16_t id);
nit: type* ptr

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:115: int       argno_;          // Syscall
arg number that we are inspecting
Nit: add "." at the end of sentences.

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode.h:118: const ErrorCode *passed_;  // Value
to be returned if comparison passed,
nit: type* ptr

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/errorcode_unittest.cc (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/errorcode_unittest.cc:77: } // namespace
nit: missing one space.

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:251: static ErrorCode
Trap(ErrorCode::TrapFnc fnc, const void *aux);
I find this name confusing. Maybe GetTrapErrorCode() or some such ?

https://chromiumcodereview.appspot.com/10833044/diff/19001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:254: static ErrorCode Kill(const char
*msg);
Same here?

[jln (very slow on Chromium), 2012-08-31 02:01:51]

lgtm

[commit-bot: I haz the power, 2012-08-31 02:05:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/10833044/30001

[commit-bot: I haz the power, 2012-08-31 06:33:37]

Change committed as 154397