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

ipc/ipc_channel_factory.cc

+// 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>
+#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), must_unlink_(false) {
+  DCHECK(delegate_);
+  if (!CreatePipe()) {
+    // The pipe may have been closed already.
+    LOG(WARNING) << "Unable to create pipe named \"" << path.value() << "\"";

[Mark Mentovai, 2013/02/28 04:57:56]

Almost every failure case in CreatePipe has already logged something. If you fix
the last case that doesn’t log something, this line becomes extraneous.

[jeremya, 2013/02/28 05:58:18]

Not quite true, I had to add a few extra logs, but I did that and removed this
log.

+  }
+}
+
+ChannelFactory::~ChannelFactory() {
+  Close();
+}
+
+bool ChannelFactory::CreatePipe() {
+  DCHECK(listen_pipe_ == -1);
+
+  int local_pipe = -1;

[Mark Mentovai, 2013/02/28 04:57:56]

Why do you need local_pipe when you can just use &listen_pipe_?

[jeremya, 2013/02/28 05:58:18]

Good question. Fixed.

+  // Create the socket.
+  if (!CreateServerUnixDomainSocket(path_, &local_pipe))
+    return false;
+  listen_pipe_ = local_pipe;
+  must_unlink_ = true;

[Mark Mentovai, 2013/02/28 04:57:56]

Why do yo need a separate variable for this? Isn’t it the same as (listen_pipe_
>= 0)?

[jeremya, 2013/02/28 05:58:18]

Yep, but now that I look at it there's also a case I missed where bind() worked
but listen() failed where the socket would stick around. Which I have fixed.

+  return true;
+}
+
+bool ChannelFactory::Listen() {
+  if (listen_pipe_ == -1) {
+    DLOG(INFO) << "Factory creation failed: " << path_.value();

[Mark Mentovai, 2013/02/28 04:57:56]

You’ve got lots of LOG(WARNING)s and LOG(ERROR)s, and then you have this case,
which is a DLOG (not LOG) and INFO (not something more severe). If you can only
reach this after something else that already executed logged something with LOG
(not DLOG) and at a higher severity, what’s the point of this added noise?

What’s your logging philosophy? I can’t discern any rhyme or reason.

Personally, I prefer less logging, and when there is logging, for there to be
less of a message (the file and line information is usually more than enough).

[jeremya, 2013/02/28 05:58:18]

... philosophy? :) A lot of this code was copied in part from other places and
then modified. This particular function is analogous to ipc_channel_posix.cc
Channel::ChannelImpl::Connect(), which does a DLOG(INFO) in the failure case.
The extra log could be helpful when debugging to see what it was that was
causing the earlier log messages, but perhaps not useful enough.
Fair enough. I've removed the message.

+    return false;
+  }
+  // Watch the pipe for connections, and turn any connections into
+  // 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 = 0;

[Mark Mentovai, 2013/02/28 04:57:56]

You probably wanted to initialize this to -1.

[jeremya, 2013/02/28 05:58:18]

Done.

+  if (!ServerAcceptConnection(listen_pipe_, &new_pipe)) {
+    Close();
+    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)) {
+    DLOG(ERROR) << "Unable to query client euid";

[Mark Mentovai, 2013/02/28 04:57:56]

All of the failure cases have already logged something.

[jeremya, 2013/02/28 05:58:18]

Removed.

+    // TODO close new pipe
+    return;
+  }
+  if (client_euid != geteuid()) {
+    DLOG(WARNING) << "Client euid is not authorised";
+    // TODO close new pipe

[Mark Mentovai, 2013/02/28 04:57:56]

Yup, do these TODOs.

[jeremya, 2013/02/28 05:58:18]

Done.

+    return;
+  }
+
+  ChannelHandle handle("", base::FileDescriptor(new_pipe, true));
+  delegate_->OnClientConnected(handle);
+}
+
+void ChannelFactory::Close() {
+  if (must_unlink_) {
+    unlink(path_.value().c_str());
+    must_unlink_ = false;
+  }
+  if (listen_pipe_ != -1) {
+    if (HANDLE_EINTR(close(listen_pipe_)) < 0)
+      DPLOG(ERROR) << "close " << listen_pipe_;

[Mark Mentovai, 2013/02/28 04:57:56]

Logging the FD number is probably never useful.

[jeremya, 2013/02/28 05:58:18]

Done.

+    listen_pipe_ = -1;
+    // Unregister libevent for the listening socket and close it.
+    server_listen_connection_watcher_.StopWatchingFileDescriptor();
+  }
+}
+
+}

[Mark Mentovai, 2013/02/28 04:57:56]

}  // namespace IPC

[jeremya, 2013/02/28 05:58:18]

Done.