Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139009:
Tests for socks4/4a implementation. (Closed)
|
Created:
11 years, 6 months ago by Arindam Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review) Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionTests for socks4/4a implementation.
Refactoring of void BuildHandshakeWriteBuffer() to const std::string BuildHandshakeWriteBuffer() const,and removing private members handshake_buf_len_ and buffer_len_ (since buffer_ is now std::string, buffer_.size()) is more than sufficient for buffer size.
TEST=unittests
BUG=469
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19474
Patch Set 1 #Patch Set 2 : '' #
Total comments: 9
Patch Set 3 : passing HostResolver as a constructor argument #Patch Set 4 : adding testcase when hostname DNS resolves to IPv6 #
Total comments: 2
Patch Set 5 : changing to mock sockets, adding more tests #
Total comments: 42
Patch Set 6 : no more IO_PENDING loops #
Total comments: 15
Patch Set 7 : snapshot #Patch Set 8 : refactoring #
Total comments: 21
Patch Set 9 : fixed FailedSocketRead test #
Total comments: 4
Patch Set 10 : '' #Patch Set 11 : repo-change fix #Patch Set 12 : uint fix #Patch Set 13 : scoped_refptr #
Messages
Total messages: 18 (0 generated)
Drive by style nits. Not doing a full review, letting eroman take care of that. Cool to see you sending out patches! http://codereview.chromium.org/139009/diff/3/1003 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3/1003#newcode11 Line 11: #include "net/base/socks_client_socket.h" It's not obvious from the style guide that "foo_unittest.cc" should include "foo.h" first, but if you expand the summary, you'll see the example of "base/basictypes_unittest.cc" including "base/basictypes.h" first. Please #include net/base/socks_client_socket.h first. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/139009/diff/3/1003#newcode25 Line 25: typedef std::pair<const char*, int> sized_buffer; This should be CamelCasedType instead of unix_hacker_style_type http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N.... http://codereview.chromium.org/139009/diff/3/1003#newcode134 Line 134: EXPECT_EQ(rv, ERR_IO_PENDING); Your EXPECT_EQ() usage is reversed. It should be EXPECT_EQ(expected, actual). http://codereview.chromium.org/139009/diff/3/1003#newcode270 Line 270: // EXPECT_EQ(user_sock_->socks_version_, Is this supposed to be commented out? It looks like this is the main thing that the test is asserting.
Added another test for a case when the hostname DNS resolves to IPv6 http://codereview.chromium.org/139009/diff/3/1003 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3/1003#newcode11 Line 11: #include "net/base/socks_client_socket.h" On 2009/06/19 21:04:54, willchan wrote: > It's not obvious from the style guide that "foo_unittest.cc" should include > "foo.h" first, but if you expand the summary, you'll see the example of > "base/basictypes_unittest.cc" including "base/basictypes.h" first. Please > #include net/base/socks_client_socket.h first. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... Done. http://codereview.chromium.org/139009/diff/3/1003#newcode25 Line 25: typedef std::pair<const char*, int> sized_buffer; On 2009/06/19 21:04:54, willchan wrote: > This should be CamelCasedType instead of unix_hacker_style_type > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N.... Done. http://codereview.chromium.org/139009/diff/3/1003#newcode134 Line 134: EXPECT_EQ(rv, ERR_IO_PENDING); there seems to be a lot of code using it the other way around. Although this makes sense since the failure codes are more human-readable, methinks. Done. http://codereview.chromium.org/139009/diff/3/1003#newcode270 Line 270: // EXPECT_EQ(user_sock_->socks_version_, Whether it connects or not successfully is an assertion. Nevertheless I was having compile issues when accessing |socks_version_| private member even when declaring it as a FRIEND_TEST in the header file.
http://codereview.chromium.org/139009/diff/3/1003 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3/1003#newcode270 Line 270: // EXPECT_EQ(user_sock_->socks_version_, On 2009/06/21 00:24:32, Arindam wrote: > Whether it connects or not successfully is an assertion. > Nevertheless I was having compile issues when accessing |socks_version_| private > member even when declaring it as a FRIEND_TEST in the header file. > > You can't friend something in an anonymous namespace. If you need to friend this test, you will have to pull it out of the anonymous namespace.
http://codereview.chromium.org/139009/diff/3003/3004 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3003/3004#newcode100 Line 100: sock = ListenSocket::Listen("127.0.0.1", port, this); Please use mock sockets instead of real sockets on the loopback interface. There are many advantages to using mock sockets over real sockets. I will enumerate a couple: With mock sockets, you can replay *exact* sequences of what Socket::Read() and Socket::Write() return. For example, you can (and in fact should) write a test for when it takes multiple calls to Socket::Read() to complete the handshake, or when the Read/Write complete synchronously, or when the transport socket fails at different points, etc. Also, the initialization sequence is simpler when using mock socket, since there isn't the worry of having to listen on a special port. The setup for using mock sockets for the transport socket, while a bit tedious, is going to be exactly the same as what you used in http_network_transaction_unittest.cc Here is the general structure of that boilerplate: MockRead data_reads[] = { // TODO: fill the mock result for each Socket::Read() MockRead(...), }; MockWrite data_writes[] = { // TODO: fill the mock result for each Socket::Write() MockWrite(...), }; StaticMockSocket data(data_reads, data_writes); MockClientSocketFactory mock_socket_factory; mock_socket_factory.AddMockSocket(&data); ClientSocket* mock_transport_socket = mock_socket_factory.CreateTCPClientSocket(AddressList()); // ========> The mock transport socket above is what we pass to the SOCKSClientSocket ctor.
I moved MockTCPClientSocket from socket_test_util.cc to socket_test_util.h so that it can be used directly. Also added PartialWriteTest for socks, along with some code-cleanup. http://codereview.chromium.org/139009/diff/3003/3004 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3003/3004#newcode100 Line 100: sock = ListenSocket::Listen("127.0.0.1", port, this); On 2009/06/21 03:30:12, eroman wrote: > Please use mock sockets instead of real sockets on the loopback interface. > > There are many advantages to using mock sockets over real sockets. I will > enumerate a couple: > > With mock sockets, you can replay *exact* sequences of what Socket::Read() and > Socket::Write() return. For example, you can (and in fact should) write a test > for when it takes multiple calls to Socket::Read() to complete the handshake, or > when the Read/Write complete synchronously, or when the transport socket fails > at different points, etc. > > Also, the initialization sequence is simpler when using mock socket, since there > isn't the worry of having to listen on a special port. > > The setup for using mock sockets for the transport socket, while a bit tedious, > is going to be exactly the same as what you used in > http_network_transaction_unittest.cc > > Here is the general structure of that boilerplate: > > MockRead data_reads[] = { > // TODO: fill the mock result for each Socket::Read() > MockRead(...), > }; > > MockWrite data_writes[] = { > // TODO: fill the mock result for each Socket::Write() > MockWrite(...), > }; > > StaticMockSocket data(data_reads, data_writes); > MockClientSocketFactory mock_socket_factory; > mock_socket_factory.AddMockSocket(&data); > > ClientSocket* mock_transport_socket = > mock_socket_factory.CreateTCPClientSocket(AddressList()); > > // ========> The mock transport socket above is what we pass to the > SOCKSClientSocket ctor. > > Done.
http://codereview.chromium.org/139009/diff/3006/3009 File net/base/socket_test_util.h (right): http://codereview.chromium.org/139009/diff/3006/3009#newcode207 Line 207: class MockClientSocket : public net::SSLClientSocket { I assume this is a pure move, so I only casually reviewed these diffs. http://codereview.chromium.org/139009/diff/3006/3007 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3006/3007#newcode5 Line 5: #include "net/base/socks_client_socket.h" I am afraid the net/base/*socket* files have been moved to net/socket. So the socks stuff should live there too. net/socket/socks_client_socket.h net/socket/socks_client_socket.cc net/socket/socks_client_socket_unittest.cc Sorry for that! See <http://codereview.chromium.org/144009> for the details. http://codereview.chromium.org/139009/diff/3006/3007#newcode47 Line 47: class SOCKSTestHostMapper : public net::HostMapper { Alternatively, you could use RuleBasedHostMapper (declared in host_resolver_unittest.h). See host_resolver_unittest.cc for some example usages. The general pattern is: scoped_refptr<RuleBasedHostMapper> mapper = new RuleBasedHostMapper; mapper->AddRule("an.ipv6.address", "2001:db8:8714:3a90::1200"); http://codereview.chromium.org/139009/diff/3006/3007#newcode69 Line 69: CHECK(rv == OK); Since we are in a test environment here, can use: EXPECT_EQ(OK, rv); instead. Or if you really want it to abort: ASSERT_EQ(OK, rv); http://codereview.chromium.org/139009/diff/3006/3007#newcode78 Line 78: const char* hostname = "localhost", google c++ style guide disallows default parameters. you will need to define them as required instead, and have each callsite pass the default (you could define the default as a constant to avoid repetition). also minor nit: i would suggest making the hostname parameter be a "const std::string&" http://codereview.chromium.org/139009/diff/3006/3007#newcode86 Line 86: while (rv != OK) { This doesn't really need to be in a while loop; the callback can be completed at most once. http://codereview.chromium.org/139009/diff/3006/3007#newcode87 Line 87: EXPECT_EQ(rv, ERR_IO_PENDING); flip this: EXPECT_EQ(ERR_IO_PENDING, rv); (expectation is listed first). http://codereview.chromium.org/139009/diff/3006/3007#newcode104 Line 104: MockWrite(true, kSOCKSOkRequest, arraysize(kSOCKSOkRequest)), indent continued lines by 4 spaces. http://codereview.chromium.org/139009/diff/3006/3007#newcode107 Line 107: MockRead(true, kSOCKSOkReply, arraysize(kSOCKSOkReply)), same as above.
Great start on the unit-tests. Including the second batch of comments. http://codereview.chromium.org/139009/diff/3006/3007 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3006/3007#newcode112 Line 112: // At this state the TCP connection is completed but not the socks socket. nit: "socks" --> SOCKS http://codereview.chromium.org/139009/diff/3006/3007#newcode117 Line 117: while (rv != OK) { similar comment to earlier -- the mock transport socket should behave consistently, so we can just check the expected situation and not have a loop. rv = user_sock_->Connect(..); EXPECT_EQ(ERR_IO_PENDING, rv); callback_.WaitForResult(); (I don't remember whether the default connects it synchronously or not). http://codereview.chromium.org/139009/diff/3006/3007#newcode129 Line 129: if (rv == ERR_IO_PENDING) same comment as above -- since this is a test, we want to hard-code against just one possible path. the test should complete one way each time. (so remove the "if"). http://codereview.chromium.org/139009/diff/3006/3007#newcode135 Line 135: if (rv == ERR_IO_PENDING) same comment as above. http://codereview.chromium.org/139009/diff/3006/3007#newcode149 Line 149: net::Error FailCode; We are in the "net::" namespace already, so "net::" prefix is unnecessary. Also, please use hacker_style_naming_for_variables http://codereview.chromium.org/139009/diff/3006/3007#newcode174 Line 174: while (rv == ERR_IO_PENDING) { see earlier comment. http://codereview.chromium.org/139009/diff/3006/3007#newcode198 Line 198: while (rv == ERR_IO_PENDING) { see earlier comment. http://codereview.chromium.org/139009/diff/3006/3007#newcode220 Line 220: while (rv == ERR_IO_PENDING) { see earlier comment. http://codereview.chromium.org/139009/diff/3006/3007#newcode235 Line 235: { arraysize(kSOCKSOkReply), ERR_INVALID_RESPONSE }, this value for reply_code doesn't make sense to me -- below |reply_code| is used as the synchronous error code to be returned from the transport socket during a read. So it should be a net error code (negative number). http://codereview.chromium.org/139009/diff/3006/3007#newcode253 Line 253: while (rv == ERR_IO_PENDING) { see earlier comment. http://codereview.chromium.org/139009/diff/3006/3007#newcode277 Line 277: while (rv == ERR_IO_PENDING) { See earlier comment. http://codereview.chromium.org/139009/diff/3006/3007#newcode305 Line 305: while (rv == ERR_IO_PENDING) { See earlier comment.
Finished with the changes suggested. also moved the unittest file around. http://codereview.chromium.org/139009/diff/3006/3009 File net/base/socket_test_util.h (right): http://codereview.chromium.org/139009/diff/3006/3009#newcode207 Line 207: class MockClientSocket : public net::SSLClientSocket { On 2009/06/23 01:06:26, eroman wrote: > I assume this is a pure move, so I only casually reviewed these diffs. Yes, it was a cut-paste. http://codereview.chromium.org/139009/diff/3006/3007 File net/base/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/3006/3007#newcode5 Line 5: #include "net/base/socks_client_socket.h" On 2009/06/23 01:06:26, eroman wrote: > I am afraid the net/base/*socket* files have been moved to net/socket. So the > socks stuff should live there too. > > net/socket/socks_client_socket.h > net/socket/socks_client_socket.cc > net/socket/socks_client_socket_unittest.cc > > Sorry for that! > See <http://codereview.chromium.org/144009> for the details. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode47 Line 47: class SOCKSTestHostMapper : public net::HostMapper { On 2009/06/23 01:06:26, eroman wrote: > Alternatively, you could use RuleBasedHostMapper (declared in > host_resolver_unittest.h). > > See host_resolver_unittest.cc for some example usages. > The general pattern is: > > scoped_refptr<RuleBasedHostMapper> mapper = new RuleBasedHostMapper; > > mapper->AddRule("an.ipv6.address", "2001:db8:8714:3a90::1200"); Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode69 Line 69: CHECK(rv == OK); On 2009/06/23 01:06:26, eroman wrote: > Since we are in a test environment here, can use: > EXPECT_EQ(OK, rv); > instead. > > Or if you really want it to abort: > > ASSERT_EQ(OK, rv); Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode78 Line 78: const char* hostname = "localhost", On 2009/06/23 01:06:26, eroman wrote: > google c++ style guide disallows default parameters. > you will need to define them as required instead, and have each callsite pass > the default (you could define the default as a constant to avoid repetition). > > also minor nit: i would suggest making the hostname parameter be a "const > std::string&" Done. (sad I could not do function overloading to specify default params) http://codereview.chromium.org/139009/diff/3006/3007#newcode86 Line 86: while (rv != OK) { On 2009/06/23 01:06:26, eroman wrote: > This doesn't really need to be in a while loop; the callback can be completed at > most once. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode87 Line 87: EXPECT_EQ(rv, ERR_IO_PENDING); On 2009/06/23 01:06:26, eroman wrote: > flip this: > > EXPECT_EQ(ERR_IO_PENDING, rv); > > (expectation is listed first). Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode104 Line 104: MockWrite(true, kSOCKSOkRequest, arraysize(kSOCKSOkRequest)), On 2009/06/23 01:06:26, eroman wrote: > indent continued lines by 4 spaces. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode107 Line 107: MockRead(true, kSOCKSOkReply, arraysize(kSOCKSOkReply)), On 2009/06/23 01:06:26, eroman wrote: > same as above. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode112 Line 112: // At this state the TCP connection is completed but not the socks socket. On 2009/06/23 01:23:03, eroman wrote: > nit: "socks" --> SOCKS Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode117 Line 117: while (rv != OK) { On 2009/06/23 01:23:03, eroman wrote: > similar comment to earlier -- the mock transport socket should behave > consistently, so we can just check the expected situation and not have a loop. > > rv = user_sock_->Connect(..); > EXPECT_EQ(ERR_IO_PENDING, rv); > callback_.WaitForResult(); > > (I don't remember whether the default connects it synchronously or not). > > Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode129 Line 129: if (rv == ERR_IO_PENDING) On 2009/06/23 01:23:03, eroman wrote: > same comment as above -- since this is a test, we want to hard-code against just > one possible path. the test should complete one way each time. > > (so remove the "if"). Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode135 Line 135: if (rv == ERR_IO_PENDING) On 2009/06/23 01:23:03, eroman wrote: > same comment as above. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode149 Line 149: net::Error FailCode; On 2009/06/23 01:23:03, eroman wrote: > We are in the "net::" namespace already, so "net::" prefix is unnecessary. > > Also, please use hacker_style_naming_for_variables Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode174 Line 174: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > see earlier comment. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode198 Line 198: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > see earlier comment. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode220 Line 220: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > see earlier comment. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode235 Line 235: { arraysize(kSOCKSOkReply), ERR_INVALID_RESPONSE }, On 2009/06/23 01:23:03, eroman wrote: > this value for reply_code doesn't make sense to me -- below |reply_code| is used > as the synchronous error code to be returned from the transport socket during a > read. So it should be a net error code (negative number). In this test case, although the mocksocket returns the correct value of read, it does not change the contents of the IOBuffer that was passed to it. Hence we expect an ERR_INVALID_RESPONSE for it, since the further asserts to check to contents of the Buffer will fail. http://codereview.chromium.org/139009/diff/3006/3007#newcode253 Line 253: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > see earlier comment. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode277 Line 277: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > See earlier comment. Done. http://codereview.chromium.org/139009/diff/3006/3007#newcode305 Line 305: while (rv == ERR_IO_PENDING) { On 2009/06/23 01:23:03, eroman wrote: > See earlier comment. Done.
http://codereview.chromium.org/139009/diff/5001/4006 File net/socket/socks_client_socket.h (right): http://codereview.chromium.org/139009/diff/5001/4006#newcode125 Line 125: FRIEND_TEST(SOCKSClientSocketTest, CompleteHandshake); Move these declarations to to top of the "private:" section. http://codereview.chromium.org/139009/diff/5001/4004 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5001/4004#newcode43 Line 43: DISALLOW_COPY_AND_ASSIGN(SOCKSClientSocketTest); I believe this should go in a "private:" section. http://codereview.chromium.org/139009/diff/5001/4004#newcode62 Line 62: mapper_->AddRule("www.google.com", "localhost"); Map this to an ip address instead. (127.0.0.1 for instance). Strictly speaking this isn't needed though, since net/base/run_all_unittests.cc mapps everything to 127.0.0.1. http://codereview.chromium.org/139009/diff/5001/4004#newcode89 Line 89: const char payload_read[] = "moar random data"; [optional]: I suggest making these std::string, and using payload_read.size() in place of arraysize(payload_read). http://codereview.chromium.org/139009/diff/5001/4004#newcode124 Line 124: EXPECT_EQ(arraysize(payload_read), rv); I suggest comparing the data in |buffer| to |payload_read| as well. http://codereview.chromium.org/139009/diff/5001/4004#newcode152 Line 152: for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) { nit: how about declaring "i" as size_t. http://codereview.chromium.org/139009/diff/5001/4004#newcode231 Line 231: MockWrite(true, kSOCKSOkRequest, arraysize(kSOCKSOkRequest)) }; indent by 4. http://codereview.chromium.org/139009/diff/5001/4004#newcode233 Line 233: MockRead(true, tests[i].reply_code) }; This test is still off. Let me try to explain the problem better: By not specifying any data for the MockRead, and yet giving a result code greater than 0, the Socket::Read() call to the mock transport socket will succeed, but not write anything into the IOBuffer. The IOBuffer is supplied by SocksClientSocket. It is not initialized. So in completion when we reach DoHandhsakeReadComplete(), since the sizes matches up in at least 1 of the 3 tests, we will end up reading |buffer_|. This is a problem, since we are reading un-initialized memory, and we could end up getting any result (although likeliest outcome of course is a failure of ERR_INVALID_RESPONSE). My recommendation to fix this test, is to set it up more like this, when testing a reply less than arraysize(kSOCKSOkReply). (And I don't think you can test the case where more data is returned, since SOCKSClientSocket will only read as much as is missing). MockRead data_reads[] = { MockRead(true, kSOCKSOkReply, tests[i].reply_len); MockRead(false, OK); // end of stream. };
fixed most comments. Also refactored BuildHandshakeWriteBuffer() as willchan suggested. http://codereview.chromium.org/139009/diff/5001/4006 File net/socket/socks_client_socket.h (right): http://codereview.chromium.org/139009/diff/5001/4006#newcode125 Line 125: FRIEND_TEST(SOCKSClientSocketTest, CompleteHandshake); On 2009/06/23 23:58:12, eroman wrote: > Move these declarations to to top of the "private:" section. Done. http://codereview.chromium.org/139009/diff/5001/4004 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5001/4004#newcode43 Line 43: DISALLOW_COPY_AND_ASSIGN(SOCKSClientSocketTest); On 2009/06/23 23:58:12, eroman wrote: > I believe this should go in a "private:" section. Done. http://codereview.chromium.org/139009/diff/5001/4004#newcode62 Line 62: mapper_->AddRule("www.google.com", "localhost"); On 2009/06/23 23:58:12, eroman wrote: > Map this to an ip address instead. (127.0.0.1 for instance). > > Strictly speaking this isn't needed though, since net/base/run_all_unittests.cc > mapps everything to 127.0.0.1. Since I am recreating another mapper for this test, I was thinking maybe it over-rides the initial mapper. Done. http://codereview.chromium.org/139009/diff/5001/4004#newcode89 Line 89: const char payload_read[] = "moar random data"; On 2009/06/23 23:58:12, eroman wrote: > [optional]: I suggest making these std::string, and using payload_read.size() in > place of arraysize(payload_read). Done. http://codereview.chromium.org/139009/diff/5001/4004#newcode124 Line 124: EXPECT_EQ(arraysize(payload_read), rv); On 2009/06/23 23:58:12, eroman wrote: > I suggest comparing the data in |buffer| to |payload_read| as well. Done. http://codereview.chromium.org/139009/diff/5001/4004#newcode152 Line 152: for (unsigned int i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) { On 2009/06/23 23:58:12, eroman wrote: > nit: how about declaring "i" as size_t. Done. http://codereview.chromium.org/139009/diff/5001/4004#newcode231 Line 231: MockWrite(true, kSOCKSOkRequest, arraysize(kSOCKSOkRequest)) }; On 2009/06/23 23:58:12, eroman wrote: > indent by 4. Done.
http://codereview.chromium.org/139009/diff/5008/5013 File net/socket/socks_client_socket.cc (right): http://codereview.chromium.org/139009/diff/5008/5013#newcode246 Line 246: struct sockaddr_in *ipv4_host = nit: "struct sockaddr_in* ipv4_host" (put the asterisk on the left). http://codereview.chromium.org/139009/diff/5008/5013#newcode266 Line 266: handshake_data.append(1, '\0'); alternatively: handshake_data.push_back('\0'); http://codereview.chromium.org/139009/diff/5008/5013#newcode277 Line 277: buffer_ = std::string(BuildHandshakeWriteBuffer()); no need to wrap inside "std::string(..)". How about: buffer_ = BuildHandshakeWriteBuffer(); http://codereview.chromium.org/139009/diff/5008/5014 File net/socket/socks_client_socket.h (right): http://codereview.chromium.org/139009/diff/5008/5014#newcode92 Line 92: const std::string BuildHandshakeWriteBuffer() const; I like this refactor! http://codereview.chromium.org/139009/diff/5008/5010 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5008/5010#newcode37 Line 37: AddressList addr_; nit: i suggest calling this "addrlist_" or "addresslist" (since that is what we call this in some other places). http://codereview.chromium.org/139009/diff/5008/5010#newcode74 Line 74: new StaticMockSocket(reads, writes)); This is a leak. (MockTCPClientSocket does not take ownership of the passed in StaticMockSocket). http://codereview.chromium.org/139009/diff/5008/5010#newcode84 Line 84: new HostResolver()); This is a leak, since the caller does not take ownership of this pointer. Instead, add a member to SOCKSClientSocketTest: HostResovler host_resolver_; And here pass in: &host_resolver_ Note that you will probably want to construct host_resolver_ as: : host_resolver_(0, 0) Otherwise, it will use a cache, which will cause some of the tests to fail (SOCKSClientSocketTest.HandshakeFailures will now fail synchronously rather than asynchronously, since the host resolution is already in the cache). http://codereview.chromium.org/139009/diff/5008/5010#newcode183 Line 183: user_sock_.reset(BuildMockSocket(data_reads, data_writes, "localhost", 80)); indentation is off here (remove 2 spaces). http://codereview.chromium.org/139009/diff/5008/5010#newcode235 Line 235: MockRead(true, tests[i].reply_code) }; Please see my previous comment about this test. http://codereview.chromium.org/139009/diff/5008/5010#newcode243 Line 243: EXPECT_TRUE((rv != OK) ^ user_sock_->IsConnected()); Can this just be 2 expects (since I believe all of these should fail). EXPECT_NE(OK, rv); EXPECT_FALSE(user_sock_->IsConnected()); http://codereview.chromium.org/139009/diff/5008/5010#newcode251 Line 251: mapper_->AddRule(hostname, ""); This is fine. But it will be more descriptive to write it as: mapper_->AddSimulatedFailure(hostname); http://codereview.chromium.org/139009/diff/5008/5010#newcode277 Line 277: mapper_->AddRule(hostname, "2001:db8:8714:3a90::12"); Note that this may behave similarly to the test above (will be a resolve failure), when running the unit-test on system's that don't support IPv6. I don't think this will change the outcome of the test (it will still select SOCKS4A), but figured I would point this out in case you encounter issues.
worked on the comments. Also including fixed FailedHandshakeRead test and corresponding changes for the sockets implementation. http://codereview.chromium.org/139009/diff/5008/5013 File net/socket/socks_client_socket.cc (right): http://codereview.chromium.org/139009/diff/5008/5013#newcode246 Line 246: struct sockaddr_in *ipv4_host = On 2009/06/24 18:14:26, eroman wrote: > nit: "struct sockaddr_in* ipv4_host" (put the asterisk on the left). Done. http://codereview.chromium.org/139009/diff/5008/5013#newcode266 Line 266: handshake_data.append(1, '\0'); On 2009/06/24 18:14:26, eroman wrote: > alternatively: > handshake_data.push_back('\0'); Done. http://codereview.chromium.org/139009/diff/5008/5013#newcode277 Line 277: buffer_ = std::string(BuildHandshakeWriteBuffer()); On 2009/06/24 18:14:26, eroman wrote: > no need to wrap inside "std::string(..)". > How about: > buffer_ = BuildHandshakeWriteBuffer(); Done. http://codereview.chromium.org/139009/diff/5008/5010 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5008/5010#newcode37 Line 37: AddressList addr_; On 2009/06/24 18:14:26, eroman wrote: > nit: i suggest calling this "addrlist_" or "addresslist" (since that is what we > call this in some other places). Done. http://codereview.chromium.org/139009/diff/5008/5010#newcode74 Line 74: new StaticMockSocket(reads, writes)); On 2009/06/24 18:14:26, eroman wrote: > This is a leak. > > (MockTCPClientSocket does not take ownership of the passed in StaticMockSocket). Done. http://codereview.chromium.org/139009/diff/5008/5010#newcode84 Line 84: new HostResolver()); On 2009/06/24 18:14:26, eroman wrote: > This is a leak, since the caller does not take ownership of this pointer. > > Instead, add a member to SOCKSClientSocketTest: > > HostResovler host_resolver_; > > And here pass in: > > &host_resolver_ > > Note that you will probably want to construct host_resolver_ as: > > : host_resolver_(0, 0) > > Otherwise, it will use a cache, which will cause some of the tests to fail > (SOCKSClientSocketTest.HandshakeFailures will now fail synchronously rather than > asynchronously, since the host resolution is already in the cache). Done. http://codereview.chromium.org/139009/diff/5008/5010#newcode183 Line 183: user_sock_.reset(BuildMockSocket(data_reads, data_writes, "localhost", 80)); On 2009/06/24 18:14:26, eroman wrote: > indentation is off here (remove 2 spaces). Done. http://codereview.chromium.org/139009/diff/5008/5010#newcode243 Line 243: EXPECT_TRUE((rv != OK) ^ user_sock_->IsConnected()); On 2009/06/24 18:14:26, eroman wrote: > Can this just be 2 expects (since I believe all of these should fail). > > EXPECT_NE(OK, rv); > EXPECT_FALSE(user_sock_->IsConnected()); Done. http://codereview.chromium.org/139009/diff/5008/5010#newcode251 Line 251: mapper_->AddRule(hostname, ""); On 2009/06/24 18:14:26, eroman wrote: > This is fine. But it will be more descriptive to write it as: > > mapper_->AddSimulatedFailure(hostname); Done.
LGTM http://codereview.chromium.org/139009/diff/5019/4028 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5019/4028#newcode42 Line 42: scoped_ptr<HostResolver> host_resolver_; Why not simply make this HostResolver host_resolver_; (you can still initialize it during the constructor with (0,0)) http://codereview.chromium.org/139009/diff/5019/4028#newcode220 Line 220: // Tests error propagation from transport socket during reads. This description should probably be updated as well.
fixed. http://codereview.chromium.org/139009/diff/5019/4028 File net/socket/socks_client_socket_unittest.cc (right): http://codereview.chromium.org/139009/diff/5019/4028#newcode42 Line 42: scoped_ptr<HostResolver> host_resolver_; On 2009/06/24 20:06:38, eroman wrote: > Why not simply make this > HostResolver host_resolver_; > > (you can still initialize it during the constructor with (0,0)) Done. http://codereview.chromium.org/139009/diff/5019/4028#newcode220 Line 220: // Tests error propagation from transport socket during reads. On 2009/06/24 20:06:38, eroman wrote: > This description should probably be updated as well. Done.
LGTM
gcc/mac compilations fix.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
