Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
509133002:
Raw SocketDescriptor variant of UnixDomainServerSocket::Accept (Closed)
|
Created:
6 years, 3 months ago by Chris Masone Modified:
6 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRaw SocketDescriptor variant of UnixDomainServerSocket::Accept
The Mojo code on CrOS needs to accept inbound connections
on a unix domain socket, and then 'promote' the resulting
sockets to Mojo MessagePipes. This really requires access
to the underying file descriptor, so provide a mechanism
to accept a connection and get back a SocketDescriptor.
BUG=407782
TEST=UnixDomain*SocketTest
R=mmenke@chromium.org
Committed: https://crrev.com/ca100d5970b0d0b9a3af96d180f5ea2862227a48
Cr-Commit-Position: refs/heads/master@{#293172}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address byungchul's comments #Patch Set 3 : Add ReleaseSocketDescriptor to UnixDomainClientSocket class #
Total comments: 5
Patch Set 4 : Address nits; waiting on mmenke for broader comments. #
Total comments: 13
Patch Set 5 : Fix usage of Close(), add a read() to unittest #
Messages
Total messages: 26 (1 generated)
cmasone@chromium.org changed reviewers: + byungchul@chromium.org
The unittests for this work locally, and I'm running trybots now. Suggestions for other tests to run and/or a recommendation that I do something completely different are welcome :-)
Though I am fine with this approach, what about introducing "SocketLibevent* socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in UnixDomainClientSocket? https://codereview.chromium.org/509133002/diff/1/net/socket/socket_libevent.cc File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/509133002/diff/1/net/socket/socket_libevent.c... net/socket/socket_libevent.cc:111: tmp_fd = socket_fd_; why not just "SocketDescriptor tmp_fd = socket_fd_;"? https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... File net/socket/unix_domain_client_socket_posix_unittest.cc (right): https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:163: EXPECT_EQ(accepted_socket, kInvalidSocket); expected value should go first. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:171: EXPECT_EQ(accepted_socket, kInvalidSocket); ditto https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:174: EXPECT_GT(accepted_socket, 0); EXPECT_NE(kInvalidSocket, accepted_socket) because we have no assumption on socket fd except kInvalidSocket. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... net/socket/unix_domain_server_socket_posix.h:62: // Accept an incoming connection on |listen_socket_|, but pass back s/Accept/Accepts, s/pass/passes https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... net/socket/unix_domain_server_socket_posix.h:63: // a raw SocketDescriptor instead of a a StreamSocket. s/a a/a
On 2014/08/27 23:35:51, byungchul wrote: > Though I am fine with this approach, what about introducing "SocketLibevent* > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > UnixDomainClientSocket? I prefer this approach - really not a big fan of casting if we can avoid it (Note that we're returning a StreamSocket, not a UnixDomainClientSocket), and I'd also rather not modify the StreamSocket interface to allow passing a SocketDescriptor, because they're rather different beasts on Windows.
On 2014/08/28 00:15:01, mmenke wrote: > On 2014/08/27 23:35:51, byungchul wrote: > > Though I am fine with this approach, what about introducing "SocketLibevent* > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > UnixDomainClientSocket? > > I prefer this approach - really not a big fan of casting if we can avoid it > (Note that we're returning a StreamSocket, not a UnixDomainClientSocket), and > I'd also rather not modify the StreamSocket interface to allow passing a > SocketDescriptor, because they're rather different beasts on Windows. Cool. As I continue with the work for the other side of this connection, though, I've found myself in a similar bind. I need to connect to the socket...which should use UnixDomainClientSocket, and now I have the same problem :-) I tried using SocketLibevent directly, because I really just need to Open() and Connect(), but that's not part of the net module's exposed API. I wouldn't need to cast there, so I could add an API to UnixDomainClientSocket for releasing the socket descriptor as well. I'm not super happy with that. Is there a reason to keep SocketLibevent internal to the net module?
Anyone have an idea about how to address the issue I mentioned in my previous response, with the other side of this connection? https://codereview.chromium.org/509133002/diff/1/net/socket/socket_libevent.cc File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/509133002/diff/1/net/socket/socket_libevent.c... net/socket/socket_libevent.cc:111: tmp_fd = socket_fd_; On 2014/08/27 23:35:51, byungchul wrote: > why not just "SocketDescriptor tmp_fd = socket_fd_;"? ...I have no idea :-) https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... File net/socket/unix_domain_client_socket_posix_unittest.cc (right): https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:163: EXPECT_EQ(accepted_socket, kInvalidSocket); On 2014/08/27 23:35:51, byungchul wrote: > expected value should go first. Done. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:171: EXPECT_EQ(accepted_socket, kInvalidSocket); On 2014/08/27 23:35:51, byungchul wrote: > ditto Done. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_clien... net/socket/unix_domain_client_socket_posix_unittest.cc:174: EXPECT_GT(accepted_socket, 0); On 2014/08/27 23:35:51, byungchul wrote: > EXPECT_NE(kInvalidSocket, accepted_socket) because we have no assumption on > socket fd except kInvalidSocket. Done. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... net/socket/unix_domain_server_socket_posix.h:62: // Accept an incoming connection on |listen_socket_|, but pass back On 2014/08/27 23:35:51, byungchul wrote: > s/Accept/Accepts, s/pass/passes Done. https://codereview.chromium.org/509133002/diff/1/net/socket/unix_domain_serve... net/socket/unix_domain_server_socket_posix.h:63: // a raw SocketDescriptor instead of a a StreamSocket. On 2014/08/27 23:35:51, byungchul wrote: > s/a a/a Done.
On 2014/08/28 00:30:45, Chris Masone wrote: > On 2014/08/28 00:15:01, mmenke wrote: > > On 2014/08/27 23:35:51, byungchul wrote: > > > Though I am fine with this approach, what about introducing "SocketLibevent* > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > UnixDomainClientSocket? > > > > I prefer this approach - really not a big fan of casting if we can avoid it > > (Note that we're returning a StreamSocket, not a UnixDomainClientSocket), and > > I'd also rather not modify the StreamSocket interface to allow passing a > > SocketDescriptor, because they're rather different beasts on Windows. > > Cool. > > As I continue with the work for the other side of this connection, though, I've > found myself in a similar bind. I need to connect to the socket...which should > use UnixDomainClientSocket, and now I have the same problem :-) > > I tried using SocketLibevent directly, because I really just need to Open() and > Connect(), but that's not part of the net module's exposed API. I wouldn't need > to cast there, so I could add an API to UnixDomainClientSocket for releasing the > socket descriptor as well. I'm not super happy with that. Is there a reason to > keep SocketLibevent internal to the net module? The main reason is to have the same public API on all platforms (Admittedly, the very existence UnixDomainSocket rather breaks that). I do think it's a little weird to use one socket wrapper to connect a socket, and then another to actually read/write to it (I think it makes a bit more sense with the server socket, where one socket is managed by the wrapper, and the other by something else).
On 2014/08/28 19:33:03, mmenke wrote: > On 2014/08/28 00:30:45, Chris Masone wrote: > > On 2014/08/28 00:15:01, mmenke wrote: > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > Though I am fine with this approach, what about introducing > "SocketLibevent* > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > UnixDomainClientSocket? > > > > > > I prefer this approach - really not a big fan of casting if we can avoid it > > > (Note that we're returning a StreamSocket, not a UnixDomainClientSocket), > and > > > I'd also rather not modify the StreamSocket interface to allow passing a > > > SocketDescriptor, because they're rather different beasts on Windows. > > > > Cool. > > > > As I continue with the work for the other side of this connection, though, > I've > > found myself in a similar bind. I need to connect to the socket...which should > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > I tried using SocketLibevent directly, because I really just need to Open() > and > > Connect(), but that's not part of the net module's exposed API. I wouldn't > need > > to cast there, so I could add an API to UnixDomainClientSocket for releasing > the > > socket descriptor as well. I'm not super happy with that. Is there a reason to > > keep SocketLibevent internal to the net module? > > The main reason is to have the same public API on all platforms (Admittedly, the > very existence UnixDomainSocket rather breaks that). I do think it's a little > weird to use one socket wrapper to connect a socket, and then another to > actually read/write to it (I think it makes a bit more sense with the server > socket, where one socket is managed by the wrapper, and the other by something > else). That's fair. So...should I add a method to UnixDomainClientSocket to extract the underlying SocketLibevent, and from there use the API added here to extract the FD? I still need to wind up with the FD at the end of the day to craft a Mojo MessagePipe :-/
On 2014/08/28 20:10:01, Chris Masone wrote: > On 2014/08/28 19:33:03, mmenke wrote: > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > Though I am fine with this approach, what about introducing > > "SocketLibevent* > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > UnixDomainClientSocket? > > > > > > > > I prefer this approach - really not a big fan of casting if we can avoid > it > > > > (Note that we're returning a StreamSocket, not a UnixDomainClientSocket), > > and > > > > I'd also rather not modify the StreamSocket interface to allow passing a > > > > SocketDescriptor, because they're rather different beasts on Windows. > > > > > > Cool. > > > > > > As I continue with the work for the other side of this connection, though, > > I've > > > found myself in a similar bind. I need to connect to the socket...which > should > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > I tried using SocketLibevent directly, because I really just need to Open() > > and > > > Connect(), but that's not part of the net module's exposed API. I wouldn't > > need > > > to cast there, so I could add an API to UnixDomainClientSocket for releasing > > the > > > socket descriptor as well. I'm not super happy with that. Is there a reason > to > > > keep SocketLibevent internal to the net module? > > > > The main reason is to have the same public API on all platforms (Admittedly, > the > > very existence UnixDomainSocket rather breaks that). I do think it's a little > > weird to use one socket wrapper to connect a socket, and then another to > > actually read/write to it (I think it makes a bit more sense with the server > > socket, where one socket is managed by the wrapper, and the other by something > > else). > > That's fair. So...should I add a method to UnixDomainClientSocket to extract the > underlying SocketLibevent, and from there use the API added here to extract the > FD? I still need to wind up with the FD at the end of the day to craft a Mojo > MessagePipe :-/ I don't suppose the MessagePipe could just wrap the client socket?
On 2014/08/28 20:21:24, mmenke wrote: > On 2014/08/28 20:10:01, Chris Masone wrote: > > On 2014/08/28 19:33:03, mmenke wrote: > > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > > Though I am fine with this approach, what about introducing > > > "SocketLibevent* > > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > > UnixDomainClientSocket? > > > > > > > > > > I prefer this approach - really not a big fan of casting if we can avoid > > it > > > > > (Note that we're returning a StreamSocket, not a > UnixDomainClientSocket), > > > and > > > > > I'd also rather not modify the StreamSocket interface to allow passing a > > > > > SocketDescriptor, because they're rather different beasts on Windows. > > > > > > > > Cool. > > > > > > > > As I continue with the work for the other side of this connection, though, > > > I've > > > > found myself in a similar bind. I need to connect to the socket...which > > should > > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > > > I tried using SocketLibevent directly, because I really just need to > Open() > > > and > > > > Connect(), but that's not part of the net module's exposed API. I wouldn't > > > need > > > > to cast there, so I could add an API to UnixDomainClientSocket for > releasing > > > the > > > > socket descriptor as well. I'm not super happy with that. Is there a > reason > > to > > > > keep SocketLibevent internal to the net module? > > > > > > The main reason is to have the same public API on all platforms (Admittedly, > > the > > > very existence UnixDomainSocket rather breaks that). I do think it's a > little > > > weird to use one socket wrapper to connect a socket, and then another to > > > actually read/write to it (I think it makes a bit more sense with the server > > > socket, where one socket is managed by the wrapper, and the other by > something > > > else). > > > > That's fair. So...should I add a method to UnixDomainClientSocket to extract > the > > underlying SocketLibevent, and from there use the API added here to extract > the > > FD? I still need to wind up with the FD at the end of the day to craft a Mojo > > MessagePipe :-/ > > I don't suppose the MessagePipe could just wrap the client socket? It can't, no. The file descriptors underlying MessagePipes get handed across processes often, so the code would ultimately need to get at the FD anyway. I'm happy to figure out another way to factor out the Open/Connect code if you can help me determine something less ugly.
On 2014/08/28 20:35:25, Chris Masone wrote: > On 2014/08/28 20:21:24, mmenke wrote: > > On 2014/08/28 20:10:01, Chris Masone wrote: > > > On 2014/08/28 19:33:03, mmenke wrote: > > > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > > > Though I am fine with this approach, what about introducing > > > > "SocketLibevent* > > > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > > > UnixDomainClientSocket? > > > > > > > > > > > > I prefer this approach - really not a big fan of casting if we can > avoid > > > it > > > > > > (Note that we're returning a StreamSocket, not a > > UnixDomainClientSocket), > > > > and > > > > > > I'd also rather not modify the StreamSocket interface to allow passing > a > > > > > > SocketDescriptor, because they're rather different beasts on Windows. > > > > > > > > > > Cool. > > > > > > > > > > As I continue with the work for the other side of this connection, > though, > > > > I've > > > > > found myself in a similar bind. I need to connect to the socket...which > > > should > > > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > > > > > I tried using SocketLibevent directly, because I really just need to > > Open() > > > > and > > > > > Connect(), but that's not part of the net module's exposed API. I > wouldn't > > > > need > > > > > to cast there, so I could add an API to UnixDomainClientSocket for > > releasing > > > > the > > > > > socket descriptor as well. I'm not super happy with that. Is there a > > reason > > > to > > > > > keep SocketLibevent internal to the net module? > > > > > > > > The main reason is to have the same public API on all platforms > (Admittedly, > > > the > > > > very existence UnixDomainSocket rather breaks that). I do think it's a > > little > > > > weird to use one socket wrapper to connect a socket, and then another to > > > > actually read/write to it (I think it makes a bit more sense with the > server > > > > socket, where one socket is managed by the wrapper, and the other by > > something > > > > else). > > > > > > That's fair. So...should I add a method to UnixDomainClientSocket to extract > > the > > > underlying SocketLibevent, and from there use the API added here to extract > > the > > > FD? I still need to wind up with the FD at the end of the day to craft a > Mojo > > > MessagePipe :-/ > > > > I don't suppose the MessagePipe could just wrap the client socket? > > It can't, no. The file descriptors underlying MessagePipes get handed across > processes often, so the code would ultimately need to get at the FD anyway. > > I'm happy to figure out another way to factor out the Open/Connect code if you > can help me determine something less ugly. Any thoughts here?
On 2014/08/29 20:42:32, Chris Masone wrote: > On 2014/08/28 20:35:25, Chris Masone wrote: > > On 2014/08/28 20:21:24, mmenke wrote: > > > On 2014/08/28 20:10:01, Chris Masone wrote: > > > > On 2014/08/28 19:33:03, mmenke wrote: > > > > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > > > > Though I am fine with this approach, what about introducing > > > > > "SocketLibevent* > > > > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > > > > UnixDomainClientSocket? > > > > > > > > > > > > > > I prefer this approach - really not a big fan of casting if we can > > avoid > > > > it > > > > > > > (Note that we're returning a StreamSocket, not a > > > UnixDomainClientSocket), > > > > > and > > > > > > > I'd also rather not modify the StreamSocket interface to allow > passing > > a > > > > > > > SocketDescriptor, because they're rather different beasts on > Windows. > > > > > > > > > > > > Cool. > > > > > > > > > > > > As I continue with the work for the other side of this connection, > > though, > > > > > I've > > > > > > found myself in a similar bind. I need to connect to the > socket...which > > > > should > > > > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > > > > > > > I tried using SocketLibevent directly, because I really just need to > > > Open() > > > > > and > > > > > > Connect(), but that's not part of the net module's exposed API. I > > wouldn't > > > > > need > > > > > > to cast there, so I could add an API to UnixDomainClientSocket for > > > releasing > > > > > the > > > > > > socket descriptor as well. I'm not super happy with that. Is there a > > > reason > > > > to > > > > > > keep SocketLibevent internal to the net module? > > > > > > > > > > The main reason is to have the same public API on all platforms > > (Admittedly, > > > > the > > > > > very existence UnixDomainSocket rather breaks that). I do think it's a > > > little > > > > > weird to use one socket wrapper to connect a socket, and then another to > > > > > actually read/write to it (I think it makes a bit more sense with the > > server > > > > > socket, where one socket is managed by the wrapper, and the other by > > > something > > > > > else). > > > > > > > > That's fair. So...should I add a method to UnixDomainClientSocket to > extract > > > the > > > > underlying SocketLibevent, and from there use the API added here to > extract > > > the > > > > FD? I still need to wind up with the FD at the end of the day to craft a > > Mojo > > > > MessagePipe :-/ > > > > > > I don't suppose the MessagePipe could just wrap the client socket? > > > > It can't, no. The file descriptors underlying MessagePipes get handed across > > processes often, so the code would ultimately need to get at the FD anyway. > > > > I'm happy to figure out another way to factor out the Open/Connect code if you > > can help me determine something less ugly. > > Any thoughts here? In my opinion, UnixDomainClientSocket::ReleaseSocketDescriptor() with downcast in accept callback is the simplest and cleanest way. But, Matt has the owner, and you should wait for his advice. Anyway, please beware that the socket fd is set non-blocking on IO operations.
On 2014/08/29 20:48:36, byungchul wrote: > On 2014/08/29 20:42:32, Chris Masone wrote: > > On 2014/08/28 20:35:25, Chris Masone wrote: > > > On 2014/08/28 20:21:24, mmenke wrote: > > > > On 2014/08/28 20:10:01, Chris Masone wrote: > > > > > On 2014/08/28 19:33:03, mmenke wrote: > > > > > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > > > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > > > > > Though I am fine with this approach, what about introducing > > > > > > "SocketLibevent* > > > > > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > > > > > UnixDomainClientSocket? > > > > > > > > > > > > > > > > I prefer this approach - really not a big fan of casting if we can > > > avoid > > > > > it > > > > > > > > (Note that we're returning a StreamSocket, not a > > > > UnixDomainClientSocket), > > > > > > and > > > > > > > > I'd also rather not modify the StreamSocket interface to allow > > passing > > > a > > > > > > > > SocketDescriptor, because they're rather different beasts on > > Windows. > > > > > > > > > > > > > > Cool. > > > > > > > > > > > > > > As I continue with the work for the other side of this connection, > > > though, > > > > > > I've > > > > > > > found myself in a similar bind. I need to connect to the > > socket...which > > > > > should > > > > > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > > > > > > > > > I tried using SocketLibevent directly, because I really just need to > > > > Open() > > > > > > and > > > > > > > Connect(), but that's not part of the net module's exposed API. I > > > wouldn't > > > > > > need > > > > > > > to cast there, so I could add an API to UnixDomainClientSocket for > > > > releasing > > > > > > the > > > > > > > socket descriptor as well. I'm not super happy with that. Is there a > > > > reason > > > > > to > > > > > > > keep SocketLibevent internal to the net module? > > > > > > > > > > > > The main reason is to have the same public API on all platforms > > > (Admittedly, > > > > > the > > > > > > very existence UnixDomainSocket rather breaks that). I do think it's > a > > > > little > > > > > > weird to use one socket wrapper to connect a socket, and then another > to > > > > > > actually read/write to it (I think it makes a bit more sense with the > > > server > > > > > > socket, where one socket is managed by the wrapper, and the other by > > > > something > > > > > > else). > > > > > > > > > > That's fair. So...should I add a method to UnixDomainClientSocket to > > extract > > > > the > > > > > underlying SocketLibevent, and from there use the API added here to > > extract > > > > the > > > > > FD? I still need to wind up with the FD at the end of the day to craft a > > > Mojo > > > > > MessagePipe :-/ > > > > > > > > I don't suppose the MessagePipe could just wrap the client socket? > > > > > > It can't, no. The file descriptors underlying MessagePipes get handed across > > > processes often, so the code would ultimately need to get at the FD anyway. > > > > > > I'm happy to figure out another way to factor out the Open/Connect code if > you > > > can help me determine something less ugly. > > > > Any thoughts here? > > In my opinion, UnixDomainClientSocket::ReleaseSocketDescriptor() with downcast > in accept callback is the simplest and cleanest way. > But, Matt has the owner, and you should wait for his advice. > Anyway, please beware that the socket fd is set non-blocking on IO operations. Matt? Thoughts? I'm pretty well blocked on this.
On 2014/09/02 16:52:38, Chris Masone wrote: > On 2014/08/29 20:48:36, byungchul wrote: > > On 2014/08/29 20:42:32, Chris Masone wrote: > > > On 2014/08/28 20:35:25, Chris Masone wrote: > > > > On 2014/08/28 20:21:24, mmenke wrote: > > > > > On 2014/08/28 20:10:01, Chris Masone wrote: > > > > > > On 2014/08/28 19:33:03, mmenke wrote: > > > > > > > On 2014/08/28 00:30:45, Chris Masone wrote: > > > > > > > > On 2014/08/28 00:15:01, mmenke wrote: > > > > > > > > > On 2014/08/27 23:35:51, byungchul wrote: > > > > > > > > > > Though I am fine with this approach, what about introducing > > > > > > > "SocketLibevent* > > > > > > > > > > socket()", or "SocketDescriptor ReleaseSocketDescriptor()" in > > > > > > > > > > UnixDomainClientSocket? > > > > > > > > > > > > > > > > > > I prefer this approach - really not a big fan of casting if we > can > > > > avoid > > > > > > it > > > > > > > > > (Note that we're returning a StreamSocket, not a > > > > > UnixDomainClientSocket), > > > > > > > and > > > > > > > > > I'd also rather not modify the StreamSocket interface to allow > > > passing > > > > a > > > > > > > > > SocketDescriptor, because they're rather different beasts on > > > Windows. > > > > > > > > > > > > > > > > Cool. > > > > > > > > > > > > > > > > As I continue with the work for the other side of this connection, > > > > though, > > > > > > > I've > > > > > > > > found myself in a similar bind. I need to connect to the > > > socket...which > > > > > > should > > > > > > > > use UnixDomainClientSocket, and now I have the same problem :-) > > > > > > > > > > > > > > > > I tried using SocketLibevent directly, because I really just need > to > > > > > Open() > > > > > > > and > > > > > > > > Connect(), but that's not part of the net module's exposed API. I > > > > wouldn't > > > > > > > need > > > > > > > > to cast there, so I could add an API to UnixDomainClientSocket for > > > > > releasing > > > > > > > the > > > > > > > > socket descriptor as well. I'm not super happy with that. Is there > a > > > > > reason > > > > > > to > > > > > > > > keep SocketLibevent internal to the net module? > > > > > > > > > > > > > > The main reason is to have the same public API on all platforms > > > > (Admittedly, > > > > > > the > > > > > > > very existence UnixDomainSocket rather breaks that). I do think > it's > > a > > > > > little > > > > > > > weird to use one socket wrapper to connect a socket, and then > another > > to > > > > > > > actually read/write to it (I think it makes a bit more sense with > the > > > > server > > > > > > > socket, where one socket is managed by the wrapper, and the other by > > > > > something > > > > > > > else). > > > > > > > > > > > > That's fair. So...should I add a method to UnixDomainClientSocket to > > > extract > > > > > the > > > > > > underlying SocketLibevent, and from there use the API added here to > > > extract > > > > > the > > > > > > FD? I still need to wind up with the FD at the end of the day to craft > a > > > > Mojo > > > > > > MessagePipe :-/ > > > > > > > > > > I don't suppose the MessagePipe could just wrap the client socket? > > > > > > > > It can't, no. The file descriptors underlying MessagePipes get handed > across > > > > processes often, so the code would ultimately need to get at the FD > anyway. > > > > > > > > I'm happy to figure out another way to factor out the Open/Connect code if > > you > > > > can help me determine something less ugly. > > > > > > Any thoughts here? > > > > In my opinion, UnixDomainClientSocket::ReleaseSocketDescriptor() with downcast > > in accept callback is the simplest and cleanest way. > > But, Matt has the owner, and you should wait for his advice. > > Anyway, please beware that the socket fd is set non-blocking on IO operations. > > Matt? Thoughts? I'm pretty well blocked on this. Sorry, took Friday off, and accumulated 10+ reviews over the last 4 days...Fun! Anyways, yea, I think adding a method to UnixDomainClientSocket is the way to go.
> > Sorry, took Friday off, and accumulated 10+ reviews over the last 4 days...Fun! > > Anyways, yea, I think adding a method to UnixDomainClientSocket is the way to > go. Thanks for all the reviews, folks. PTAL.
https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.cc:163: if (socket_) { Fast return if !socket_ https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:68: // destroyed in the process, as it no longer owns the underlying FD. Not sure if it is good to comment with private member, socket_. https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.h:65: const CompletionCallback& callback); Do we still prefer it to downcast in the callback?
Addressed nits; I'll let mmenke speak to the down-cast issue. https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.cc:163: if (socket_) { On 2014/09/03 00:13:54, byungchul wrote: > Fast return if !socket_ Done. https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/40001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:68: // destroyed in the process, as it no longer owns the underlying FD. On 2014/09/03 00:13:54, byungchul wrote: > Not sure if it is good to comment with private member, socket_. Done.
I'm generally very strongly against casting, perhaps irrationally so, unless it's really needed, and in this case, I don't think it is. Too bad C++ doesn't support more specific implementations of inherited functions (Yes this is StreamSocket, but if you have one of this class, it's a UnixDomainClientSocket, too!), though with scoped pointers, suppose that wouldn't work, anyways. Anyhow, I prefer this approach, as I think it's more robust against regressions. https://codereview.chromium.org/509133002/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:112: Close(); This seems like a misuse of Close, since we just want the stop watching and cleanup parts of it...i.e., everything *but* actually closing the socket. Maybe add a function StopWatchingAndCleanup(), and make both this and Close() use it? https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.cc:166: SocketDescriptor socket_fd = socket_->ReleaseConnectedSocket(); Should use a consistent temp name here and in ReleaseConnectedSocket. And method names, for that matter. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:69: // Returns kInvalidSocket if this object is not connected. This isn't true. A listening socket isn't connected, but will be returned. The same is true of a connecting socket. Suggest DCHECKing in both of these cases, and explicitly disallowing them in the comment. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix_unittest.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix_unittest.cc:177: SocketDescriptor client_socket_fd = client_socket.ReleaseSocketDescriptor(); Can we wrap this in a UnixDomainClientSocket (And a TcpClientSocketLibEvent) and try reading something from it? Closing should return an error if it's already closed, but I'm very paranoid about regressions here. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:30: } // anonymous nanmespace nit: -anonymous, nanmespace -> namespace https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:30: } // anonymous nanmespace nit: I believe blank lines before end and after start of namespace are more common, at least in net/, when you have more than just a couple variables.
https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix_unittest.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix_unittest.cc:177: SocketDescriptor client_socket_fd = client_socket.ReleaseSocketDescriptor(); On 2014/09/03 14:56:46, mmenke wrote: > Can we wrap this in a UnixDomainClientSocket (And a TcpClientSocketLibEvent) and > try reading something from it? Closing should return an error if it's already > closed, but I'm very paranoid about regressions here. TclClientSocketLibEvent? This is the wrong kind of FD to be injected into that class, no?
On 2014/09/03 15:03:41, Chris Masone wrote: > https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... > File net/socket/unix_domain_client_socket_posix_unittest.cc (right): > > https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... > net/socket/unix_domain_client_socket_posix_unittest.cc:177: SocketDescriptor > client_socket_fd = client_socket.ReleaseSocketDescriptor(); > On 2014/09/03 14:56:46, mmenke wrote: > > Can we wrap this in a UnixDomainClientSocket (And a TcpClientSocketLibEvent) > and > > try reading something from it? Closing should return an error if it's already > > closed, but I'm very paranoid about regressions here. > > TclClientSocketLibEvent? This is the wrong kind of FD to be injected into that > class, no? Sorry, SocketLibevent
https://codereview.chromium.org/509133002/diff/60001/net/socket/socket_libeve... File net/socket/socket_libevent.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/socket_libeve... net/socket/socket_libevent.cc:112: Close(); On 2014/09/03 14:56:46, mmenke wrote: > This seems like a misuse of Close, since we just want the stop watching and > cleanup parts of it...i.e., everything *but* actually closing the socket. Maybe > add a function StopWatchingAndCleanup(), and make both this and Close() use it? Done. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.cc:166: SocketDescriptor socket_fd = socket_->ReleaseConnectedSocket(); On 2014/09/03 14:56:46, mmenke wrote: > Should use a consistent temp name here and in ReleaseConnectedSocket. And > method names, for that matter. Done. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix.h (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix.h:69: // Returns kInvalidSocket if this object is not connected. On 2014/09/03 14:56:46, mmenke wrote: > This isn't true. A listening socket isn't connected, but will be returned. The > same is true of a connecting socket. > > Suggest DCHECKing in both of these cases, and explicitly disallowing them in the > comment. The code will now DCHECK if the underlying SocketLibevent doesn't exist or returns false from IsConnected(), which I think has the semantics you're looking for. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... File net/socket/unix_domain_client_socket_posix_unittest.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_c... net/socket/unix_domain_client_socket_posix_unittest.cc:177: SocketDescriptor client_socket_fd = client_socket.ReleaseSocketDescriptor(); On 2014/09/03 15:03:40, Chris Masone wrote: > On 2014/09/03 14:56:46, mmenke wrote: > > Can we wrap this in a UnixDomainClientSocket (And a TcpClientSocketLibEvent) > and > > try reading something from it? Closing should return an error if it's already > > closed, but I'm very paranoid about regressions here. > > TclClientSocketLibEvent? This is the wrong kind of FD to be injected into that > class, no? Ok, I've done a read() and made sure I got ERR_IO_PENDING and not EOF, which i think verifies what you're looking for. I had to add NET_EXPORT_PRIVATE to SocketLibevent to use it here, though. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... File net/socket/unix_domain_server_socket_posix.cc (right): https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:30: } // anonymous nanmespace On 2014/09/03 14:56:46, mmenke wrote: > nit: -anonymous, nanmespace -> namespace Done. https://codereview.chromium.org/509133002/diff/60001/net/socket/unix_domain_s... net/socket/unix_domain_server_socket_posix.cc:30: } // anonymous nanmespace On 2014/09/03 14:56:46, mmenke wrote: > nit: -anonymous, nanmespace -> namespace Done.
LGTM
The CQ bit was checked by cmasone@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cmasone@chromium.org/509133002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as b5a2ab003bd62b4c48206ecfef05694984747320
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ca100d5970b0d0b9a3af96d180f5ea2862227a48 Cr-Commit-Position: refs/heads/master@{#293172} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
