Issue 6658027: UDP sockets implementation for windows.

Issue 6658027: UDP sockets implementation for windows. (Closed)

Created:
9 years, 9 months ago by Sergey Ulanov

Modified:
9 years, 7 months ago

Reviewers:
Mike Belshe

CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

UDP sockets implementation for windows. BUG=None TEST=Unittests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77895

Patch Set 3 : addressed comments #

Patch Set 9 : typo #

Created: 9 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+787 lines, -160 lines)			Patch
A	net/base/winsock_util.h	View	1 2	1 chunk	+31 lines, -0 lines	0 comments	Download
A	net/base/winsock_util.cc	View	1 2 3 4 5 6 7 8	1 chunk	+96 lines, -0 lines	0 comments	Download
M	net/net.gyp	View	1 2 3 4 5 6	6 chunks	+17 lines, -23 lines	0 comments	Download
M	net/socket/tcp_client_socket_win.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -86 lines	0 comments	Download
M	net/udp/datagram_server_socket.h	View		1 chunk	+0 lines, -2 lines	0 comments	Download
M	net/udp/udp_socket.h	View		1 chunk	+18 lines, -1 line	0 comments	Download
M	net/udp/udp_socket_libevent.h	View		4 chunks	+1 line, -23 lines	0 comments	Download
M	net/udp/udp_socket_libevent.cc	View	1	13 chunks	+28 lines, -14 lines	0 comments	Download
M	net/udp/udp_socket_unittest.cc	View	1 2 3 4 5 6 7	5 chunks	+51 lines, -11 lines	0 comments	Download
A	net/udp/udp_socket_win.h	View	1 2 3 4 5	1 chunk	+178 lines, -0 lines	0 comments	Download
A	net/udp/udp_socket_win.cc	View	1 2 3 4	1 chunk	+366 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Mike Belshe

Thanks Sergey! This looks great! A few nits and suggestions. http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#newcode26 ...

9 years, 9 months ago (2011-03-10 06:53:38 UTC) #2

Sergey Ulanov

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#newcode26 net/udp/udp_socket_win.cc:26: void AssertEventNotSignaled(WSAEVENT hEvent) { On 2011/03/10 06:53:38, Mike Belshe ...

9 years, 9 months ago (2011-03-11 00:54:37 UTC) #3

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:26: void AssertEventNotSignaled(WSAEVENT hEvent) {
On 2011/03/10 06:53:38, Mike Belshe wrote:
> Since this code (AssertEventNotSignaled and ResaetEventIfSignaled) are
> duplicated in the tcp_client_socket_win and here, should we move to net_util?

Moved it all to net/base/winsock_util.cc .

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:123:
ALLOW_THIS_IN_INITIALIZER_LIST(read_delegate_(this)),
On 2011/03/10 06:53:38, Mike Belshe wrote:
> nit: for argument ordering, read_delegate should follow socket_, line 120.

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:136: write_overlapped_.hEvent = WSACreateEvent();
On 2011/03/10 06:53:38, Mike Belshe wrote:
> should we also:
>   memset(&read_buffer_, 0, sizeof(read_buffer_));
>   memset(&write_buffer_, 0, sizeof(write_buffer_));
I don't think it is neccessary: WSABUF has only two members, and we set both
explicitly before reading/writing.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:225: read_buffer_.len = buf_len;
On 2011/03/10 06:53:38, Mike Belshe wrote:
> Note:  I liked what you did in the libevent.cc changes - you make
InternalRead()
> take the buf/len as arguments, and then you moved lines 224/225 down to after
> the InternalRead if it returned ERR_IO_PENDING.

Added the same arguments in InternalRead() here

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:237: int buf_len,
On 2011/03/10 06:53:38, Mike Belshe wrote:
> nit spacing

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:311: if (SetNonBlocking(socket_)) {
On 2011/03/10 06:53:38, Mike Belshe wrote:
> I don't think we want this SetNonBlocking call - is this the
> net_util::SetNonBlocking?
> 
> In this case, we're using the overlapped IO APIs, and those should suffice,
> right?

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:351: read_iobuffer_ = NULL;
On 2011/03/10 06:53:38, Mike Belshe wrote:
> I think you should zero out the read_buffer_ in here.  Otherwise, we're still
> pointing to the last Read() call's buffer (should be benign...).
Oh, I've found this in MSDN:

If this function is completed in an overlapped manner, it is the Winsock service
provider's responsibility to capture the WSABUF structures before returning from
this call. This enables applications to build stack-based WSABUF arrays pointed
to by the lpBuffers parameter.

So we don't need read_buffer_ and write_buffer_ to be members! Removing them

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:387: int UDPSocketWin::InternalRead() {
On 2011/03/10 06:53:38, Mike Belshe wrote:
> DCHECK(read_buffer_.buf)

Not needed anymore, as I removed read_buffer_

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.cc#ne...
net/udp/udp_socket_win.cc:418: int UDPSocketWin::InternalWrite() {
On 2011/03/10 06:53:38, Mike Belshe wrote:
> DCHECK(write_buffer_.buf);
same here.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h#new...
net/udp/udp_socket_win.h:19: #include "net/socket/client_socket.h"
On 2011/03/10 06:53:38, Mike Belshe wrote:
> nit: do you need client_socket.h?

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h#new...
net/udp/udp_socket_win.h:28: const net::NetLog::Source& source);
On 2011/03/10 06:53:38, Mike Belshe wrote:
> nit: spacing

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h#new...
net/udp/udp_socket_win.h:100: static const int kInvalidSocket = -1;
On 2011/03/10 06:53:38, Mike Belshe wrote:
> For windows, I'd get rid of kInvalidSocket, and just use INVALID_SOCKET (see
> also udp_socket_win.h:97)

Done.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h#new...
net/udp/udp_socket_win.h:161: int read_buf_len_;
On 2011/03/10 06:53:38, Mike Belshe wrote:
> I think we can get rid of read_iobuffer_ and read_buf_len_.  (in fact,
> read_buf_len_ is already unused).  You're now storing the buffer and length
into
> the read_buffer_, so I think read_iobuffer is just not necessary.
We do need read_iobuffer_ to keep the buffer alive while there is a pending
read. I removed read_buf_len_, write_buf_len_, read_buffer_, write_buffer_.

http://codereview.chromium.org/6658027/diff/3010/net/udp/udp_socket_win.h#new...
net/udp/udp_socket_win.h:169: int write_buf_len_;
On 2011/03/10 06:53:38, Mike Belshe wrote:
> Same comments - nix write_iobuffer_ and write_buf_len_.

Done.

Mike Belshe

9 years, 9 months ago (2011-03-11 14:31:28 UTC) #4

lgtm

http://codereview.chromium.org/6658027/diff/12015/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

http://codereview.chromium.org/6658027/diff/12015/net/udp/udp_socket_win.cc#n...
net/udp/udp_socket_win.cc:299: read_buffer.len = buf_len;
Ah - this is even better.  Great.

Expand Messages | Collapse Messages