Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6577:
Porting of listen_socket, telnet_server to linux (Closed)
|
Created:
12 years, 2 months ago by ibrar Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionPorting of listen_socket,telnet_server to Linux.
BUG=3237
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Messages
Total messages: 14 (0 generated)
Hi, Kindly review the patch BUG = 3237 --ibrar
Hi, I have completed porting of telnet_server which is the only customer of the listen_sockt. Please review it. --Ibrar
Just a superficial review. I haven't really looked hard at this yet. http://codereview.chromium.org/6577/diff/202/206 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/202/206#newcode23 Line 23: { We're seeing this function all over the codebase, probably time to put it somewhere public. http://codereview.chromium.org/6577/diff/202/206#newcode101 Line 101: { when is state anything other than NOT_WAITING here? http://codereview.chromium.org/6577/diff/202/206#newcode130 Line 130: socket_delegate_->DidRead(this, buf); aha. That's why the +1! http://codereview.chromium.org/6577/diff/202/205 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/202/205#newcode42 Line 42: return fcntl(fd, F_SETFL, 0); I'm not sure fcntl works on windows like that; I suspect there you have to use FIONBIO like the code was doing before. http://codereview.chromium.org/6577/diff/202/205#newcode198 Line 198: usleep(1000); usleep uses signals. If you really need a way to sleep for a millisecond, better to use nanosleep or select with no fd's. http://codereview.chromium.org/6577/diff/202/205#newcode327 Line 327: sleep(10); again, do you really need to sleep for ten seconds here? I pointed this out to you some time ago in email, am surprised to see this still here. http://codereview.chromium.org/6577/diff/202/208 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/202/208#newcode247 Line 247: char buf[READ_BUF_SIZE+1]; Why the +1? http://codereview.chromium.org/6577/diff/202/207 File net/base/telnet_server_unittest.cc (right): http://codereview.chromium.org/6577/diff/202/207#newcode31 Line 31: sleep(10); Do you really need to sleep for ten seconds here?
http://codereview.chromium.org/6577/diff/202/206 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/202/206#newcode23 Line 23: { On 2008/10/12 20:26:06, dank wrote: > We're seeing this function all over the codebase, probably time to put it > somewhere public. Agreed http://codereview.chromium.org/6577/diff/202/206#newcode101 Line 101: { On 2008/10/12 20:26:06, dank wrote: > when is state anything other than NOT_WAITING here? Good catch! I Will remove state parameter. http://codereview.chromium.org/6577/diff/202/205 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/202/205#newcode42 Line 42: return fcntl(fd, F_SETFL, 0); On 2008/10/12 20:26:06, dank wrote: > I'm not sure fcntl works on windows like that; > I suspect there you have to use FIONBIO like the > code was doing before. OK! will look at the Windows docs. http://codereview.chromium.org/6577/diff/202/208 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/202/208#newcode247 Line 247: char buf[READ_BUF_SIZE+1]; On 2008/10/12 20:26:06, dank wrote: > Why the +1? Will remove +1 http://codereview.chromium.org/6577/diff/202/207 File net/base/telnet_server_unittest.cc (right): http://codereview.chromium.org/6577/diff/202/207#newcode31 Line 31: sleep(10); On 2008/10/12 20:26:06, dank wrote: > Do you really need to sleep for ten seconds here? No, actually there is no need of sleep in Windows too. I will send a separate patch to update Windows/Linux code to run without sleep.
http://codereview.chromium.org/6577/diff/202/206 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/202/206#newcode23 Line 23: { On 2008/10/12 20:26:06, dank wrote: > We're seeing this function all over the codebase, probably time to put it > somewhere public. Where do you suggest? http://codereview.chromium.org/6577/diff/202/206#newcode101 Line 101: { On 2008/10/12 20:56:40, ibrar.ahmad wrote: > On 2008/10/12 20:26:06, dank wrote: > > when is state anything other than NOT_WAITING here? > > Good catch! I Will remove state parameter. Done. http://codereview.chromium.org/6577/diff/202/205 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/202/205#newcode42 Line 42: return fcntl(fd, F_SETFL, 0); On 2008/10/12 20:26:06, dank wrote: > I'm not sure fcntl works on windows like that; > I suspect there you have to use FIONBIO like the > code was doing before. Done, but not tested on windows. http://codereview.chromium.org/6577/diff/202/205#newcode42 Line 42: return fcntl(fd, F_SETFL, 0); On 2008/10/12 20:26:06, dank wrote: > I'm not sure fcntl works on windows like that; > I suspect there you have to use FIONBIO like the > code was doing before. Done. http://codereview.chromium.org/6577/diff/202/205#newcode198 Line 198: usleep(1000); On 2008/10/12 20:26:06, dank wrote: > usleep uses signals. If you really need a way to sleep for a millisecond, better > to use nanosleep or select with no fd's. Done. http://codereview.chromium.org/6577/diff/202/205#newcode327 Line 327: sleep(10); On 2008/10/12 20:26:06, dank wrote: > again, do you really need to sleep for ten seconds here? > I pointed this out to you some time ago in email, am surprised to see this still > here. Done. http://codereview.chromium.org/6577/diff/202/207 File net/base/telnet_server_unittest.cc (right): http://codereview.chromium.org/6577/diff/202/207#newcode31 Line 31: sleep(10); On 2008/10/12 20:56:40, ibrar.ahmad wrote: > On 2008/10/12 20:26:06, dank wrote: > > Do you really need to sleep for ten seconds here? > > No, actually there is no need of sleep in Windows too. I will send a separate > patch to update Windows/Linux code to run without sleep. > I have change the logic of code need to test on windows http://codereview.chromium.org/6577/diff/202/207#newcode31 Line 31: sleep(10); On 2008/10/12 20:56:40, ibrar.ahmad wrote: > On 2008/10/12 20:26:06, dank wrote: > > Do you really need to sleep for ten seconds here? > > No, actually there is no need of sleep in Windows too. I will send a separate > patch to update Windows/Linux code to run without sleep. > Done.
I have done all the points you have mentioned. All test cases passed on windows and Linux. http://codereview.chromium.org/6577/diff/202/206 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/202/206#newcode23 Line 23: { On 2008/10/13 08:52:00, ibrar.ahmad wrote: > On 2008/10/12 20:26:06, dank wrote: > > We're seeing this function all over the codebase, probably time to put it > > somewhere public. > > Where do you suggest? Done. http://codereview.chromium.org/6577/diff/202/205 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/202/205#newcode42 Line 42: return fcntl(fd, F_SETFL, 0); On 2008/10/13 08:52:00, ibrar.ahmad wrote: > On 2008/10/12 20:26:06, dank wrote: > > I'm not sure fcntl works on windows like that; > > I suspect there you have to use FIONBIO like the > > code was doing before. > > Done, but not tested on windows. Windows testing done
Sorry for the delay on this review. I've been out on vacation. http://codereview.chromium.org/6577/diff/215/402 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/215/402#newcode22 Line 22: #include <errno.h> Do all of these #includes you've added need to be in this header file? Our coding style is to put #includes only where the file itself actually references something in the header file). http://codereview.chromium.org/6577/diff/215/402#newcode41 Line 41: public base::ObjectWatcher::Delegate whitespace is wrong here (the public should line up with the previous line) http://codereview.chromium.org/6577/diff/215/402#newcode41 Line 41: public base::ObjectWatcher::Delegate Having two different interfaces here seems wrong to me. Can we abstract this delegate interface so that ObjectWatcher and MessagePumpLibevent share the same delegate methods? http://codereview.chromium.org/6577/diff/215/402#newcode43 Line 43: public base::MessagePumpLibevent::Watcher same here http://codereview.chromium.org/6577/diff/215/402#newcode95 Line 95: int wait_state_; shouldn't this be type WaitState? http://codereview.chromium.org/6577/diff/215/402#newcode101 Line 101: void WatchSocket(int state); type WaitState? http://codereview.chromium.org/6577/diff/215/402#newcode102 Line 102: void UnWatchSocket(); "Unwatch" isn't an English word, so I'm not a big fan of it in a method name. Maybe "StopWatching" instead? If you're going to stick with it, capitalization should be "Unwatch" instead of "UnWatch". http://codereview.chromium.org/6577/diff/215/402#newcode108 Line 108: private: you added a space here - it should be one space http://codereview.chromium.org/6577/diff/215/404 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/215/404#newcode5 Line 5: General comment on this file - it looks like this is *very* close to the original windows implementation with relatively minor differences. Is there a reason that these changes couldn't have been #ifdef'd? If there are individual methods which are radically different, then those could have been split into separate _posix.cc and _win.cc files. You made a bunch of improvements to the original (especially with regard to error handling) which would work as in the windows version as well and it's a shame to have them diverge. One further comment for the future - if you're going to derive one file from another, please use svn cp so that the deltas can be more easily seen and that the change log is preserved. http://codereview.chromium.org/6577/diff/215/404#newcode67 Line 67: if (errno == -1 && errno != EINPROGRESS && errno != EWOULDBLOCK) { remove unneeded {} for this block http://codereview.chromium.org/6577/diff/215/404#newcode78 Line 78: net::SetNonBlocking(conn); whitespace http://codereview.chromium.org/6577/diff/215/404#newcode94 Line 94: { put this on the previous line - check the file for all instances of this http://codereview.chromium.org/6577/diff/215/404#newcode102 Line 102: socket_, EV_READ|EV_PERSIST, event_.get(),this); indent 4 spaces not two http://codereview.chromium.org/6577/diff/215/404#newcode110 Line 110: len = recv(socket_, buf, READ_BUF_SIZE, 0); lots of whitespace issues in this method http://codereview.chromium.org/6577/diff/215/404#newcode139 Line 139: else { this should be combined with the previous line http://codereview.chromium.org/6577/diff/215/404#newcode160 Line 160: Accept(); whitespace http://codereview.chromium.org/6577/diff/215/403 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/215/403#newcode19 Line 19: #include <arpa/inet.h> again, are all of these includes necessary? http://codereview.chromium.org/6577/diff/215/403#newcode35 Line 35: #define READ_BUF_SIZE 1024 change to static const int kReadBufSize http://codereview.chromium.org/6577/diff/215/403#newcode39 Line 39: { move to previous line http://codereview.chromium.org/6577/diff/215/403#newcode43 Line 43: struct timespec req={0}; there are a ton of whitespace issues here. spaces between all operators: req = {0}; sec = (int)(millisec / 1000); etc. http://codereview.chromium.org/6577/diff/215/403#newcode185 Line 185: pthread_mutex_lock(&lock_); rather than doing the #ifdef with this pattern, it would be nice if the lock/unlock logic were factored out http://codereview.chromium.org/6577/diff/215/403#newcode214 Line 214: pthread_mutex_unlock(&lock_); ...that way, when you fixed bugs in one implementation (like this line does), it would also get fixed in the other http://codereview.chromium.org/6577/diff/215/403#newcode238 Line 238: if(time_out > 10) break; whitespace between if and '(' newline before break http://codereview.chromium.org/6577/diff/215/403#newcode243 Line 243: if (errno == EWOULDBLOCK || errno == EAGAIN) { move the #ifdef to be around this line rather than the whole if block http://codereview.chromium.org/6577/diff/215/403#newcode294 Line 294: return false; whitespace here and the next line http://codereview.chromium.org/6577/diff/215/407 File net/base/net_util.cc (right): http://codereview.chromium.org/6577/diff/215/407#newcode118 Line 118: extra newline http://codereview.chromium.org/6577/diff/215/407#newcode923 Line 923: { move to previous line http://codereview.chromium.org/6577/diff/215/407#newcode925 Line 925: unsigned long no_block = 1; indent on all of these lines should be 2, not 4 http://codereview.chromium.org/6577/diff/215/408 File net/base/net_util.h (right): http://codereview.chromium.org/6577/diff/215/408#newcode139 Line 139: //Set socket to non-blocking mode add space after // http://codereview.chromium.org/6577/diff/215/406 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/215/406#newcode12 Line 12: const int INVALID_SOCKET = -1; constant names should be of the form kInvalidSocket rather than INVALID_SOCKET. They should be declared after the #include lines. These should also be static. http://codereview.chromium.org/6577/diff/215/406#newcode15 Line 15: #define SOCKET int these two definitions should also be after the #include lines. http://codereview.chromium.org/6577/diff/215/406#newcode252 Line 252: len = recv(socket_, buf, READ_BUF_SIZE, 0); indent for these lines should be 2 not 4 http://codereview.chromium.org/6577/diff/215/406#newcode261 Line 261: if (errno == EWOULDBLOCK || errno == EAGAIN) { remove unneeded {} for this block
Thanks Erik, I have done everything except 2 points. These two points need further clarification http://codereview.chromium.org/6577/diff/215/402#newcode41 http://codereview.chromium.org/6577/diff/215/403#newcode185 --ibrar http://codereview.chromium.org/6577/diff/215/402 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/215/402#newcode22 Line 22: #include <errno.h> On 2008/10/22 19:36:06, erikkay wrote: > Do all of these #includes you've added need to be in this header file? Our > coding style is to put #includes only where the file itself actually references > something in the header file). Done. http://codereview.chromium.org/6577/diff/215/402#newcode41 Line 41: public base::ObjectWatcher::Delegate On 2008/10/22 19:36:06, erikkay wrote: > whitespace is wrong here (the public should line up with the previous line) Done. http://codereview.chromium.org/6577/diff/215/402#newcode41 Line 41: public base::ObjectWatcher::Delegate On 2008/10/22 19:36:06, erikkay wrote: > Having two different interfaces here seems wrong to me. Can we abstract this > delegate interface so that ObjectWatcher and MessagePumpLibevent share the same > delegate methods? Actually there are more occurrence of these type of implementation in code base, please see tcp_client_socket.h. I think Dan can comment on this. http://codereview.chromium.org/6577/diff/215/402#newcode43 Line 43: public base::MessagePumpLibevent::Watcher On 2008/10/22 19:36:06, erikkay wrote: > same here Done. http://codereview.chromium.org/6577/diff/215/402#newcode95 Line 95: int wait_state_; On 2008/10/22 19:36:06, erikkay wrote: > shouldn't this be type WaitState? Done. http://codereview.chromium.org/6577/diff/215/402#newcode101 Line 101: void WatchSocket(int state); On 2008/10/22 19:36:06, erikkay wrote: > type WaitState? Done. http://codereview.chromium.org/6577/diff/215/402#newcode102 Line 102: void UnWatchSocket(); On 2008/10/22 19:36:06, erikkay wrote: > "Unwatch" isn't an English word, so I'm not a big fan of it in a method name. > Maybe "StopWatching" instead? If you're going to stick with it, capitalization > should be "Unwatch" instead of "UnWatch". I have renamed function with Unwatch http://codereview.chromium.org/6577/diff/215/402#newcode108 Line 108: private: On 2008/10/22 19:36:06, erikkay wrote: > you added a space here - it should be one space Done. http://codereview.chromium.org/6577/diff/215/404 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/215/404#newcode5 Line 5: On 2008/10/22 19:36:06, erikkay wrote: > General comment on this file - it looks like this is *very* close to the > original windows implementation with relatively minor differences. Is there a > reason that these changes couldn't have been #ifdef'd? If there are individual > methods which are radically different, then those could have been split into > separate _posix.cc and _win.cc files. You made a bunch of improvements to the > original (especially with regard to error handling) which would work as in the > windows version as well and it's a shame to have them diverge. One further > comment for the future - if you're going to derive one file from another, please > use svn cp so that the deltas can be more easily seen and that the change log is > preserved. > Agreed I have tried to make a single file but it looks so ugly with too much #ifdef, BTW I will fix the issue present in Windows code. If you really need to merge these file in a single file let me know I will do that. http://codereview.chromium.org/6577/diff/215/404#newcode67 Line 67: if (errno == -1 && errno != EINPROGRESS && errno != EWOULDBLOCK) { On 2008/10/22 19:36:06, erikkay wrote: > remove unneeded {} for this block Done. http://codereview.chromium.org/6577/diff/215/404#newcode78 Line 78: net::SetNonBlocking(conn); On 2008/10/22 19:36:06, erikkay wrote: > whitespace Done. http://codereview.chromium.org/6577/diff/215/404#newcode102 Line 102: socket_, EV_READ|EV_PERSIST, event_.get(),this); On 2008/10/22 19:36:06, erikkay wrote: > indent 4 spaces not two Done. http://codereview.chromium.org/6577/diff/215/404#newcode110 Line 110: len = recv(socket_, buf, READ_BUF_SIZE, 0); On 2008/10/22 19:36:06, erikkay wrote: > lots of whitespace issues in this method Done. http://codereview.chromium.org/6577/diff/215/404#newcode139 Line 139: else { On 2008/10/22 19:36:06, erikkay wrote: > this should be combined with the previous line Done. http://codereview.chromium.org/6577/diff/215/404#newcode160 Line 160: Accept(); On 2008/10/22 19:36:06, erikkay wrote: > whitespace Done. http://codereview.chromium.org/6577/diff/215/403 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/215/403#newcode19 Line 19: #include <arpa/inet.h> On 2008/10/22 19:36:06, erikkay wrote: > again, are all of these includes necessary? I have removed unnecessary files. http://codereview.chromium.org/6577/diff/215/403#newcode35 Line 35: #define READ_BUF_SIZE 1024 On 2008/10/22 19:36:06, erikkay wrote: > change to static const int kReadBufSize Done. http://codereview.chromium.org/6577/diff/215/403#newcode39 Line 39: { On 2008/10/22 19:36:06, erikkay wrote: > move to previous line Done. http://codereview.chromium.org/6577/diff/215/403#newcode43 Line 43: struct timespec req={0}; On 2008/10/22 19:36:06, erikkay wrote: > there are a ton of whitespace issues here. spaces between all operators: > req = {0}; > sec = (int)(millisec / 1000); > etc. Done. http://codereview.chromium.org/6577/diff/215/403#newcode185 Line 185: pthread_mutex_lock(&lock_); On 2008/10/22 19:36:06, erikkay wrote: > rather than doing the #ifdef with this pattern, it would be nice if the > lock/unlock logic were factored out Do you want me to place this lock/unlock logic in separate functions ? http://codereview.chromium.org/6577/diff/215/403#newcode214 Line 214: pthread_mutex_unlock(&lock_); On 2008/10/22 19:36:06, erikkay wrote: > ...that way, when you fixed bugs in one implementation (like this line does), it > would also get fixed in the other Done. http://codereview.chromium.org/6577/diff/215/403#newcode243 Line 243: if (errno == EWOULDBLOCK || errno == EAGAIN) { On 2008/10/22 19:36:06, erikkay wrote: > move the #ifdef to be around this line rather than the whole if block Done. http://codereview.chromium.org/6577/diff/215/403#newcode294 Line 294: return false; On 2008/10/22 19:36:06, erikkay wrote: > whitespace here and the next line Done. http://codereview.chromium.org/6577/diff/215/407 File net/base/net_util.cc (right): http://codereview.chromium.org/6577/diff/215/407#newcode118 Line 118: On 2008/10/22 19:36:06, erikkay wrote: > extra newline Done. http://codereview.chromium.org/6577/diff/215/407#newcode923 Line 923: { On 2008/10/22 19:36:06, erikkay wrote: > move to previous line Done. http://codereview.chromium.org/6577/diff/215/407#newcode925 Line 925: unsigned long no_block = 1; On 2008/10/22 19:36:06, erikkay wrote: > indent on all of these lines should be 2, not 4 Done. http://codereview.chromium.org/6577/diff/215/408 File net/base/net_util.h (right): http://codereview.chromium.org/6577/diff/215/408#newcode139 Line 139: //Set socket to non-blocking mode On 2008/10/22 19:36:06, erikkay wrote: > add space after // Done. http://codereview.chromium.org/6577/diff/215/406 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/215/406#newcode12 Line 12: const int INVALID_SOCKET = -1; On 2008/10/22 19:36:06, erikkay wrote: > constant names should be of the form kInvalidSocket rather than INVALID_SOCKET. > They should be declared after the #include lines. These should also be static. Done. http://codereview.chromium.org/6577/diff/215/406#newcode15 Line 15: #define SOCKET int On 2008/10/22 19:36:06, erikkay wrote: > these two definitions should also be after the #include lines. Done. http://codereview.chromium.org/6577/diff/215/406#newcode252 Line 252: len = recv(socket_, buf, READ_BUF_SIZE, 0); On 2008/10/22 19:36:06, erikkay wrote: > indent for these lines should be 2 not 4 Done. http://codereview.chromium.org/6577/diff/215/406#newcode261 Line 261: if (errno == EWOULDBLOCK || errno == EAGAIN) { On 2008/10/22 19:36:06, erikkay wrote: > remove unneeded {} for this block Done.
http://codereview.chromium.org/6577/diff/215/402 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/215/402#newcode41 Line 41: public base::ObjectWatcher::Delegate On 2008/10/23 12:34:48, ibrar wrote: > On 2008/10/22 19:36:06, erikkay wrote: > > Having two different interfaces here seems wrong to me. Can we abstract this > > delegate interface so that ObjectWatcher and MessagePumpLibevent share the > same > > delegate methods? > > Actually there are more occurrence of these type of implementation in code base, > please see tcp_client_socket.h. I think Dan can comment on this. Any thoughts Dan? http://codereview.chromium.org/6577/diff/215/404 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/215/404#newcode5 Line 5: On 2008/10/23 12:34:48, ibrar wrote: > Agreed I have tried to make a single file but it looks so ugly with too much > #ifdef, BTW I will fix the issue present in Windows code. If you really need to > merge these file in a single file let me know I will do that. Looking at the differences between this and the Windows code, I think it can be done without a too many #ifdefs. In general the model we've gone for is that if a particular method would have too many #ifdefs, then split the file into three files: listen_socket.cc, listen_socket_win.cc and listen_socket_posix.cc and put platform-specific methods into the platform files. So looking at this file, I'd put the platform-specific methods like ObjectSignaled and OnSocketReady into their own specific files. Maybe have a Windows impl of Watch and Unwatch to match the posix one, and #ifdef the rest. http://codereview.chromium.org/6577/diff/803/603 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/803/603#newcode24 Line 24: #define SOCKET int These definitions still need to move to after all of the #include lines. Another #ifdef sucks, but oh well. http://codereview.chromium.org/6577/diff/803/605 File net/base/listen_socket_libevent.cc (right): http://codereview.chromium.org/6577/diff/803/605#newcode111 Line 111: wait_state_ = WAITING_CLOSE; indent is off here http://codereview.chromium.org/6577/diff/803/604 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/803/604#newcode17 Line 17: #define SOCKET int these definitions need to move down to after the includes http://codereview.chromium.org/6577/diff/803/604#newcode18 Line 18: const int kSocketError = -1; It looks like changing this to kSocketError led to more #ifdef's below. Switch it back to a #define SOCKET_ERROR and add a comment explaining it. http://codereview.chromium.org/6577/diff/803/604#newcode34 Line 34: //millisecond sleep space after // http://codereview.chromium.org/6577/diff/803/604#newcode234 Line 234: int err = WSAGetLastError(); indent issues here http://codereview.chromium.org/6577/diff/803/604#newcode242 Line 242: if (time_out > 10) break; newline before break http://codereview.chromium.org/6577/diff/803/607 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/803/607#newcode11 Line 11: #else don't use #else here. We should be using #elif defined(OS_POSIX). However, in this case I'd use #endif and #if defined(OS_POSIX) so that you can put the telnet_server.h include before these (see comment below) http://codereview.chromium.org/6577/diff/803/607#newcode12 Line 12: #define SOCKET int again, definitions need to come after all includes. If not, they need a comment to explain why. Also see the ones below. http://codereview.chromium.org/6577/diff/803/607#newcode21 Line 21: const int kSocketError = -1; it looks like chaning these to typical constants caused extra #ifdefs below. Sorry about that. Go ahead and switch back to #defines and put a comment explaining why. http://codereview.chromium.org/6577/diff/803/607#newcode24 Line 24: #include "net/base/telnet_server.h" technically, this should be the first include. on windows it can't be (see the comment before winsock2.h). http://codereview.chromium.org/6577/diff/803/607#newcode152 Line 152: if (s == kInvalidSocket) { indent
Thanks Erik, I have removed listen_socket_libevent.cc and added code into listen_socket.cc using #ifdef. --ibrar http://codereview.chromium.org/6577/diff/803/603 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/803/603#newcode24 Line 24: #define SOCKET int On 2008/10/24 00:34:18, Erik Kay wrote: > These definitions still need to move to after all of the #include lines. > Another #ifdef sucks, but oh well. Done. http://codereview.chromium.org/6577/diff/803/604 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/803/604#newcode17 Line 17: #define SOCKET int On 2008/10/24 00:34:18, Erik Kay wrote: > these definitions need to move down to after the includes Done. http://codereview.chromium.org/6577/diff/803/604#newcode18 Line 18: const int kSocketError = -1; On 2008/10/24 00:34:18, Erik Kay wrote: > It looks like changing this to kSocketError led to more #ifdef's below. Switch > it back to a #define SOCKET_ERROR and add a comment explaining it. Done. http://codereview.chromium.org/6577/diff/803/604#newcode34 Line 34: //millisecond sleep On 2008/10/24 00:34:18, Erik Kay wrote: > space after // Done. http://codereview.chromium.org/6577/diff/803/604#newcode234 Line 234: int err = WSAGetLastError(); On 2008/10/24 00:34:18, Erik Kay wrote: > indent issues here Done. http://codereview.chromium.org/6577/diff/803/604#newcode242 Line 242: if (time_out > 10) break; On 2008/10/24 00:34:18, Erik Kay wrote: > newline before break Done. http://codereview.chromium.org/6577/diff/803/607 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/803/607#newcode11 Line 11: #else On 2008/10/24 00:34:18, Erik Kay wrote: > don't use #else here. We should be using #elif defined(OS_POSIX). However, in > this case I'd use #endif and #if defined(OS_POSIX) so that you can put the > telnet_server.h include before these (see comment below) Done. http://codereview.chromium.org/6577/diff/803/607#newcode12 Line 12: #define SOCKET int On 2008/10/24 00:34:18, Erik Kay wrote: > again, definitions need to come after all includes. If not, they need a comment > to explain why. Also see the ones below. Done. http://codereview.chromium.org/6577/diff/803/607#newcode21 Line 21: const int kSocketError = -1; On 2008/10/24 00:34:18, Erik Kay wrote: > it looks like chaning these to typical constants caused extra #ifdefs below. > Sorry about that. Go ahead and switch back to #defines and put a comment > explaining why. Done. http://codereview.chromium.org/6577/diff/803/607#newcode24 Line 24: #include "net/base/telnet_server.h" On 2008/10/24 00:34:18, Erik Kay wrote: > technically, this should be the first include. on windows it can't be (see the > comment before winsock2.h). Done. http://codereview.chromium.org/6577/diff/803/607#newcode152 Line 152: if (s == kInvalidSocket) { On 2008/10/24 00:34:18, Erik Kay wrote: > indent Done.
We're very close here. Thanks for putting the time into this. http://codereview.chromium.org/6577/diff/649/659 File net/base/listen_socket.cc (right): http://codereview.chromium.org/6577/diff/649/659#newcode42 Line 42: WatchSocket(); Is there a reason why Watch/Unwatch couldn't be called in the same places? I don't think we do anything until Listen anyway, so I'd say we could just move the code into Listen/Close and get rid of the #ifdefs. http://codereview.chromium.org/6577/diff/649/659#newcode52 Line 52: if (socket_) { Can this part be moved out of the ifdef and into a "Close" method? You could use this in the Listen error handling below as well. http://codereview.chromium.org/6577/diff/649/659#newcode120 Line 120: new ListenSocket(conn, socket_delegate_); indent should be 4 here http://codereview.chromium.org/6577/diff/649/659#newcode146 Line 146: break; we still need a TODO - error comment here. Right now both sides of the if do the same thing. If there's no TODO, then we should just get rid of the if. http://codereview.chromium.org/6577/diff/649/659#newcode148 Line 148: } else if (len == 0) { // socket closed ignore in case of window only move comment to next line http://codereview.chromium.org/6577/diff/649/659#newcode150 Line 150: wait_state_ = WAITING_CLOSE; indent is off http://codereview.chromium.org/6577/diff/649/659#newcode197 Line 197: // TODO (ibrar): there should be logic here to handle this beacsue it is not an error 80 columns. "beacsue" typo. http://codereview.chromium.org/6577/diff/649/650 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/649/650#newcode17 Line 17: #else #elif defined(OS_POSIX) http://codereview.chromium.org/6577/diff/649/650#newcode84 Line 84: // used default parameter for Windows because we are not using state for Windows 80 columns for comments http://codereview.chromium.org/6577/diff/649/650#newcode85 Line 85: void WatchSocket(WaitState state = NOT_WAITING); We don't use default paramters. Maybe just #ifdef the method signature. http://codereview.chromium.org/6577/diff/649/651 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/649/651#newcode232 Line 232: #if defined(OS_WIN) move the #ifdef down one line http://codereview.chromium.org/6577/diff/649/653 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/649/653#newcode152 Line 152: TelnetServer *serv = new TelnetServer(s, del); indent http://codereview.chromium.org/6577/diff/649/653#newcode254 Line 254: #if defined(OS_WIN) move ifdef down one line here
Thanks Erik, I have almost all points you have mentioned. --ibrar http://codereview.chromium.org/6577/diff/649/659 File net/base/listen_socket.cc (right): http://codereview.chromium.org/6577/diff/649/659#newcode42 Line 42: WatchSocket(); On 2008/10/27 17:08:48, Erik Kay wrote: > Is there a reason why Watch/Unwatch couldn't be called in the same places? I > don't think we do anything until Listen anyway, so I'd say we could just move > the code into Listen/Close and get rid of the #ifdefs. Actually concept of libevent and WSA Events are totally different, at lease I don't want to touch windows code in this go. I will fix these issue with all other TODO's. http://codereview.chromium.org/6577/diff/649/659#newcode52 Line 52: if (socket_) { On 2008/10/27 17:08:48, Erik Kay wrote: > Can this part be moved out of the ifdef and into a "Close" method? You could > use this in the Listen error handling below as well. I have done first part of this because in second part listen is an static function. For using Close within this I have to make Close static and go on. http://codereview.chromium.org/6577/diff/649/659#newcode120 Line 120: new ListenSocket(conn, socket_delegate_); On 2008/10/27 17:08:48, Erik Kay wrote: > indent should be 4 here Done. http://codereview.chromium.org/6577/diff/649/659#newcode146 Line 146: break; On 2008/10/27 17:08:48, Erik Kay wrote: > we still need a TODO - error comment here. Right now both sides of the if do > the same thing. If there's no TODO, then we should just get rid of the if. Done. http://codereview.chromium.org/6577/diff/649/659#newcode148 Line 148: } else if (len == 0) { // socket closed ignore in case of window only On 2008/10/27 17:08:48, Erik Kay wrote: > move comment to next line Done. http://codereview.chromium.org/6577/diff/649/659#newcode150 Line 150: wait_state_ = WAITING_CLOSE; On 2008/10/27 17:08:48, Erik Kay wrote: > indent is off Done. http://codereview.chromium.org/6577/diff/649/659#newcode197 Line 197: // TODO (ibrar): there should be logic here to handle this beacsue it is not an error On 2008/10/27 17:08:48, Erik Kay wrote: > 80 columns. "beacsue" typo. Done. http://codereview.chromium.org/6577/diff/649/659#newcode197 Line 197: // TODO (ibrar): there should be logic here to handle this beacsue it is not an error On 2008/10/27 17:08:48, Erik Kay wrote: > 80 columns. "beacsue" typo. Done. http://codereview.chromium.org/6577/diff/649/650 File net/base/listen_socket.h (right): http://codereview.chromium.org/6577/diff/649/650#newcode17 Line 17: #else On 2008/10/27 17:08:48, Erik Kay wrote: > #elif defined(OS_POSIX) Done. http://codereview.chromium.org/6577/diff/649/650#newcode84 Line 84: // used default parameter for Windows because we are not using state for Windows On 2008/10/27 17:08:48, Erik Kay wrote: > 80 columns for comments Done. http://codereview.chromium.org/6577/diff/649/650#newcode85 Line 85: void WatchSocket(WaitState state = NOT_WAITING); On 2008/10/27 17:08:48, Erik Kay wrote: > We don't use default paramters. Maybe just #ifdef the method signature. > > > Done. http://codereview.chromium.org/6577/diff/649/651 File net/base/listen_socket_unittest.h (right): http://codereview.chromium.org/6577/diff/649/651#newcode232 Line 232: #if defined(OS_WIN) On 2008/10/27 17:08:48, Erik Kay wrote: > move the #ifdef down one line Done. http://codereview.chromium.org/6577/diff/649/653 File net/base/telnet_server.cc (right): http://codereview.chromium.org/6577/diff/649/653#newcode152 Line 152: TelnetServer *serv = new TelnetServer(s, del); On 2008/10/27 17:08:48, Erik Kay wrote: > indent Done. http://codereview.chromium.org/6577/diff/649/653#newcode254 Line 254: #if defined(OS_WIN) On 2008/10/27 17:08:48, Erik Kay wrote: > move ifdef down one line here Done.
LGTM I can submit your change for you, but it looks like you haven't filled out the CLA (contributor license agreement): http://code.google.com/legal/individual-cla-v1.0.html At the end of the page there's an electronic version of the form that most people find more convenient. Let me know when you've done it and I'll start getting your change submitted.
LGTM. http://codereview.chromium.org/6577/diff/834/669 File net/base/listen_socket_unittest.cc (right): http://codereview.chromium.org/6577/diff/834/669#newcode9 Line 9: You don't need this include, do you? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
