Forwarder 2 implementation

tools/android/forwarder2/socket.h

Index: tools/android/forwarder2/socket.h
diff --git a/tools/android/forwarder2/socket.h b/tools/android/forwarder2/socket.h
new file mode 100644
index 0000000000000000000000000000000000000000..cc6af2ade263015a73404455ef93385713a9e9ac
--- /dev/null
+++ b/tools/android/forwarder2/socket.h
@@ -0,0 +1,133 @@
+// Copyright (c) 2012 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.
+
+#ifndef TOOLS_ANDROID_FORWARDER2_SOCKET_H_
+#define TOOLS_ANDROID_FORWARDER2_SOCKET_H_
+
+#include <fcntl.h>
+#include <netinet/in.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
+#include <string>
+
+#include "base/basictypes.h"
+
+using std::string;

[bulach, 2012/09/07 08:30:28]

nit: this is uncommon, we normally just have "std::string" everywhere..

[tfarina, 2012/09/07 11:39:39]

specially in a HEADER file! ;)

[felipeg, 2012/09/07 13:19:26]

Done.

[felipeg, 2012/09/07 13:19:26]

Done.

+
+namespace forwarder {

[bulach, 2012/09/07 08:30:28]

nit: I guess it'd be clearer as forwarder2

[felipeg, 2012/09/07 13:19:26]

Done.

+
+// Wrapper class around unix socket api.  Can be used to create, bind or
+// connect to both Unix domain sockets and TCP sockets.
+class Socket {
+ public:
+  Socket();
+  ~Socket();
+
+  bool BindUnix(const string& path, bool abstract);
+  bool BindTcp(const string& host, int port);
+  bool ConnectUnix(const string& path, bool abstract);
+  bool ConnectTcp(const string& host, int port);
+
+  // Just a wrapper around unix socket shutdown(), see man 2 shutdown.
+  void Shutdown();
+
+  // Just a wrapper around unix socket close(), see man 2 close.
+  void Close();
+  bool IsClosed() const { return socket_ < 0; }
+
+  bool Accept(Socket* new_socket);
+
+  bool IsFdInSet(const fd_set& fds) const;
+  bool AddFdToSet(fd_set* fds) const;
+
+  // Just a wrapper around unix read() function.
+  // Reads up to buffer_size, but may read less then buffer_size.
+  // Returns the number of bytes read.
+  int Read(char* buffer, size_t buffer_size);
+
+  // Same as Read(), just a wrapper around write().
+  int Write(const char* buffer, size_t count);
+
+  // Calls Read() multiple times until num_bytes is written to the provided
+  // buffer. No bounds checking is performed.
+  // Returns number of bytes read, which can be different from num_bytes in case
+  // of errror.
+  int ReadNumBytes(char* buffer, size_t num_bytes);
+
+  // Calls Write() multiple times until num_bytes is written. No bounds checking
+  // is performed. Returns number of bytes written, which can be different from
+  // num_bytes in case of errror.
+  int WriteNumBytes(const char* buffer, size_t num_bytes);
+
+  // Calls WriteNumBytes for the given string.
+  int WriteString(const string& buffer) {
+    return WriteNumBytes(buffer.c_str(), buffer.size());

[bulach, 2012/09/07 08:30:28]

nit: shouldn't be inlined, move to the .cc file..

[felipeg, 2012/09/07 13:19:26]

Done.

+  }
+
+  static int GetHighestFileDescriptor(const Socket& s1, const Socket& s2);

[bulach, 2012/09/07 08:30:28]

nit: move this one to 80

[felipeg, 2012/09/07 13:19:26]

Done.

+
+  bool HasError() const { return socket_error_; }

[bulach, 2012/09/07 08:30:28]

nit: "has_error", otherwise it can't be inlined..

[felipeg, 2012/09/07 13:19:26]

Done.

[digit, 2012/09/07 13:47:12]

The style guide says that lower_case() method should match the internal member
field, which isn't the case here. Also, why can't a simple function be inlined
even if it uses CamelCase()? I'm not aware of such rule.

[tfarina, 2012/09/07 13:50:46]

Like digit pointed out. I think it's fine as is. This is a trivial getter
accessor. I don't think renaming to socket_error() will help much. HasError()
seems to be the best option here.

[felipeg, 2012/09/07 13:56:57]

Done.

[felipeg, 2012/09/07 13:56:57]

Done.

[bulach, 2012/09/07 14:03:32]

it's a recommendation from:
http://dev.chromium.org/developers/coding-style
Google style strongly encourages (but does not mandate) unix_hacker style on
member functions that do so little work they're effectively free, such as
simple, non-virtual getters and setters. This is also the team position. Some
code uses CamelCase for everything; avoid writing patches just to convert these
functions to unix_hacker style, but try to use it in new code you write unless
there are encapsulation reasons why CamelCase would be much more appropriate.

+
+  // |notifier_fd| must be a valid pipe file descriptor created from the
+  // PipeNotifier and must live (not be closed) at least as long as this socket
+  // is alive.
+  void set_notifier_fd(int notifier_fd) { exit_notifier_fd_ = notifier_fd; }

[digit, 2012/09/07 13:47:12]

nit: this doesn't match the internal field name.

[felipeg, 2012/09/07 13:56:57]

Done.

+  // Unset the |exit_notifier_fd_| so that it will not receive notifications
+  // anymore.
+  void reset_notifier_fd() { exit_notifier_fd_ = -1; }
+
+ private:
+  // If |host| is empty, use localhost.
+  bool InitTcpSocket(const string& host, int port);
+  bool InitUnixSocket(const string& path, bool abstract);
+  bool BindAndListen();
+  bool Connect();
+
+  bool Resolve(const string& host);
+  bool InitSocketInternal();
+  void SetSocketError();
+
+  // Waits until either the Socket or the |exit_notifier_fd_| has received a
+  // read event (accept or read). Returns false iff an exit notification was
+  // received.
+  bool WaitForEvent() const;
+
+  int socket_;
+  int port_;
+  bool socket_error_;
+
+  // Family of the socket (IF_INET, IF_INET6 or PF_UNIX).
+  int family_;
+
+  // True if this is an abstract unix domain socket.
+  bool abstract_;
+
+  typedef union {

[tfarina, 2012/09/07 11:39:39]

Also move it to the beginning of private section. before the function
declarations.

[tfarina, 2012/09/07 11:39:39]

does this needs to be typedefed? this is c++ land, not C.

[felipeg, 2012/09/07 13:19:26]

Done.

[felipeg, 2012/09/07 13:19:26]

Done.

+    // IPv4 sockaddr
+    sockaddr_in addr4;
+    // IPv6 sockaddr
+    sockaddr_in6 addr6;
+    // Unix Domain sockaddr
+    sockaddr_un addr_un;
+  } SockAddr;
+
+  SockAddr addr_;
+
+  // Points to one of the members of the above union depending on the family.
+  sockaddr* addr_ptr_;
+  // Length of one of the members of the above union depending on the family.
+  socklen_t addr_len_;
+
+  // File descriptor from PipeNotifier (see pipe_notifier.h) to send application
+  // exit notifications before calling socket blocking operations such as Read
+  // and Accept.
+  int exit_notifier_fd_;
+
+  DISALLOW_COPY_AND_ASSIGN(Socket);
+};
+
+}  // namespace forwarder
+
+#endif  // TOOLS_ANDROID_FORWARDER2_SOCKET_H_