First cut at POSIX Implementation of IPC Channel using FIFOs.

First cut at POSIX Implementation of IPC Channel using FIFOs.
    
    The following work still remains:
    * Provide better encapsulation of libevent.
    * Use socketpair and use named FIFO code only when debugging.

[jeremy, 2008-12-04 00:33:18]

I'm splitting this change into a few smaller CLs, this one only includes the
IPC::Channel implementation.
Changes to MessageLoopForIO and ipc_test will follow shortly.

[agl, 2008-12-04 01:46:16]

Only half done; have to rush. Will do the rest tonight/tomorrow.

http://codereview.chromium.org/12927/diff/9/11
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/12927/diff/9/11#newcode8
Line 8: #include	<sys/types.h>
Whitespace malfunction.

http://codereview.chromium.org/12927/diff/9/11#newcode30
Line 30: // The -1 is to take the NULl terminator into account.
s/NULl/NULL

http://codereview.chromium.org/12927/diff/9/11#newcode31
Line 31: const size_t kMaxPipeNameLength = 104 - 1;
This should be UNIX_PATH_MAX to be portable

http://codereview.chromium.org/12927/diff/9/11#newcode44
Line 44: int fd = socket(AF_UNIX, SOCK_STREAM, 0);
whitespace

http://codereview.chromium.org/12927/diff/9/11#newcode46
Line 46: return false;
Whitespace, I'll stop here - it's all funky.

http://codereview.chromium.org/12927/diff/9/11#newcode64
Line 64: unix_addr.sun_len = unix_addr_len;
Is this a Macism? I don't see sun_len defined in man 7 unix

http://codereview.chromium.org/12927/diff/9/11#newcode68
Line 68: unix_addr_len) != 0) {
This is incorrect on Linux, unix_addr_len should be sizeof(struct sockaddr_un)

http://codereview.chromium.org/12927/diff/9/11#newcode69
Line 69: return false;
You need to close the socket here

http://codereview.chromium.org/12927/diff/9/11#newcode75
Line 75: return false;
You need to close the socket here.

http://codereview.chromium.org/12927/diff/9/11#newcode90
Line 90: reinterpret_cast<sockaddr*>(&unix_addr),
Since you don't do anything with the address, why not pass NULL?

http://codereview.chromium.org/12927/diff/9/11#newcode123
Line 123: server_unix_addr.sun_len = server_unix_addr_len;
Again, I think sun_len is a Macism

http://codereview.chromium.org/12927/diff/9/11#newcode126
Line 126: server_unix_addr_len) != 0) {
Again, sizeof(struct sockaddr_un) on Linux.

http://codereview.chromium.org/12927/diff/9/11#newcode127
Line 127: return false;
Need to close the socket

[agl, 2008-12-04 18:20:55]

http://codereview.chromium.org/12927/diff/9/11
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/12927/diff/9/11#newcode125
Line 125: if (connect(fd, reinterpret_cast<sockaddr*>(&server_unix_addr),
Should be able to cope with signals:

int r;
do {
  r = connect(...)
} while (r == -1 && errno == EINTR);

if (r == -1) {
  ...

http://codereview.chromium.org/12927/diff/9/11#newcode147
Line 147: memset(&event, 0, sizeof(event));
It's not clear why you need to do this. libevent's set functions will set all
the needed members of the struct.

http://codereview.chromium.org/12927/diff/9/11#newcode171
Line 171: "\" in " << (mode == 0 ? "server" : "client") << " mode.";
strerror(errno) might be useful information too.

http://codereview.chromium.org/12927/diff/9/11#newcode219
Line 219: event *ev = &(server_listen_connection_event_->event);
The extra ( ) is a little odd, but no big deal.

http://codereview.chromium.org/12927/diff/9/11#newcode254
Line 254: bytes_read = recv(pipe_, input_buf_, BUF_SIZE, 0);
Signal hygiene:

do {
  bytes_read = recv(..)
} while (bytes_read == -1 && errno == EINTR)

http://codereview.chromium.org/12927/diff/9/11#newcode254
Line 254: bytes_read = recv(pipe_, input_buf_, BUF_SIZE, 0);
In this case, read() is probably better since you don't use any recv specific
arguments

http://codereview.chromium.org/12927/diff/9/11#newcode259
Line 259: LOG(ERROR) << "pipe error: " << errno;
strerror(errno)

http://codereview.chromium.org/12927/diff/9/11#newcode271
Line 271: const char* p, *end;
const char *p, *end;
  or
const char* p;
const char* end;

http://codereview.chromium.org/12927/diff/9/11#newcode333
Line 333: write_event_->ClearEvent();
again, not sure why we need to do this.

http://codereview.chromium.org/12927/diff/9/11#newcode345
Line 345: int bytes_written = send(pipe_, out_bytes, amt_to_write, 0);
ssize_t bytes_written, and signal hygiene

http://codereview.chromium.org/12927/diff/9/11#newcode345
Line 345: int bytes_written = send(pipe_, out_bytes, amt_to_write, 0);
When not using any send specific arguments, write() is probably better.

http://codereview.chromium.org/12927/diff/9/11#newcode391
Line 391: if (!waiting_connect_) {
Maybe use a single if with &&?

[jeremy, 2008-12-04 22:31:40]

Adam: all fixed, ready for another look...

http://codereview.chromium.org/12927/diff/9/11
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/12927/diff/9/11#newcode8
Line 8: #include	<sys/types.h>
On 2008/12/04 01:46:16, agl wrote:
> Whitespace malfunction.

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode30
Line 30: // The -1 is to take the NULl terminator into account.
On 2008/12/04 01:46:16, agl wrote:
> s/NULl/NULL

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode31
Line 31: const size_t kMaxPipeNameLength = 104 - 1;
UNIX_PATH_MAX isn't defined on OS X, added #ifdef.

http://codereview.chromium.org/12927/diff/9/11#newcode44
Line 44: int fd = socket(AF_UNIX, SOCK_STREAM, 0);
On 2008/12/04 01:46:16, agl wrote:
> whitespace

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode46
Line 46: return false;
I had some tabs in the code, hopefully they should all be nuked now, sorry about
that...

http://codereview.chromium.org/12927/diff/9/11#newcode64
Line 64: unix_addr.sun_len = unix_addr_len;
BSDism maybe :|
Advanced Programming in the Unix Environemnt (APUE) doesn't fill this in so I've
removed this definition.

http://codereview.chromium.org/12927/diff/9/11#newcode68
Line 68: unix_addr_len) != 0) {
Just mimicking APUE here, lets hold off on changing this unless it causes the
unit tests to fail on Linux.

http://codereview.chromium.org/12927/diff/9/11#newcode69
Line 69: return false;
On 2008/12/04 01:46:16, agl wrote:
> You need to close the socket here

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode75
Line 75: return false;
On 2008/12/04 01:46:16, agl wrote:
> You need to close the socket here.

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode90
Line 90: reinterpret_cast<sockaddr*>(&unix_addr),
done.

http://codereview.chromium.org/12927/diff/9/11#newcode123
Line 123: server_unix_addr.sun_len = server_unix_addr_len;
On 2008/12/04 01:46:16, agl wrote:
> Again, I think sun_len is a Macism

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode125
Line 125: if (connect(fd, reinterpret_cast<sockaddr*>(&server_unix_addr),
Thanks, changed, are there any other calls you think I might need to wrap this
way?

http://codereview.chromium.org/12927/diff/9/11#newcode127
Line 127: return false;
On 2008/12/04 01:46:16, agl wrote:
> Need to close the socket

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode147
Line 147: memset(&event, 0, sizeof(event));
On 2008/12/04 18:20:56, agl wrote:
> It's not clear why you need to do this. libevent's set functions will set all
> the needed members of the struct.

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode171
Line 171: "\" in " << (mode == 0 ? "server" : "client") << " mode.";
On 2008/12/04 18:20:56, agl wrote:
> strerror(errno) might be useful information too.

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode254
Line 254: bytes_read = recv(pipe_, input_buf_, BUF_SIZE, 0);
On 2008/12/04 18:20:56, agl wrote:
> Signal hygiene:
> 
> do {
>   bytes_read = recv(..)
> } while (bytes_read == -1 && errno == EINTR)
> 

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode254
Line 254: bytes_read = recv(pipe_, input_buf_, BUF_SIZE, 0);
On 2008/12/04 18:20:56, agl wrote:
> Signal hygiene:
> 
> do {
>   bytes_read = recv(..)
> } while (bytes_read == -1 && errno == EINTR)
> 

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode259
Line 259: LOG(ERROR) << "pipe error: " << errno;
On 2008/12/04 18:20:56, agl wrote:
> strerror(errno)

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode271
Line 271: const char* p, *end;
On 2008/12/04 18:20:56, agl wrote:
> const char *p, *end;
>   or
> const char* p;
> const char* end;

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode333
Line 333: write_event_->ClearEvent();
On 2008/12/04 18:20:56, agl wrote:
> again, not sure why we need to do this.

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode345
Line 345: int bytes_written = send(pipe_, out_bytes, amt_to_write, 0);
On 2008/12/04 18:20:56, agl wrote:
> ssize_t bytes_written, and signal hygiene 

Done.

http://codereview.chromium.org/12927/diff/9/11#newcode391
Line 391: if (!waiting_connect_) {
Like allot of things in this code, just trying to match the Windows version as
closely as possible.

[agl, 2008-12-04 22:43:43]

There's no diff to the newest version.

[agl, 2008-12-05 00:08:24]

LGTM other than a couple of nits

http://codereview.chromium.org/12927/diff/213/16
File chrome/common/ipc_channel_posix.cc (right):

http://codereview.chromium.org/12927/diff/213/16#newcode67
Line 67: snprintf(unix_addr.sun_path, kMaxPipeNameLength, "%s",
pipe_name.c_str());
kMaxPipeNameLength + 1 here, so that it can write the NUL byte when we are at
the maximum length.

http://codereview.chromium.org/12927/diff/213/16#newcode123
Line 123: snprintf(server_unix_addr.sun_path, kMaxPipeNameLength, "%s",
+1 again

http://codereview.chromium.org/12927/diff/213/16#newcode128
Line 128: int ret_val = -1;
no point setting it to -1

[jeremy, 2008-12-05 00:16:05]

awalker: does this look ok to you?

darin: are you ok with me checking in this way and then refactoring things in a
followup cl?

[Amanda Walker, 2008-12-05 02:35:38]

LGTM with an issue opened  for refactoring in a followup if you don't plan to do
so immediately (just so we don't forget).