Added broadcasting feature to UDP server sockets.

Added broadcasting feature to UDP server sockets.


BUG=136797
TEST=net_unittests:UDPSocketTest.Broadcast

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146790

[wtc, 2012-07-11 18:26:37]

Unsolicited drive-by review comments on patch set 4:

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:449: reinterpret_cast<const char*>(&true_value),
Since the fourth argument to setsockopt is a const void* on Unix
(http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html),
we should not need to cast &true_value to const char* here
and on lines 455 and 461.

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:453: #if defined(OS_MACOSX)

If SO_REUSEPORT is defined as a macro (rather than an enum
constant) on Mac OS X, you can also test

    #if defined(SO_REUSEPORT)

here.  Then the code will automatically work on other platforms
that define the SO_REUSEPORT macro.

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

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_win.cc#...
net/udp/udp_socket_win.cc:448: int true_value = 1;

It looks nicer to use the BOOL type here:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740476(v=vs.85).aspx

BOOL and int have the same size on Windows, so this doesn't
really matter.

The fourth argument to setsockopt is a const char* on Windows,
so the casts to const char* are necessary in this file.

[ygorshenin1, 2012-07-12 09:28:08]

PTAL

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:449: reinterpret_cast<const char*>(&true_value),
On 2012/07/11 18:26:37, wtc wrote:
> Since the fourth argument to setsockopt is a const void* on Unix
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/setsockopt.html),
> we should not need to cast &true_value to const char* here
> and on lines 455 and 461.

Done.

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:453: #if defined(OS_MACOSX)
On 2012/07/11 18:26:37, wtc wrote:
> 
> If SO_REUSEPORT is defined as a macro (rather than an enum
> constant) on Mac OS X, you can also test
> 
>     #if defined(SO_REUSEPORT)
> 
> here.  Then the code will automatically work on other platforms
> that define the SO_REUSEPORT macro.

Done.

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

http://codereview.chromium.org/10739002/diff/21001/net/udp/udp_socket_win.cc#...
net/udp/udp_socket_win.cc:448: int true_value = 1;
On 2012/07/11 18:26:37, wtc wrote:
> 
> It looks nicer to use the BOOL type here:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740476%28v=vs.85%29...
> 
> BOOL and int have the same size on Windows, so this doesn't
> really matter.
> 
> The fourth argument to setsockopt is a const char* on Windows,
> so the casts to const char* are necessary in this file.

Done.

[ygorshenin1, 2012-07-12 14:05:13]

+ Sergey

Sergey, could you please review content/* part?

[Sergey Ulanov, 2012-07-12 18:15:37]

LGTM for content/browser/renderer_host/p2p, but please see my nits.

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_udp_unittest.cc (right):

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:111: }
NOTIMPLEMENTED()

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:114: }
NOTIMPLEMENTED()

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
File net/udp/datagram_server_socket.h (right):

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:64: // be bound with other sockets. Should be
called before Listen.
nit: s/Listen/Listen()/

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:67: // Makes the socket ready for broadcasting.
Should be called before
This comment is not very clear. Suggest "Allow sending and receiving packets
sent to and from broadcast addresses."

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:68: // Listen.
nit: s/Listen/Listen()/

[wtc, 2012-07-12 23:49:47]

Patch set 6 LGTM.

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
File net/udp/datagram_server_socket.h (right):

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:64: // be bound with other sockets. Should be
called before Listen.

It would be nice to point out that this is often about
sharing the address with (the sockets in) other processes.

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libevent.h
File net/udp/udp_socket_libevent.h (right):

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.h:108: void AllowBroadcast();

NOTE: all the changes I suggest for this file also apply to
net/udp/udp_socket_win.h .

Document these two methods.

They must be called before Bind.  (This class doesn't have the
Listen method.)

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.h:189: int socket_options_;

Nit: document these members.

[ygorshenin1, 2012-07-16 10:29:43]

Many thanks!

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
File content/browser/renderer_host/p2p/socket_host_udp_unittest.cc (right):

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:111: }
On 2012/07/12 18:15:37, sergeyu wrote:
> NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/10739002/diff/21003/content/browser/renderer_h...
content/browser/renderer_host/p2p/socket_host_udp_unittest.cc:114: }
On 2012/07/12 18:15:37, sergeyu wrote:
> NOTIMPLEMENTED()

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
File net/udp/datagram_server_socket.h (right):

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:64: // be bound with other sockets. Should be
called before Listen.
On 2012/07/12 18:15:37, sergeyu wrote:
> nit: s/Listen/Listen()/

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:64: // be bound with other sockets. Should be
called before Listen.
On 2012/07/12 23:49:47, wtc wrote:
> 
> It would be nice to point out that this is often about
> sharing the address with (the sockets in) other processes.

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:67: // Makes the socket ready for broadcasting.
Should be called before
On 2012/07/12 18:15:37, sergeyu wrote:
> This comment is not very clear. Suggest "Allow sending and receiving packets
> sent to and from broadcast addresses."

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/datagram_server_so...
net/udp/datagram_server_socket.h:68: // Listen.
On 2012/07/12 18:15:37, sergeyu wrote:
> nit: s/Listen/Listen()/

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libevent.h
File net/udp/udp_socket_libevent.h (right):

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.h:108: void AllowBroadcast();
On 2012/07/12 23:49:47, wtc wrote:
> 
> NOTE: all the changes I suggest for this file also apply to
> net/udp/udp_socket_win.h .
> 
> Document these two methods.
> 
> They must be called before Bind.  (This class doesn't have the
> Listen method.)

Done.

http://codereview.chromium.org/10739002/diff/21003/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.h:189: int socket_options_;
On 2012/07/12 23:49:47, wtc wrote:
> 
> Nit: document these members.

Done.

[commit-bot: I haz the power, 2012-07-16 10:32:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/10739002/32001

[commit-bot: I haz the power, 2012-07-16 11:52:37]

Change committed as 146790

[Sergey Ulanov, 2012-09-12 19:13:10]

Sorry for belated comments, but I've just noticed that we set SO_REUSEPORT now
and have questions about it.

https://chromiumcodereview.appspot.com/10739002/diff/32001/net/udp/udp_socket...
File net/udp/udp_socket_libevent.cc (right):

https://chromiumcodereview.appspot.com/10739002/diff/32001/net/udp/udp_socket...
net/udp/udp_socket_libevent.cc:459: rv = setsockopt(socket_, SOL_SOCKET,
SO_REUSEPORT, &true_value,
Why do we really need this option here? It seems like if we have it here then
AllowAddressReuse() will behave differently on Mac and Windows. Also I'm not
sure if implementation of SO_REUSEPORT is consistent between Linux and Mac -
e.g. Mac's man page mentions how this option changes behavior for broadcast
sockets, but Linux docs don't. If we need it on mac to receive broadcast
messages properly, then maybe enable it only when broadcast is enabled?

[ygorshenin1, 2012-09-13 18:29:16]

http://codereview.chromium.org/10739002/diff/32001/net/udp/udp_socket_libeven...
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/10739002/diff/32001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:459: rv = setsockopt(socket_, SOL_SOCKET,
SO_REUSEPORT, &true_value,
Thanks, Sergey, you're right. SO_REUSEPORT is used together with a
broadcasting/multicasting. Please, take a look at
http://codereview.chromium.org/10917235/.

On 2012/09/12 19:13:10, sergeyu wrote:
> Why do we really need this option here? It seems like if we have it here then
> AllowAddressReuse() will behave differently on Mac and Windows. Also I'm not
> sure if implementation of SO_REUSEPORT is consistent between Linux and Mac -
> e.g. Mac's man page mentions how this option changes behavior for broadcast
> sockets, but Linux docs don't. If we need it on mac to receive broadcast
> messages properly, then maybe enable it only when broadcast is enabled?