Implement IPC::ChannelFactory, a class that accept()s on a UNIX socket.

ipc/ipc_channel_factory.cc

Index: ipc/ipc_channel_factory.cc
diff --git a/ipc/ipc_channel_factory.cc b/ipc/ipc_channel_factory.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4041e874d48dc4128f55364e58c3dc426c447ad0
--- /dev/null
+++ b/ipc/ipc_channel_factory.cc
@@ -0,0 +1,92 @@
+// Copyright 2013 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.
+
+#include "ipc/ipc_channel_factory.h"
+
+#include <errno.h>

[Mark Mentovai, 2013/03/05 20:08:03]

I’m surprised to see this many #includes for a file that’s so short. For
example, I don’t see any uses of errno or the error constants in this file, but
here <errno.h> is. I don’t see any constants, types, or functions from fcntl.h,
but here’s <fcntl.h> (close doesn’t count, it comes from <unistd.h>). Can you
clean this up?

[jeremya, 2013/03/06 05:03:18]

Yeah, I think I must have just copied this in along with the unix domain socket
stuff before I factored it out. Turns out none of these <headers> were needed.

+#include <fcntl.h>
+#include <stddef.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <unistd.h>
+
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "ipc/ipc_channel_posix.h"
+#include "ipc/unix_domain_socket_util.h"
+
+namespace IPC {
+
+ChannelFactory::ChannelFactory(const base::FilePath& path, Delegate* delegate)
+    : path_(path), delegate_(delegate), listen_pipe_(-1) {
+  DCHECK(delegate_);
+  CreatePipe();
+}
+
+ChannelFactory::~ChannelFactory() {
+  Close();
+}
+
+bool ChannelFactory::CreatePipe() {
+  DCHECK(listen_pipe_ < 0);
+
+  // Create the socket.
+  return CreateServerUnixDomainSocket(path_, &listen_pipe_);
+}
+
+bool ChannelFactory::Listen() {
+  if (listen_pipe_ < 0)
+    return false;
+  // Watch the pipe for connections, and turn any connections into

[Mark Mentovai, 2013/03/05 20:08:03]

I like to see a blank line before comments like this one (that aren’t at the
beginning of a new {scope}), because otherwise the comment tends to visually
blend in with the surrounding code. Same on line 64.

[jeremya, 2013/03/06 05:03:18]

Done.

+  // active sockets.
+  MessageLoopForIO::current()->WatchFileDescriptor(
+      listen_pipe_,
+      true,
+      MessageLoopForIO::WATCH_READ,
+      &server_listen_connection_watcher_,
+      this);
+  return true;
+}
+
+// Called by libevent when we can read from the pipe without blocking.
+void ChannelFactory::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK(fd == listen_pipe_);
+  int new_pipe = -1;
+  if (!ServerAcceptConnection(listen_pipe_, &new_pipe)) {
+    Close();

[Mark Mentovai, 2013/03/05 20:08:03]

I’m not sure that Close() is appropriate here. What if accept() returns
ECONNABORTED? This might happen if a client attempts to connect and then gives
up before you have a chance to call accept(). You don’t want to just throw the
whole server away when that happens, right?

[jeremya, 2013/03/06 05:03:18]

Yeah, you're right. I've changed the logic so that ServerAcceptConnection() only
returns false if an unrecoverable error occurs, and this function takes the
signal new_pipe >= 0 to indicate that a socket was successfully opened.

Where ipc_channel_posix uses this I just check for both failure conditions.

+    delegate_->OnListenError();
+    return;
+  }
+
+  // Verify that the IPC channel peer is running as the same user.
+  uid_t client_euid;
+  if (!GetPeerEuid(new_pipe, &client_euid)) {
+    close(new_pipe);

[Mark Mentovai, 2013/03/05 20:08:03]

Close does HANDLE_EINTR around its close. This close and the one on line 72 are
bare. Be consistent.

[jeremya, 2013/03/06 05:03:18]

file_util::ScopedFD to the rescue.

+    return;
+  }
+  if (client_euid != geteuid()) {
+    DLOG(ERROR) << "Client euid is not authorised";
+    close(new_pipe);
+    return;
+  }
+
+  ChannelHandle handle("", base::FileDescriptor(new_pipe, true));
+  delegate_->OnClientConnected(handle);
+}
+
+void ChannelFactory::Close() {
+  if (listen_pipe_ < 0)
+    return;
+  if (HANDLE_EINTR(close(listen_pipe_)) < 0)
+    PLOG(ERROR) << "close";
+  if (unlink(path_.value().c_str()) < 0)
+    PLOG(ERROR) << "unlink";
+  listen_pipe_ = -1;
+  // Unregister libevent for the listening socket and close it.
+  server_listen_connection_watcher_.StopWatchingFileDescriptor();
+}
+
+}  // namespace IPC