Linux sandbox: introduce BrokerChannel

Linux sandbox: introduce BrokerChannel

Introduce a small BrokerChannel class to create the IPC channel for the
broker process.
Also clean-up ownership of the IPC channel and use scopers.

BUG=429028
Committed: https://crrev.com/878dca3cca9b96886ddb4233adb5c7fca779fbcc
Cr-Commit-Position: refs/heads/master@{#303602}

[jln (very slow on Chromium), 2014-11-11 05:03:15]

jln@chromium.org changed reviewers:
+ mdempsky@chromium.org

[jln (very slow on Chromium), 2014-11-11 05:03:28]

Matthew, would you mind taking a quick look at this small CL?

[mdempsky, 2014-11-11 05:16:13]

lgtm

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
File sandbox/linux/syscall_broker/broker_channel.h (right):

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
sandbox/linux/syscall_broker/broker_channel.h:20: typedef base::ScopedFD
EndPoint;
Do you plan on adding more to BrokerChannel later to interact with EndPoints?  I
kind of feel like I would just use base::ScopedFD directly, but I don't feel
strongly either way.

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
sandbox/linux/syscall_broker/broker_channel.h:21: static void GetPair(EndPoint*
reader, EndPoint* writer);
Nit: I'd name it "MakePair" or "CreatePair" or something; "GetPair" seems like
it should be accessing an existing channel.

(Incidentally, there's CreateSocketPair() in
base/posix/unix_domain_socket_linux.cc that is almost exactly this; but
base/OWNERS keep killing my enthusiasm for trying to improve base/, so I've
never gotten around to making it a public API.)

[jln (very slow on Chromium), 2014-11-11 05:39:05]

Thanks!

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
File sandbox/linux/syscall_broker/broker_channel.h (right):

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
sandbox/linux/syscall_broker/broker_channel.h:20: typedef base::ScopedFD
EndPoint;
On 2014/11/11 05:16:13, mdempsky (Tokyo until Nov 14) wrote:
> Do you plan on adding more to BrokerChannel later to interact with EndPoints? 
I
> kind of feel like I would just use base::ScopedFD directly, but I don't feel
> strongly either way.

At first I wanted to make EndPoint its own class, because I wanted type checking
to enforce these file descriptors being the proper type of socket.

In the end I stopped halfway, which I think is a reasonable compromise: I expect
the name itself will be enough to convince users to rely on CreatePair().

https://codereview.chromium.org/717673002/diff/1/sandbox/linux/syscall_broker...
sandbox/linux/syscall_broker/broker_channel.h:21: static void GetPair(EndPoint*
reader, EndPoint* writer);
On 2014/11/11 05:16:13, mdempsky (Tokyo until Nov 14) wrote:
> Nit: I'd name it "MakePair" or "CreatePair" or something; "GetPair" seems like
> it should be accessing an existing channel.

Done.

> (Incidentally, there's CreateSocketPair() in
> base/posix/unix_domain_socket_linux.cc that is almost exactly this; but
> base/OWNERS keep killing my enthusiasm for trying to improve base/, so I've
> never gotten around to making it a public API.)

Yeah, I'm feeling the same. I think we could make a good case to be base/posix
owners at the very least though. We should talk about this, because clearly
there is room to improve base/ and get some stuff out of sandbox/

[jln (very slow on Chromium), 2014-11-11 05:39:13]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-11-11 05:40:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717673002/20001

[commit-bot: I haz the power, 2014-11-11 07:44:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-11-11 07:45:38]

Patchset 2 (id:??) landed as
https://crrev.com/878dca3cca9b96886ddb4233adb5c7fca779fbcc
Cr-Commit-Position: refs/heads/master@{#303602}