Refactor unix domain socket.

Refactor unix domain socket.

This is a pre-requisite for http server refactoring,
https://codereview.chromium.org/296053012/.

1) Define UnixDomainClientSocket and UnixDomainServerSocket utilizing
   SocketLibevent.
2) Rename UnixDomainSocket to UnixDomainListenSocket to reduce confusion.
3) unittests

BUG=371906
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285148

[byungchul, 2014-07-09 22:46:00]

Please review unix domain sockets based on socket libevent.

[mmenke, 2014-07-10 19:07:29]

Not quite a full review - still need to look at the unit tests, and I haven't
looked at the listen_socket code.  Could you see if you could reduce git's
similarity threshold, to get it to realize the listen socket change is just a
rename, and not a new file?  Thanks!

Just nits so far.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.cc:70: DCHECK(!socket_);
include base/logging.h for DCHECK

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.cc:80: int rv =
socket_->Open(AF_UNIX);
Suggest a DCHECK_NE(ERR_IO_PENDING, rv); for documentation purposes.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:11: #include "base/callback.h"
Should be including net/base/completion_callback.h instead.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:30: explicit
UnixDomainClientSocket(scoped_ptr<SocketLibevent> socket);
should include scoped_ptr

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:35: // platform, it supports
abstract namespace.
nit:  it->this, namespace->namespaces.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
These are generally called net_log_

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
Should include net/base/net_log.h

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
Should also have a comment that this BoundNetLog just throws away everything.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:12: #include
"net/socket/unix_domain_client_socket_posix.h"
nit:  This should go up top

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:22: std::string
MakeSocketPathForTesting() {
nit:  Maybe Make -> Get?

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:22: std::string
MakeSocketPathForTesting() {
nit:  "ForTesting" generally not needed in a unittest file.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:25: return
temp_dir.Append(kSocketFilename).value();
This doesn't work - multiple tests should be able to run in parallel.  Should
create a distinct filename each time the test is run.  I don't know if
CreateTemporaryFile, which actually creates the file itself, works with unix
domain sockets, or if we should just create a temporary directory and then just
use some filename we like there.

Know this is what the old code does, but it's a bug.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:40: CompletionCallback
GetCompletionCallback(int expected_rv) {
nit:  Maybe Get -> Create?

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:56: void
DeleteSocketPathForTesting() {
nit:  "ForTesting" generally not needed in a unittest file.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:63: const bool
use_abstract_namespace = false;
Sure use kConstNamingScheme for consts, I believe.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:23:
use_abstract_namespace_(use_abstract_namespace) {
Maybe add DCHECK(!auth_callback_.is_null())?

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:44: return true;
optional:  May be a little cleaner to keep the paths separate:

  return true;
#else
  return getpeereid(socket, user_id, group_id) >= 0;
#endif

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:66: int rv =
listen_socket_->Open(AF_UNIX);
Maybe DCHECK_NE(ERR_IO_PENDING, rv);?

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:116: }
Hrm...Don't see a clean way to avoid duplicating this code (The OK check and
subsequent call to AuthenticateAndGetStreamSocket).  Don't suppose you have any
ideas.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.h:41: gid_t* group_id);
I should say that I'm not remotely familiar with unix domain sockets.

Does this need to be public?

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.h:53: void
AcceptCompleted(scoped_ptr<StreamSocket>* socket,
optional nit:  OnAcceptComplete seems to follow the more common naming scheme.

[byungchul, 2014-07-11 02:56:33]

Tried to reduce renameLimit, but it's only for diff and merge. Actually, git
detects correctly for unix_domain_listen_socket_posix_unittests.cc, but here it
just shows "added".

Basically, I didn't touch unix_domain_socket_posix.cc/.h except name and moving
GetPeerId() and AuthCallback.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.cc:70: DCHECK(!socket_);
On 2014/07/10 19:07:27, mmenke wrote:
> include base/logging.h for DCHECK

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.cc:80: int rv =
socket_->Open(AF_UNIX);
On 2014/07/10 19:07:27, mmenke wrote:
> Suggest a DCHECK_NE(ERR_IO_PENDING, rv); for documentation purposes.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:11: #include "base/callback.h"
On 2014/07/10 19:07:27, mmenke wrote:
> Should be including net/base/completion_callback.h instead.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:30: explicit
UnixDomainClientSocket(scoped_ptr<SocketLibevent> socket);
On 2014/07/10 19:07:28, mmenke wrote:
> should include scoped_ptr

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:35: // platform, it supports
abstract namespace.
On 2014/07/10 19:07:27, mmenke wrote:
> nit:  it->this, namespace->namespaces.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
On 2014/07/10 19:07:27, mmenke wrote:
> These are generally called net_log_

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
On 2014/07/10 19:07:27, mmenke wrote:
> Should include net/base/net_log.h

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix.h:68: BoundNetLog netlog_;
On 2014/07/10 19:07:28, mmenke wrote:
> Should also have a comment that this BoundNetLog just throws away everything.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:12: #include
"net/socket/unix_domain_client_socket_posix.h"
On 2014/07/10 19:07:28, mmenke wrote:
> nit:  This should go up top

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:22: std::string
MakeSocketPathForTesting() {
On 2014/07/10 19:07:28, mmenke wrote:
> nit:  "ForTesting" generally not needed in a unittest file.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:22: std::string
MakeSocketPathForTesting() {
On 2014/07/10 19:07:28, mmenke wrote:
> nit:  Maybe Make -> Get?

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:25: return
temp_dir.Append(kSocketFilename).value();
On 2014/07/10 19:07:28, mmenke wrote:
> This doesn't work - multiple tests should be able to run in parallel.  Should
> create a distinct filename each time the test is run.  I don't know if
> CreateTemporaryFile, which actually creates the file itself, works with unix
> domain sockets, or if we should just create a temporary directory and then
just
> use some filename we like there.
> 
> Know this is what the old code does, but it's a bug.

Creates a temp dir instead.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:40: CompletionCallback
GetCompletionCallback(int expected_rv) {
On 2014/07/10 19:07:28, mmenke wrote:
> nit:  Maybe Get -> Create?

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:56: void
DeleteSocketPathForTesting() {
On 2014/07/10 19:07:28, mmenke wrote:
> nit:  "ForTesting" generally not needed in a unittest file.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_clien...
net/socket/unix_domain_client_socket_posix_unittest.cc:63: const bool
use_abstract_namespace = false;
On 2014/07/10 19:07:28, mmenke wrote:
> Sure use kConstNamingScheme for consts, I believe.

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:23:
use_abstract_namespace_(use_abstract_namespace) {
On 2014/07/10 19:07:28, mmenke wrote:
> Maybe add DCHECK(!auth_callback_.is_null())?

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:44: return true;
On 2014/07/10 19:07:28, mmenke wrote:
> optional:  May be a little cleaner to keep the paths separate:
> 
>   return true;
> #else
>   return getpeereid(socket, user_id, group_id) >= 0;
> #endif

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:66: int rv =
listen_socket_->Open(AF_UNIX);
On 2014/07/10 19:07:28, mmenke wrote:
> Maybe DCHECK_NE(ERR_IO_PENDING, rv);?

Done.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.cc:116: }
On 2014/07/10 19:07:28, mmenke wrote:
> Hrm...Don't see a clean way to avoid duplicating this code (The OK check and
> subsequent call to AuthenticateAndGetStreamSocket).  Don't suppose you have
any
> ideas.

Well, if rv != OK goes into AuthenticateAndGetStreamSocket() and returns true,
we can do that technically, but afraid it is less readable.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.h:41: gid_t* group_id);
On 2014/07/10 19:07:28, mmenke wrote:
> I should say that I'm not remotely familiar with unix domain sockets.
> 
> Does this need to be public?

I moved it from old unix listen socket and used by this and by listen socket.
That's why it is public. If you allow, I will make this private and make listen
socket as friend class.

https://codereview.chromium.org/376323002/diff/1/net/socket/unix_domain_serve...
net/socket/unix_domain_server_socket_posix.h:53: void
AcceptCompleted(scoped_ptr<StreamSocket>* socket,
On 2014/07/10 19:07:28, mmenke wrote:
> optional nit:  OnAcceptComplete seems to follow the more common naming scheme.

TCPSocketLibevent has same naming rule though.

[byungchul, 2014-07-11 16:19:19]

PTAL,

For review convenience, reverted filename of unix domain listen socket
temporarily. Will re-change it before submitting this CL>

[mmenke, 2014-07-11 20:44:05]

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:43: virtual void SetUp()
OVERRIDE {
Can just do this in the constructor / destructor (Every test iteration gets its
own test fixture object).

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:47: socket_path_ =
temp_dir.Append(kSocketFilename).value();
Suggest you use base/files/scoped_temp_dir.h.  Sorry for not suggesting this
earlier, forgot about the class.  Same goes for other files.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:68:
EXPECT_TRUE(!accepted_socket);
nit:  EXPECT_TRUE(!...) is the same as EXPECT_FALSE(...)

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:73: EXPECT_EQ(OK,
client_socket.Connect(GetCompletionCallback(OK)));
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.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:75: // Not yet notified
to server.
Maybe:  "Server has not yet been notified of the connection."

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:78:
base::RunLoop().RunUntilIdle();
Is the connect message guaranteed to already be queued at this point?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:80:
EXPECT_TRUE(accepted_socket->IsConnected());
Maybe disconnect and then check IsConnected?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:113:
TEST_F(UnixDomainClientSocketTest, ConnectToNonExistingSocket) {
ConnectToNonExistentServer, maybe?  (And same for next test)

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:150: // Sends data from
client to server.
nit:  I believe imperatives ("Send data from client to server") are a lot more
common in comments in the code itself.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:156:
GetCompletionCallback(OK)));
Are we guaranteed the entire buffer will be written synchronously in one write?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:157: // Buffer bigger
than write data size.
Per google style guide, comments should generally be sentences ("The buffer is
bigger than...")

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:162:
GetCompletionCallback(OK)));
Should compare buffer contents here, and below, out of paranoia.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:162:
GetCompletionCallback(OK)));
Again, are we guaranteed the entire thing will be read in one synchronous read?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:164: // Sends data from
server and client
nit:  period

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:179:
GetCompletionCallback(ERR_CONNECTION_CLOSED)));
Should we wait to get the close message from the server?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:182:
TEST_F(UnixDomainClientSocketTest, ReadWriteAsynchronously) {
There are no asyncronous writes here.  Maybe ReadBeforeWrite?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
Can we validate uid and gid here?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:38: return ERR_ADDRESS_INVALID;
nit:  Use braces when the condition takes up more than one line.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:40: SocketDescriptor s =
CreatePlatformSocket(PF_UNIX, SOCK_STREAM, 0);
nit:  The name "s" violates the google naming guidelines.  find with just fd,
since we're already using that, or descriptor or socket or something.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:52: DCHECK(socket_fd);
nit:  Know this was here before, but since we're cleaning this up, anyways, this
should be at the top of this function.

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

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:43: virtual void SetUp()
OVERRIDE {
On 2014/07/11 20:44:03, mmenke wrote:
> Can just do this in the constructor / destructor (Every test iteration gets
its
> own test fixture object).

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:47: socket_path_ =
temp_dir.Append(kSocketFilename).value();
On 2014/07/11 20:44:04, mmenke wrote:
> Suggest you use base/files/scoped_temp_dir.h.  Sorry for not suggesting this
> earlier, forgot about the class.  Same goes for other files.

Done. Thank you for good suggestion.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:68:
EXPECT_TRUE(!accepted_socket);
On 2014/07/11 20:44:04, mmenke wrote:
> nit:  EXPECT_TRUE(!...) is the same as EXPECT_FALSE(...)

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:73: EXPECT_EQ(OK,
client_socket.Connect(GetCompletionCallback(OK)));
On 2014/07/11 20:44:03, mmenke wrote:
> 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.

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

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:75: // Not yet notified
to server.
On 2014/07/11 20:44:02, mmenke wrote:
> Maybe:  "Server has not yet been notified of the connection."

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:78:
base::RunLoop().RunUntilIdle();
On 2014/07/11 20:44:04, mmenke wrote:
> Is the connect message guaranteed to already be queued at this point?

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().

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:80:
EXPECT_TRUE(accepted_socket->IsConnected());
On 2014/07/11 20:44:04, mmenke wrote:
> Maybe disconnect and then check IsConnected?

Done in 2 other unittests disconnecting from each side.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:113:
TEST_F(UnixDomainClientSocketTest, ConnectToNonExistingSocket) {
On 2014/07/11 20:44:03, mmenke wrote:
> ConnectToNonExistentServer, maybe?  (And same for next test)

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:150: // Sends data from
client to server.
On 2014/07/11 20:44:02, mmenke wrote:
> nit:  I believe imperatives ("Send data from client to server") are a lot more
> common in comments in the code itself.

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:156:
GetCompletionCallback(OK)));
On 2014/07/11 20:44:03, mmenke wrote:
> Are we guaranteed the entire buffer will be written synchronously in one
write?

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

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:157: // Buffer bigger
than write data size.
On 2014/07/11 20:44:04, mmenke wrote:
> Per google style guide, comments should generally be sentences ("The buffer is
> bigger than...")

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:162:
GetCompletionCallback(OK)));
On 2014/07/11 20:44:04, mmenke wrote:
> Again, are we guaranteed the entire thing will be read in one synchronous
read?

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

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:162:
GetCompletionCallback(OK)));
On 2014/07/11 20:44:03, mmenke wrote:
> Should compare buffer contents here, and below, out of paranoia.

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:164: // Sends data from
server and client
On 2014/07/11 20:44:02, mmenke wrote:
> nit:  period

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:179:
GetCompletionCallback(ERR_CONNECTION_CLOSED)));
On 2014/07/11 20:44:02, mmenke wrote:
> Should we wait to get the close message from the server?

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_c...
net/socket/unix_domain_client_socket_posix_unittest.cc:182:
TEST_F(UnixDomainClientSocketTest, ReadWriteAsynchronously) {
On 2014/07/11 20:44:03, mmenke wrote:
> There are no asyncronous writes here.  Maybe ReadBeforeWrite?

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
On 2014/07/11 20:44:04, mmenke wrote:
> Can we validate uid and gid here?

I don't understand what you want to validate? Validate if uid and gid are
expected (UID and GID of current process)?

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:38: return ERR_ADDRESS_INVALID;
On 2014/07/11 20:44:05, mmenke wrote:
> nit:  Use braces when the condition takes up more than one line.

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:40: SocketDescriptor s =
CreatePlatformSocket(PF_UNIX, SOCK_STREAM, 0);
On 2014/07/11 20:44:05, mmenke wrote:
> nit:  The name "s" violates the google naming guidelines.  find with just fd,
> since we're already using that, or descriptor or socket or something.

Done.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_socket_posix.cc:52: DCHECK(socket_fd);
On 2014/07/11 20:44:05, mmenke wrote:
> nit:  Know this was here before, but since we're cleaning this up, anyways,
this
> should be at the top of this function.

Done.

[mmenke, 2014-07-14 19:41:29]

So one issue remains - expecting reads/writes to be synchronous makes me
extremely nervous.  Could imagine all sorts of underlying implementations where
even copying the buffer occurs on another thread, managed by the kernel, or
something.

I really don't think we should be relying on particular kernel behavior, unless
there's some API-level documentation that guarantees these will by synchronous.

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
On 2014/07/14 17:49:23, byungchul wrote:
> On 2014/07/11 20:44:04, mmenke wrote:
> > Can we validate uid and gid here?
> 
> I don't understand what you want to validate? Validate if uid and gid are
> expected (UID and GID of current process)?

Yes - mostly just to make sure they're populated, and we keep them straight.

[byungchul, 2014-07-14 22:47:14]

On 2014/07/14 19:41:29, mmenke wrote:
> So one issue remains - expecting reads/writes to be synchronous makes me
> extremely nervous.  Could imagine all sorts of underlying implementations
where
> even copying the buffer occurs on another thread, managed by the kernel, or
> something.
> 
> I really don't think we should be relying on particular kernel behavior,
unless
> there's some API-level documentation that guarantees these will by
synchronous.
Changed unittests to use TestCompletionCallback and made sure all read/write op
synchronous.

> 
>
https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
> File net/socket/unix_domain_server_socket_posix_unittest.cc (right):
> 
>
https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
> net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
> UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
> On 2014/07/14 17:49:23, byungchul wrote:
> > On 2014/07/11 20:44:04, mmenke wrote:
> > > Can we validate uid and gid here?
> > 
> > I don't understand what you want to validate? Validate if uid and gid are
> > expected (UID and GID of current process)?
> 
> Yes - mostly just to make sure they're populated, and we keep them straight.
Not sure if it is worthy doing because it just test getsockopt or getpeereid in
GetPeerIds() working as expected.

[byungchul, 2014-07-14 22:56:49]

On 2014/07/14 22:47:14, byungchul wrote:
> On 2014/07/14 19:41:29, mmenke wrote:
> > So one issue remains - expecting reads/writes to be synchronous makes me
> > extremely nervous.  Could imagine all sorts of underlying implementations
> where
> > even copying the buffer occurs on another thread, managed by the kernel, or
> > something.
> > 
> > I really don't think we should be relying on particular kernel behavior,
> unless
> > there's some API-level documentation that guarantees these will by
> synchronous.
> Changed unittests to use TestCompletionCallback and made sure all read/write
op
> synchronous.
> 
> > 
> >
>
https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
> > File net/socket/unix_domain_server_socket_posix_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
> > net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
> > UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
> > On 2014/07/14 17:49:23, byungchul wrote:
> > > On 2014/07/11 20:44:04, mmenke wrote:
> > > > Can we validate uid and gid here?
> > > 
> > > I don't understand what you want to validate? Validate if uid and gid are
> > > expected (UID and GID of current process)?
> > 
> > Yes - mostly just to make sure they're populated, and we keep them straight.
> Not sure if it is worthy doing because it just test getsockopt or getpeereid
in
> GetPeerIds() working as expected.
Done.

[mmenke, 2014-07-15 15:25:57]

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:97: return
write_buf->BytesConsumed();
Still theoretically possible for these both to fail on conforming
implementations, since we assume we can completely write to one socket before
reading anything on the other.  I believe the old tests handle this by using a
second thread, which is a bit of a pain.

Do we care enough to fix that in this CL?  I'm not sure we do.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:130: // kernel accepts
the connection.
Since we now work with accepting the socket async, may want to rephrase this to
something like:

"Connect() generally returns OK synchronously, but we support async responses,
just in case"

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
As with the successful connection tests, should allow failures to happen
asynchronously (Goes for the rest of the tests, too).

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:233:
TEST_F(UnixDomainClientSocketTest, DisconnectFromServer) {
Don't think we have any tests where we get a closed message when trying to read
or write from a socket.  The reading test is pretty easy to do, not sure about
the writing one.

The main difference from this test is that the read completion callback gets
called with an error.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
If this case ever happens, I think the IsConnected() expectation below may (or
may not) fail.  Makes testing this a little hairier

[byungchul, 2014-07-15 17:30:45]

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/40001/net/socket/unix_domain_s...
net/socket/unix_domain_server_socket_posix_unittest.cc:27: bool
UserCanConnectCallback(bool allow_user, uid_t uid, gid_t gid) {
On 2014/07/14 19:41:28, mmenke wrote:
> On 2014/07/14 17:49:23, byungchul wrote:
> > On 2014/07/11 20:44:04, mmenke wrote:
> > > Can we validate uid and gid here?
> > 
> > I don't understand what you want to validate? Validate if uid and gid are
> > expected (UID and GID of current process)?
> 
> Yes - mostly just to make sure they're populated, and we keep them straight.

Done.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:97: return
write_buf->BytesConsumed();
On 2014/07/15 15:25:56, mmenke wrote:
> Still theoretically possible for these both to fail on conforming
> implementations, since we assume we can completely write to one socket before
> reading anything on the other.  I believe the old tests handle this by using a
> second thread, which is a bit of a pain.
> 
> Do we care enough to fix that in this CL?  I'm not sure we do.

Default socket write buffer is much bigger than the data amount written by these
unittests. So, write() must also succeed regardless of underlying kernel
implementation. read() could be split according to kernel.
I want more deterministic unittests, but using multiple thread sounds like less
deterministic unless they are synchronized. And, I don't see any difference of
synchronized multi-threading and single-threading here.

Basically, I feel that WriteSynchronously() is overkilling. I am willing to make
unittests for async write by reducing write/read socket buffer. But, not sure if
it could be deterministic because the data amount could be written without
pending would depend on internal buffer and minimum socket buffer size besides
socket buffers.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:130: // kernel accepts
the connection.
On 2014/07/15 15:25:56, mmenke wrote:
> Since we now work with accepting the socket async, may want to rephrase this
to
> something like:
> 
> "Connect() generally returns OK synchronously, but we support async responses,
> just in case"

Done.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
On 2014/07/15 15:25:56, mmenke wrote:
> As with the successful connection tests, should allow failures to happen
> asynchronously (Goes for the rest of the tests, too).

I have no idea how to make async connect() for unix domain socket. All
operations are handled by kernel and never goes out. Unless we have mock
SocketLibevent, I don't think it is possible. And, I don't think testing with
mock SocketLibevent because UnixDomainClient/ServerSocket is just a wrapper of
SocketLibevent.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:233:
TEST_F(UnixDomainClientSocketTest, DisconnectFromServer) {
On 2014/07/15 15:25:56, mmenke wrote:
> Don't think we have any tests where we get a closed message when trying to
read
> or write from a socket.  The reading test is pretty easy to do, not sure about
> the writing one.
> 
> The main difference from this test is that the read completion callback gets
> called with an error.

Done for reading.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
On 2014/07/15 15:25:56, mmenke wrote:
> If this case ever happens, I think the IsConnected() expectation below may (or
> may not) fail.  Makes testing this a little hairier

Sorry, I don't understand. How could IsConnect() fail?

[mmenke, 2014-07-15 17:44:41]

Quick responses.  Interview soon, so may not get back to this until tomorrow.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
On 2014/07/15 17:30:45, byungchul wrote:
> On 2014/07/15 15:25:56, mmenke wrote:
> > As with the successful connection tests, should allow failures to happen
> > asynchronously (Goes for the rest of the tests, too).
> 
> I have no idea how to make async connect() for unix domain socket. All
> operations are handled by kernel and never goes out. Unless we have mock
> SocketLibevent, I don't think it is possible. And, I don't think testing with
> mock SocketLibevent because UnixDomainClient/ServerSocket is just a wrapper of
> SocketLibevent.

Not suggesting you make them, just that these tests pass if they happen.  Can do
same thing as the success tests:

int rv = client_socket.Connect(connect_callback.callback());
if (rv == ERR_IO_PENDING)
  rv = connect_callback.WaitForResult();
EXPECT_EQ(ERR_FILE_NOT_FOUND, rv);

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
On 2014/07/15 17:30:45, byungchul wrote:
> On 2014/07/15 15:25:56, mmenke wrote:
> > If this case ever happens, I think the IsConnected() expectation below may
(or
> > may not) fail.  Makes testing this a little hairier
> 
> Sorry, I don't understand. How could IsConnect() fail?

If in WaitForResult, the accept callback is also called, and hangs up on us.

[byungchul, 2014-07-15 18:07:01]

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
On 2014/07/15 17:44:41, mmenke wrote:
> On 2014/07/15 17:30:45, byungchul wrote:
> > On 2014/07/15 15:25:56, mmenke wrote:
> > > As with the successful connection tests, should allow failures to happen
> > > asynchronously (Goes for the rest of the tests, too).
> > 
> > I have no idea how to make async connect() for unix domain socket. All
> > operations are handled by kernel and never goes out. Unless we have mock
> > SocketLibevent, I don't think it is possible. And, I don't think testing
with
> > mock SocketLibevent because UnixDomainClient/ServerSocket is just a wrapper
of
> > SocketLibevent.
> 
> Not suggesting you make them, just that these tests pass if they happen.  Can
do
> same thing as the success tests:
> 
> int rv = client_socket.Connect(connect_callback.callback());
> if (rv == ERR_IO_PENDING)
>   rv = connect_callback.WaitForResult();
> EXPECT_EQ(ERR_FILE_NOT_FOUND, rv);
> 

Oh, I see what you meant.
Here, async is hanlded only for OK cases. All error cases are detected
synchronously. I don't think it is necessary, but if you feel strongly, am
willing to do it. Please confirm again.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
On 2014/07/15 17:44:41, mmenke wrote:
> On 2014/07/15 17:30:45, byungchul wrote:
> > On 2014/07/15 15:25:56, mmenke wrote:
> > > If this case ever happens, I think the IsConnected() expectation below may
> (or
> > > may not) fail.  Makes testing this a little hairier
> > 
> > Sorry, I don't understand. How could IsConnect() fail?
> 
> If in WaitForResult, the accept callback is also called, and hangs up on us.

Even if this happens, accept_callback will not be called because of auth error.
And, even if accept_callback is called, it just sets boolean flags. So,
connect_callback.WaitForResult() which runs message loop internally doesn't
block any execution. Am I missing something?

[mmenke, 2014-07-15 19:12:57]

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:97: return
write_buf->BytesConsumed();
On 2014/07/15 17:30:45, byungchul wrote:
> On 2014/07/15 15:25:56, mmenke wrote:
> > Still theoretically possible for these both to fail on conforming
> > implementations, since we assume we can completely write to one socket
before
> > reading anything on the other.  I believe the old tests handle this by using
a
> > second thread, which is a bit of a pain.
> > 
> > Do we care enough to fix that in this CL?  I'm not sure we do.
> 
> Default socket write buffer is much bigger than the data amount written by
these
> unittests. So, write() must also succeed regardless of underlying kernel
> implementation. read() could be split according to kernel.
> I want more deterministic unittests, but using multiple thread sounds like
less
> deterministic unless they are synchronized. And, I don't see any difference of
> synchronized multi-threading and single-threading here.
> 
> Basically, I feel that WriteSynchronously() is overkilling. I am willing to
make
> unittests for async write by reducing write/read socket buffer. But, not sure
if
> it could be deterministic because the data amount could be written without
> pending would depend on internal buffer and minimum socket buffer size besides
> socket buffers.

My general feeling is that we should depend on public APIs, and their documented
behavior.  We should not be depending on behind-the-scenes logic.  This is
needed to ensure we don't break when the underlying APIs change to have still
compliant behavior, but that is different, or when running on platforms that
violate our assumptions about how API behaves in ways not officially specced
out.  It's also possible our assumptions may be wrong, under certain
circumstances.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
On 2014/07/15 18:07:01, byungchul wrote:
> On 2014/07/15 17:44:41, mmenke wrote:
> > On 2014/07/15 17:30:45, byungchul wrote:
> > > On 2014/07/15 15:25:56, mmenke wrote:
> > > > As with the successful connection tests, should allow failures to happen
> > > > asynchronously (Goes for the rest of the tests, too).
> > > 
> > > I have no idea how to make async connect() for unix domain socket. All
> > > operations are handled by kernel and never goes out. Unless we have mock
> > > SocketLibevent, I don't think it is possible. And, I don't think testing
> with
> > > mock SocketLibevent because UnixDomainClient/ServerSocket is just a
wrapper
> of
> > > SocketLibevent.
> > 
> > Not suggesting you make them, just that these tests pass if they happen. 
Can
> do
> > same thing as the success tests:
> > 
> > int rv = client_socket.Connect(connect_callback.callback());
> > if (rv == ERR_IO_PENDING)
> >   rv = connect_callback.WaitForResult();
> > EXPECT_EQ(ERR_FILE_NOT_FOUND, rv);
> > 
> 
> Oh, I see what you meant.
> Here, async is hanlded only for OK cases. All error cases are detected
> synchronously. I don't think it is necessary, but if you feel strongly, am
> willing to do it. Please confirm again.

I see no code that guarantees this error case will be detected synchronously. 
If we open the socket but it fails to connect, we handle sync and async connect
failures in socket_libevent.  Or does the socket fail to open, before we get
that far?  If so, is there documentation of that somewhere?

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
On 2014/07/15 18:07:01, byungchul wrote:
> On 2014/07/15 17:44:41, mmenke wrote:
> > On 2014/07/15 17:30:45, byungchul wrote:
> > > On 2014/07/15 15:25:56, mmenke wrote:
> > > > If this case ever happens, I think the IsConnected() expectation below
may
> > (or
> > > > may not) fail.  Makes testing this a little hairier
> > > 
> > > Sorry, I don't understand. How could IsConnect() fail?
> > 
> > If in WaitForResult, the accept callback is also called, and hangs up on us.
> 
> Even if this happens, accept_callback will not be called because of auth
error.
> And, even if accept_callback is called, it just sets boolean flags. So,
> connect_callback.WaitForResult() which runs message loop internally doesn't
> block any execution. Am I missing something?

I'm not concerned about accept_socket not being NULL, I'm concerned about
expecting an "OK" rv and expecting "client_socket.IsConnected()" to be true,
until the runloop is run.

[byungchul, 2014-07-15 22:01:18]

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:97: return
write_buf->BytesConsumed();
On 2014/07/15 19:12:57, mmenke wrote:
> On 2014/07/15 17:30:45, byungchul wrote:
> > On 2014/07/15 15:25:56, mmenke wrote:
> > > Still theoretically possible for these both to fail on conforming
> > > implementations, since we assume we can completely write to one socket
> before
> > > reading anything on the other.  I believe the old tests handle this by
using
> a
> > > second thread, which is a bit of a pain.
> > > 
> > > Do we care enough to fix that in this CL?  I'm not sure we do.
> > 
> > Default socket write buffer is much bigger than the data amount written by
> these
> > unittests. So, write() must also succeed regardless of underlying kernel
> > implementation. read() could be split according to kernel.
> > I want more deterministic unittests, but using multiple thread sounds like
> less
> > deterministic unless they are synchronized. And, I don't see any difference
of
> > synchronized multi-threading and single-threading here.
> > 
> > Basically, I feel that WriteSynchronously() is overkilling. I am willing to
> make
> > unittests for async write by reducing write/read socket buffer. But, not
sure
> if
> > it could be deterministic because the data amount could be written without
> > pending would depend on internal buffer and minimum socket buffer size
besides
> > socket buffers.
> 
> My general feeling is that we should depend on public APIs, and their
documented
> behavior.  We should not be depending on behind-the-scenes logic.  This is
> needed to ensure we don't break when the underlying APIs change to have still
> compliant behavior, but that is different, or when running on platforms that
> violate our assumptions about how API behaves in ways not officially specced
> out.  It's also possible our assumptions may be wrong, under certain
> circumstances.

At least for write(), I think socket buffer size clearly documented. See "man 7
socket".

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:189:
client_socket.Connect(connect_callback.callback()));
On 2014/07/15 19:12:57, mmenke wrote:
> On 2014/07/15 18:07:01, byungchul wrote:
> > On 2014/07/15 17:44:41, mmenke wrote:
> > > On 2014/07/15 17:30:45, byungchul wrote:
> > > > On 2014/07/15 15:25:56, mmenke wrote:
> > > > > As with the successful connection tests, should allow failures to
happen
> > > > > asynchronously (Goes for the rest of the tests, too).
> > > > 
> > > > I have no idea how to make async connect() for unix domain socket. All
> > > > operations are handled by kernel and never goes out. Unless we have mock
> > > > SocketLibevent, I don't think it is possible. And, I don't think testing
> > with
> > > > mock SocketLibevent because UnixDomainClient/ServerSocket is just a
> wrapper
> > of
> > > > SocketLibevent.
> > > 
> > > Not suggesting you make them, just that these tests pass if they happen. 
> Can
> > do
> > > same thing as the success tests:
> > > 
> > > int rv = client_socket.Connect(connect_callback.callback());
> > > if (rv == ERR_IO_PENDING)
> > >   rv = connect_callback.WaitForResult();
> > > EXPECT_EQ(ERR_FILE_NOT_FOUND, rv);
> > > 
> > 
> > Oh, I see what you meant.
> > Here, async is hanlded only for OK cases. All error cases are detected
> > synchronously. I don't think it is necessary, but if you feel strongly, am
> > willing to do it. Please confirm again.
> 
> I see no code that guarantees this error case will be detected synchronously. 
> If we open the socket but it fails to connect, we handle sync and async
connect
> failures in socket_libevent.  Or does the socket fail to open, before we get
> that far?  If so, is there documentation of that somewhere?

Done.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:93: rv =
connect_callback.WaitForResult();
On 2014/07/15 19:12:57, mmenke wrote:
> On 2014/07/15 18:07:01, byungchul wrote:
> > On 2014/07/15 17:44:41, mmenke wrote:
> > > On 2014/07/15 17:30:45, byungchul wrote:
> > > > On 2014/07/15 15:25:56, mmenke wrote:
> > > > > If this case ever happens, I think the IsConnected() expectation below
> may
> > > (or
> > > > > may not) fail.  Makes testing this a little hairier
> > > > 
> > > > Sorry, I don't understand. How could IsConnect() fail?
> > > 
> > > If in WaitForResult, the accept callback is also called, and hangs up on
us.
> > 
> > Even if this happens, accept_callback will not be called because of auth
> error.
> > And, even if accept_callback is called, it just sets boolean flags. So,
> > connect_callback.WaitForResult() which runs message loop internally doesn't
> > block any execution. Am I missing something?
> 
> I'm not concerned about accept_socket not being NULL, I'm concerned about
> expecting an "OK" rv and expecting "client_socket.IsConnected()" to be true,
> until the runloop is run.

Oh, now I understand what you meant. Done.

[mmenke, 2014-07-16 18:30:36]

I want to do a final once over later today, but I'm pretty much ready to sign
off on this.

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/120001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:97: return
write_buf->BytesConsumed();
On 2014/07/15 22:01:18, byungchul wrote:
> On 2014/07/15 19:12:57, mmenke wrote:
> > On 2014/07/15 17:30:45, byungchul wrote:
> > > On 2014/07/15 15:25:56, mmenke wrote:
> > > > Still theoretically possible for these both to fail on conforming
> > > > implementations, since we assume we can completely write to one socket
> > before
> > > > reading anything on the other.  I believe the old tests handle this by
> using
> > a
> > > > second thread, which is a bit of a pain.
> > > > 
> > > > Do we care enough to fix that in this CL?  I'm not sure we do.
> > > 
> > > Default socket write buffer is much bigger than the data amount written by
> > these
> > > unittests. So, write() must also succeed regardless of underlying kernel
> > > implementation. read() could be split according to kernel.
> > > I want more deterministic unittests, but using multiple thread sounds like
> > less
> > > deterministic unless they are synchronized. And, I don't see any
difference
> of
> > > synchronized multi-threading and single-threading here.
> > > 
> > > Basically, I feel that WriteSynchronously() is overkilling. I am willing
to
> > make
> > > unittests for async write by reducing write/read socket buffer. But, not
> sure
> > if
> > > it could be deterministic because the data amount could be written without
> > > pending would depend on internal buffer and minimum socket buffer size
> besides
> > > socket buffers.
> > 
> > My general feeling is that we should depend on public APIs, and their
> documented
> > behavior.  We should not be depending on behind-the-scenes logic.  This is
> > needed to ensure we don't break when the underlying APIs change to have
still
> > compliant behavior, but that is different, or when running on platforms that
> > violate our assumptions about how API behaves in ways not officially specced
> > out.  It's also possible our assumptions may be wrong, under certain
> > circumstances.
> 
> At least for write(), I think socket buffer size clearly documented. See "man
7
> socket".

And that applies to all supported platforms that this test will be run on?  It's
also unclear if the buffer is used as one would expect - that is, fully
allocated on socket creation, and write everything we can, up to that limit, on
the first write attempt (And all subsequent write attempts).

Hrm...So it looks like "TEST_F(TCPSocketTest, ReadWrite)" is doing just what
this test is now doing, which makes me more comfortable not worrying about the
(rather unlikely) case this code doesn't work on a particular platform.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:197: EXPECT_EQ(OK,
ConnectSynchronously(&client_socket));
This is a bug, right?

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:219: // Wait for read
event.
nit:  This isn't accurate - we're not waiting, we're just trying to read.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:220: const int
kWriteDataSize = 10;
nit:  kReadDataSize?

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:235: // by local.
SocketLibevent just clears callbacks.
"by local" -> "locally" or maybe "locally, before the remote side closes it"?

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:89: // Return success
first because server socket accepts it.
nit:  Maybe "Connect will return OK, before the server rejects the connection."

[byungchul, 2014-07-16 22:02:31]

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:197: EXPECT_EQ(OK,
ConnectSynchronously(&client_socket));
On 2014/07/16 18:30:35, mmenke wrote:
> This is a bug, right?

Oops, sorry. Done.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:219: // Wait for read
event.
On 2014/07/16 18:30:35, mmenke wrote:
> nit:  This isn't accurate - we're not waiting, we're just trying to read.

Done.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:220: const int
kWriteDataSize = 10;
On 2014/07/16 18:30:35, mmenke wrote:
> nit:  kReadDataSize?

Done.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:235: // by local.
SocketLibevent just clears callbacks.
On 2014/07/16 18:30:35, mmenke wrote:
> "by local" -> "locally" or maybe "locally, before the remote side closes it"?

Done.

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/160001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix_unittest.cc:89: // Return success
first because server socket accepts it.
On 2014/07/16 18:30:36, mmenke wrote:
> nit:  Maybe "Connect will return OK, before the server rejects the
connection."

Done.

[mmenke, 2014-07-17 15:42:22]

Looks like you have failing tests, and don't forget to rename the
unix_domain_socket_posix files again.

[byungchul, 2014-07-22 21:49:47]

On 2014/07/17 15:42:22, mmenke wrote:
> Looks like you have failing tests, and don't forget to rename the
> unix_domain_socket_posix files again.

PTAL.
Renamed files for unix domain listen socket, and reverted scoped temp dir for
unix domain listen socket unittests for now and added a TODO not to block this
CL.

[mmenke, 2014-07-23 18:52:41]

LGTM!

No need to rush to land and merge it into your other CL (Though feel free to do
that, if you want).  I'll do a pass on your other CL tomorrow, probably late in
the day (Have an interview in the early afternoon), and I don't think these CLs
will have a significant impact on the part that still needs to be reviewed.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.cc:40: size_t path_max =
address->addr_len - offsetof(struct sockaddr_un, sun_path);
Can we just use sizeof(struct sockaddr_storage) or sizeof(addr->address) instead
of addr_len?  Seems weird that if this is called twice for different client
sockets, on the same SockaddrStorage, the first call can cause the second call
to fail, if the second address is longer.

You'd also need to use the same value in the memset below, but otherwise, code
would be the same.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.cc:40: size_t path_max =
address->addr_len - offsetof(struct sockaddr_un, sun_path);
Hrm...SockaddrStorage is not guaranteed to be long enough to hold any AF_UNIX
address.  Do we care?  I guess we didn't, before.  May be worth a comment about
the length here.

From socket.h:

/* Structure large enough to hold any socket address (with the historical
   exception of AF_UNIX).  We reserve 128 bytes.  */
#define __ss_aligntype	unsigned long int
#define _SS_SIZE	128
#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))

struct sockaddr_storage
  {
    __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
    __ss_aligntype __ss_align;	/* Force desired alignment.  */
    char __ss_padding[_SS_PADSIZE];
  };

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.h:26: // Builds a client socket with
socket_path. The caller should call Connect()
nit:  |socket_path|

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:26: // Here peers are
running in same process. Check if they are expected.
nit:  Suggest just removing the second sentence, think it's harder to map onto
what you're doing than the EXPECT line below.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:50: int min_data_len) {
opetional:  May want a DCHECK_LE(min_data_len, buf_len);  (DCHECK instead of
EXPECT/ASSERT because if it's wrong, there's a bug in the test, not the code
it's testing)

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:54: // Iterate reading
several times (but not infinite) until it reads buf
nit:  "reads at least |min_data_len| bytes into |buf|", maybe?

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:109: }  // namespace
Could just put the entire file in the anonymous names.  Same goes for the other
tests.  We aren't strict about that in net/, but it's theoretically preferred,
when the test fixture and tests don't need to be in the global namespace.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:373:
EXPECT_EQ(kSmallReadBufferSize, read_callback.WaitForResult());
Just out of paranoia, suggest allowing any read > 0 and <= kSmallReadBufferSize,
and the next read call can check how much was actually read.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:379: kWriteDataSize -
kSmallReadBufferSize));
Should check the bytes read from the read two calls.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:121: // to the caller.
optional:  May want to duplicate this comment at the end of the while loop in
Accept

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:25: // Linux.
nit:  Linux -> Linux and Android?

[mmenke, 2014-07-23 18:54:35]

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.cc:40: size_t path_max =
address->addr_len - offsetof(struct sockaddr_un, sun_path);
On 2014/07/23 18:52:40, mmenke wrote:
> Hrm...SockaddrStorage is not guaranteed to be long enough to hold any AF_UNIX
> address.  Do we care?  I guess we didn't, before.  May be worth a comment
about
> the length here.
> 
> From socket.h:
> 
> /* Structure large enough to hold any socket address (with the historical
>    exception of AF_UNIX).  We reserve 128 bytes.  */
> #define __ss_aligntype	unsigned long int
> #define _SS_SIZE	128
> #define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
> 
> struct sockaddr_storage
>   {
>     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
>     __ss_aligntype __ss_align;	/* Force desired alignment.  */
>     char __ss_padding[_SS_PADSIZE];
>   };

Oh....we used sockaddr_un before, which is indeed long enough, so this is
actually a regression.

[byungchul, 2014-07-23 22:27:12]

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.cc:40: size_t path_max =
address->addr_len - offsetof(struct sockaddr_un, sun_path);
On 2014/07/23 18:54:35, mmenke wrote:
> On 2014/07/23 18:52:40, mmenke wrote:
> > Hrm...SockaddrStorage is not guaranteed to be long enough to hold any
AF_UNIX
> > address.  Do we care?  I guess we didn't, before.  May be worth a comment
> about
> > the length here.
> > 
> > From socket.h:
> > 
> > /* Structure large enough to hold any socket address (with the historical
> >    exception of AF_UNIX).  We reserve 128 bytes.  */
> > #define __ss_aligntype	unsigned long int
> > #define _SS_SIZE	128
> > #define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
> > 
> > struct sockaddr_storage
> >   {
> >     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
> >     __ss_aligntype __ss_align;	/* Force desired alignment.  */
> >     char __ss_padding[_SS_PADSIZE];
> >   };
> 
> Oh....we used sockaddr_un before, which is indeed long enough, so this is
> actually a regression.

That explains why I failed in mac_chromium_rel which tries create a long socket
path name (109 bytes). According "man 7 unix":

#define UNIX_PATH_MAX    108

struct sockaddr_un {
    sa_family_t sun_family;               /* AF_UNIX */
    char        sun_path[UNIX_PATH_MAX];  /* pathname */
};

I think sockaddr_storage is big enough though its header comment, at least for
linux. So far, only user of this class is android, linux remoting_host and
unittests. So, hard to say we have regression failure.
Shortened socket path names in unittests. Let's see the result.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix.h:26: // Builds a client socket with
socket_path. The caller should call Connect()
On 2014/07/23 18:52:40, mmenke wrote:
> nit:  |socket_path|

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:26: // Here peers are
running in same process. Check if they are expected.
On 2014/07/23 18:52:40, mmenke wrote:
> nit:  Suggest just removing the second sentence, think it's harder to map onto
> what you're doing than the EXPECT line below.

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:50: int min_data_len) {
On 2014/07/23 18:52:40, mmenke wrote:
> opetional:  May want a DCHECK_LE(min_data_len, buf_len);  (DCHECK instead of
> EXPECT/ASSERT because if it's wrong, there's a bug in the test, not the code
> it's testing)

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:54: // Iterate reading
several times (but not infinite) until it reads buf
On 2014/07/23 18:52:41, mmenke wrote:
> nit:  "reads at least |min_data_len| bytes into |buf|", maybe?

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:109: }  // namespace
On 2014/07/23 18:52:41, mmenke wrote:
> Could just put the entire file in the anonymous names.  Same goes for the
other
> tests.  We aren't strict about that in net/, but it's theoretically preferred,
> when the test fixture and tests don't need to be in the global namespace.

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:373:
EXPECT_EQ(kSmallReadBufferSize, read_callback.WaitForResult());
On 2014/07/23 18:52:40, mmenke wrote:
> Just out of paranoia, suggest allowing any read > 0 and <=
kSmallReadBufferSize,
> and the next read call can check how much was actually read.

Though I don't know what you want exactly, changed the unittests to read one
more small amount data.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:379: kWriteDataSize -
kSmallReadBufferSize));
On 2014/07/23 18:52:40, mmenke wrote:
> Should check the bytes read from the read two calls.

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.cc (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.cc:121: // to the caller.
On 2014/07/23 18:52:41, mmenke wrote:
> optional:  May want to duplicate this comment at the end of the while loop in
> Accept

Done.

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
File net/socket/unix_domain_server_socket_posix.h (right):

https://codereview.chromium.org/376323002/diff/240001/net/socket/unix_domain_...
net/socket/unix_domain_server_socket_posix.h:25: // Linux.
On 2014/07/23 18:52:41, mmenke wrote:
> nit:  Linux -> Linux and Android?

Done.

[byungchul, 2014-07-23 22:34:39]

@darin,

Could you please review this CL for name change of unix domain socket from
UnixDomainSocket to UnixDomainListenSocket?
I got lgtm for /net files, but need your power for files under other
directories.

[darin (slow to review), 2014-07-23 22:39:47]

LGTM, but...

If you are trying to kill off the old class, I might have considered using a
prefix of Deprecated instead of inserting "Listen" in the middle. The new name
is a bit awkward. What is a "listen socket"? That is a very odd name.
DeprecatedUnixDomainSocket* sounds a lot better presuming it is intended to be
removed. It also helps signal to others that they shouldn't use it anymore.

[byungchul, 2014-07-23 22:51:42]

On 2014/07/23 22:39:47, darin wrote:
> LGTM, but...
> 
> If you are trying to kill off the old class, I might have considered using a
> prefix of Deprecated instead of inserting "Listen" in the middle. The new name
> is a bit awkward. What is a "listen socket"? That is a very odd name.
> DeprecatedUnixDomainSocket* sounds a lot better presuming it is intended to be
> removed. It also helps signal to others that they shouldn't use it anymore.

ListenSocket came from StreamListenSocket which it inherits. Yes, my intention
is to remove it eventually once http server refactoring is done.

@Matt, do you mind to replace the name with DeprecatedUnixDomainSocket?

[mmenke, 2014-07-23 23:50:50]

On 2014/07/23 22:51:42, byungchul wrote:
> On 2014/07/23 22:39:47, darin wrote:
> > LGTM, but...
> > 
> > If you are trying to kill off the old class, I might have considered using a
> > prefix of Deprecated instead of inserting "Listen" in the middle. The new
name
> > is a bit awkward. What is a "listen socket"? That is a very odd name.
> > DeprecatedUnixDomainSocket* sounds a lot better presuming it is intended to
be
> > removed. It also helps signal to others that they shouldn't use it anymore.
> 
> ListenSocket came from StreamListenSocket which it inherits. Yes, my intention
> is to remove it eventually once http server refactoring is done.
> 
> @Matt, do you mind to replace the name with DeprecatedUnixDomainSocket?

Rather than call the class deprecated, maybe stick it in the net::deprecated
namespace?  We use that elsewhere.

And while it's not clear what a "ListenSocket" is, I think we should probably
just keep the name, due to the StreamListenSocket class (Which, yes, is a
horrible name, I suppose we could rename that, too...But that seems well beyond
the scope of this CL).

https://codereview.chromium.org/376323002/diff/260001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/260001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:371:
EXPECT_EQ(kSmallReadBufferSize, read_callback.WaitForResult());
On 2014/07/23 22:27:11, byungchul wrote:
> On 2014/07/23 18:52:40, mmenke wrote:
> > Just out of paranoia, suggest allowing any read > 0 and <=
> kSmallReadBufferSize,
> > and the next read call can check how much was actually read.
> 
> Though I don't know what you want exactly, changed the unittests to read one
> more small amount data.

Sorry, you're not understanding me.  I'm again concerned about lack of read
length guarantees.  What you should do is have:

int rv = read_callback.WaitForResult();
EXPECT_LT(0, rv);
EXPECT_LE(rv, kWriteDataSize);

EXPECT_EQ(kExpectedRemainingDataSize,
          ReadSynchronously(accepted_socket.get(),
                            read_buffer,
                            kReadBufferSize,
                            kSmallReadBufferSize - rv);

(And get rid of the stuff you added).

[byungchul, 2014-07-24 00:30:22]

Moved UnixDomainListenSocket into net::deprecated namespace.

https://codereview.chromium.org/376323002/diff/260001/net/socket/unix_domain_...
File net/socket/unix_domain_client_socket_posix_unittest.cc (right):

https://codereview.chromium.org/376323002/diff/260001/net/socket/unix_domain_...
net/socket/unix_domain_client_socket_posix_unittest.cc:371:
EXPECT_EQ(kSmallReadBufferSize, read_callback.WaitForResult());
On 2014/07/23 23:50:50, mmenke wrote:
> On 2014/07/23 22:27:11, byungchul wrote:
> > On 2014/07/23 18:52:40, mmenke wrote:
> > > Just out of paranoia, suggest allowing any read > 0 and <=
> > kSmallReadBufferSize,
> > > and the next read call can check how much was actually read.
> > 
> > Though I don't know what you want exactly, changed the unittests to read one
> > more small amount data.
> 
> Sorry, you're not understanding me.  I'm again concerned about lack of read
> length guarantees.  What you should do is have:
> 
> int rv = read_callback.WaitForResult();
> EXPECT_LT(0, rv);
> EXPECT_LE(rv, kWriteDataSize);
> 
> EXPECT_EQ(kExpectedRemainingDataSize,
>           ReadSynchronously(accepted_socket.get(),
>                             read_buffer,
>                             kReadBufferSize,
>                             kSmallReadBufferSize - rv);
> 
> (And get rid of the stuff you added).

Done.

[byungchul, 2014-07-24 00:31:52]

The CQ bit was checked by byungchul@chromium.org

[commit-bot: I haz the power, 2014-07-24 00:33:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/byungchul@chromium.org/376323002/280001

[commit-bot: I haz the power, 2014-07-24 03:51:36]

Change committed as 285148