Refactor unix domain socket.

net/socket/unix_domain_client_socket_posix_unittest.cc

Index: net/socket/unix_domain_client_socket_posix_unittest.cc
diff --git a/net/socket/unix_domain_client_socket_posix_unittest.cc b/net/socket/unix_domain_client_socket_posix_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..62c0518c83222745151f1e92ea17ca9dd458e30f
--- /dev/null
+++ b/net/socket/unix_domain_client_socket_posix_unittest.cc
@@ -0,0 +1,226 @@
+// Copyright 2014 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 "net/socket/unix_domain_client_socket_posix.h"
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/socket/unix_domain_server_socket_posix.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+const char kSocketFilename[] = "unix_domain_socket_for_testing";
+
+bool UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
+  return allow_user;
+}
+
+UnixDomainServerSocket::AuthCallback CreateAuthCallback(bool allow_user) {
+  return base::Bind(&UserCanConnectCallback, allow_user);
+}
+
+void CallbackWithExpectedResultValue(int expected_rv, int rv) {
+  EXPECT_EQ(expected_rv, rv);
+}
+
+CompletionCallback GetCompletionCallback(int expected_rv) {
+  return base::Bind(&CallbackWithExpectedResultValue, expected_rv);
+}
+
+}  // namespace
+
+class UnixDomainClientSocketTest : public testing::Test {
+ protected:
+  virtual void SetUp() OVERRIDE {

[mmenke, 2014/07/11 20:44:03]

Can just do this in the constructor / destructor (Every test iteration gets its
own test fixture object).

[byungchul, 2014/07/14 17:49:22]

Done.

+    // Builds a socket path under a temp directory created newly.
+    base::FilePath temp_dir;
+    base::CreateNewTempDirectory("", &temp_dir);
+    socket_path_ = temp_dir.Append(kSocketFilename).value();

[mmenke, 2014/07/11 20:44:04]

Suggest you use base/files/scoped_temp_dir.h.  Sorry for not suggesting this
earlier, forgot about the class.  Same goes for other files.

[byungchul, 2014/07/14 17:49:22]

Done. Thank you for good suggestion.

+  }
+
+  virtual void TearDown() OVERRIDE {
+    // Deletes the temp dir created by SetUp().
+    base::DeleteFile(base::FilePath(socket_path_).DirName(), true);
+  }
+
+  std::string socket_path_;
+};
+
+TEST_F(UnixDomainClientSocketTest, Connect) {
+  const bool kUseAbstractNamespace = false;
+
+  UnixDomainServerSocket server_socket(CreateAuthCallback(true),
+                                       kUseAbstractNamespace);
+  EXPECT_EQ(OK, server_socket.ListenWithAddressAndPort(socket_path_, 0, 1));
+
+  scoped_ptr<StreamSocket> accepted_socket;
+  EXPECT_EQ(ERR_IO_PENDING, server_socket.Accept(&accepted_socket,
+                                                 GetCompletionCallback(OK)));
+  EXPECT_TRUE(!accepted_socket);

[mmenke, 2014/07/11 20:44:04]

nit:  EXPECT_TRUE(!...) is the same as EXPECT_FALSE(...)

[byungchul, 2014/07/14 17:49:23]

Done.

+
+  UnixDomainClientSocket client_socket(socket_path_, kUseAbstractNamespace);
+  EXPECT_FALSE(client_socket.IsConnected());
+
+  EXPECT_EQ(OK, client_socket.Connect(GetCompletionCallback(OK)));

[mmenke, 2014/07/11 20:44:03]

Why is this synchronous?  The server hasn't even called the auth callback yet,
which I would have thought would give us a connection error.

[byungchul, 2014/07/14 17:49:22]

It might be because kernel accepts the connection. Added a comment.

+  EXPECT_TRUE(client_socket.IsConnected());
+  // Not yet notified to server.

[mmenke, 2014/07/11 20:44:02]

Maybe:  "Server has not yet been notified of the connection."

[byungchul, 2014/07/14 17:49:22]

Done.

+  EXPECT_TRUE(!accepted_socket);
+
+  base::RunLoop().RunUntilIdle();

[mmenke, 2014/07/11 20:44:04]

Is the connect message guaranteed to already be queued at this point?

[byungchul, 2014/07/14 17:49:23]

They are unix domain sockets in same process and same thread, and I think yes
because kernel always has the event to deliver (or read) for every connect() or
write().

+  EXPECT_TRUE(accepted_socket);
+  EXPECT_TRUE(accepted_socket->IsConnected());

[mmenke, 2014/07/11 20:44:04]

Maybe disconnect and then check IsConnected?

[byungchul, 2014/07/14 17:49:22]

Done in 2 other unittests disconnecting from each side.

+}
+
+TEST_F(UnixDomainClientSocketTest, ConnectWithAbstractNamespace) {
+  const bool kUseAbstractNamespace = true;
+
+  UnixDomainClientSocket client_socket(socket_path_, kUseAbstractNamespace);
+  EXPECT_FALSE(client_socket.IsConnected());
+
+#if defined(OS_ANDROID) || defined(OS_LINUX)
+  UnixDomainServerSocket server_socket(CreateAuthCallback(true),
+                                       kUseAbstractNamespace);
+  EXPECT_EQ(OK, server_socket.ListenWithAddressAndPort(socket_path_, 0, 1));
+
+  scoped_ptr<StreamSocket> accepted_socket;
+  EXPECT_EQ(ERR_IO_PENDING, server_socket.Accept(&accepted_socket,
+                                                 GetCompletionCallback(OK)));
+  EXPECT_TRUE(!accepted_socket);
+
+  EXPECT_EQ(OK, client_socket.Connect(GetCompletionCallback(OK)));
+  EXPECT_TRUE(client_socket.IsConnected());
+  // Not yet notified to server.
+  EXPECT_TRUE(!accepted_socket);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(accepted_socket);
+  EXPECT_TRUE(accepted_socket->IsConnected());
+#else
+  EXPECT_EQ(ERR_ADDRESS_INVALID,
+            client_socket.Connect(GetCompletionCallback(OK)));
+#endif
+}
+
+TEST_F(UnixDomainClientSocketTest, ConnectToNonExistingSocket) {

[mmenke, 2014/07/11 20:44:03]

ConnectToNonExistentServer, maybe?  (And same for next test)

[byungchul, 2014/07/14 17:49:22]

Done.

+  const bool kUseAbstractNamespace = false;
+
+  UnixDomainClientSocket client_socket(socket_path_, kUseAbstractNamespace);
+  EXPECT_FALSE(client_socket.IsConnected());
+  EXPECT_EQ(ERR_FILE_NOT_FOUND,
+            client_socket.Connect(GetCompletionCallback(OK)));
+}
+
+TEST_F(UnixDomainClientSocketTest,
+       ConnectToNonExistingSocketWithAbstractNamespace) {
+  const bool kUseAbstractNamespace = true;
+
+  UnixDomainClientSocket client_socket(socket_path_, kUseAbstractNamespace);
+  EXPECT_FALSE(client_socket.IsConnected());
+#if defined(OS_ANDROID) || defined(OS_LINUX)
+  EXPECT_EQ(ERR_CONNECTION_REFUSED,
+            client_socket.Connect(GetCompletionCallback(OK)));
+#else
+  EXPECT_EQ(ERR_ADDRESS_INVALID,
+            client_socket.Connect(GetCompletionCallback(OK)));
+#endif
+}
+
+TEST_F(UnixDomainClientSocketTest, ReadWrite) {
+  UnixDomainServerSocket server_socket(CreateAuthCallback(true), false);
+  EXPECT_EQ(OK, server_socket.ListenWithAddressAndPort(socket_path_, 0, 1));
+  scoped_ptr<StreamSocket> accepted_socket;
+  EXPECT_EQ(ERR_IO_PENDING, server_socket.Accept(&accepted_socket,
+                                                 GetCompletionCallback(OK)));
+  UnixDomainClientSocket client_socket(socket_path_, false);
+  EXPECT_EQ(OK, client_socket.Connect(GetCompletionCallback(OK)));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(accepted_socket->IsConnected());
+  EXPECT_TRUE(client_socket.IsConnected());
+
+  // Sends data from client to server.

[mmenke, 2014/07/11 20:44:02]

nit:  I believe imperatives ("Send data from client to server") are a lot more
common in comments in the code itself.

[byungchul, 2014/07/14 17:49:22]

Done.

+  const int write_data_size = 10;
+  scoped_refptr<IOBuffer> write_buffer(
+      new StringIOBuffer(std::string(write_data_size, 'd')));
+  EXPECT_EQ(write_data_size,
+            client_socket.Write(write_buffer, write_data_size,
+                                GetCompletionCallback(OK)));

[mmenke, 2014/07/11 20:44:03]

Are we guaranteed the entire buffer will be written synchronously in one write?

[byungchul, 2014/07/14 17:49:22]

Default socket write buffer must be much greater than 10. I am pretty sure it
will be written fully.

+  // Buffer bigger than write data size.

[mmenke, 2014/07/11 20:44:04]

Per google style guide, comments should generally be sentences ("The buffer is
bigger than...")

[byungchul, 2014/07/14 17:49:22]

Done.

+  const int read_buffer_size = write_data_size * 2;
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(read_buffer_size));
+  EXPECT_EQ(write_data_size,
+            accepted_socket->Read(read_buffer, read_buffer_size,
+                                  GetCompletionCallback(OK)));

[mmenke, 2014/07/11 20:44:03]

Should compare buffer contents here, and below, out of paranoia.

[mmenke, 2014/07/11 20:44:04]

Again, are we guaranteed the entire thing will be read in one synchronous read?

[byungchul, 2014/07/14 17:49:22]

I think so, because they are running in same process and thread, and socket
read/write buffer are much bigger than 10.

[byungchul, 2014/07/14 17:49:22]

Done.

+
+  // Sends data from server and client

[mmenke, 2014/07/11 20:44:02]

nit:  period

[byungchul, 2014/07/14 17:49:23]

Done.

+  EXPECT_EQ(write_data_size,
+            accepted_socket->Write(write_buffer, write_data_size,
+                                   GetCompletionCallback(OK)));
+  // Reads multiple times.
+  const int small_read_buffer_size = write_data_size / 3;
+  EXPECT_EQ(small_read_buffer_size,
+            client_socket.Read(read_buffer, small_read_buffer_size,
+                               GetCompletionCallback(OK)));
+  EXPECT_EQ(write_data_size - small_read_buffer_size,
+            client_socket.Read(read_buffer, read_buffer_size,
+                               GetCompletionCallback(OK)));
+  // No more data.
+  EXPECT_EQ(ERR_IO_PENDING,
+            client_socket.Read(read_buffer, read_buffer_size,
+                               GetCompletionCallback(ERR_CONNECTION_CLOSED)));

[mmenke, 2014/07/11 20:44:02]

Should we wait to get the close message from the server?

[byungchul, 2014/07/14 17:49:23]

Done.

+}
+
+TEST_F(UnixDomainClientSocketTest, ReadWriteAsynchronously) {

[mmenke, 2014/07/11 20:44:03]

There are no asyncronous writes here.  Maybe ReadBeforeWrite?

[byungchul, 2014/07/14 17:49:22]

Done.

+  UnixDomainServerSocket server_socket(CreateAuthCallback(true), false);
+  EXPECT_EQ(OK, server_socket.ListenWithAddressAndPort(socket_path_, 0, 1));
+  scoped_ptr<StreamSocket> accepted_socket;
+  EXPECT_EQ(ERR_IO_PENDING, server_socket.Accept(&accepted_socket,
+                                                 GetCompletionCallback(OK)));
+  UnixDomainClientSocket client_socket(socket_path_, false);
+  EXPECT_EQ(OK, client_socket.Connect(GetCompletionCallback(OK)));
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(accepted_socket->IsConnected());
+  EXPECT_TRUE(client_socket.IsConnected());
+
+  // Waits for data from client.
+  const int write_data_size = 10;
+  const int read_buffer_size = write_data_size * 2;
+  const int small_read_buffer_size = write_data_size / 3;
+  // Reads smaller than write data size first.
+  scoped_refptr<IOBuffer> read_buffer(new IOBuffer(read_buffer_size));
+  EXPECT_EQ(
+      ERR_IO_PENDING,
+      accepted_socket->Read(read_buffer, small_read_buffer_size,
+                            GetCompletionCallback(small_read_buffer_size)));
+
+  scoped_refptr<IOBuffer> write_buffer(
+      new StringIOBuffer(std::string(write_data_size, 'd')));
+  EXPECT_EQ(write_data_size,
+            client_socket.Write(write_buffer, write_data_size,
+                                GetCompletionCallback(OK)));
+
+  base::RunLoop().RunUntilIdle();
+  // First read completed.
+
+  // Reads remaining data.
+  EXPECT_EQ(write_data_size - small_read_buffer_size,
+            accepted_socket->Read(read_buffer, read_buffer_size,
+                                  GetCompletionCallback(OK)));
+  // No more data.
+  EXPECT_EQ(ERR_IO_PENDING,
+            accepted_socket->Read(
+                read_buffer, read_buffer_size,
+                GetCompletionCallback(ERR_CONNECTION_CLOSED)));
+}
+
+}  // namespace net