SO_REUSEPORT is moved from AddressReuse feature to Broadcast feature.

SO_REUSEPORT socket option is set up when Broadcast feature is requested. Previously it's not caused any bugs because when broadcasting feature is needed, address reuse feature is set up too.

BUG=none
TEST=net_unittests:UDPSocketTest.*


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

[Sergey Ulanov, 2012-09-13 18:20:27]

Thanks for following up on this.
It's still not clear to me why we need this flag at all, and  how it works on
windows without this flag. Can you please add comments to explain why it's
required for broadcast sockets?
There is also problem with consistency across platforms. Having this flag on
Mac, but not on Windows means that broadcast sockets may behave differently
depending on OS.

[ygorshenin1, 2012-09-13 18:31:33]

We have this flag only on Mac, and it's required only for Mac. This flag is
unavailable for Linux or Windows.

On 2012/09/13 18:20:27, sergeyu wrote:
> Thanks for following up on this.
> It's still not clear to me why we need this flag at all, and  how it works on
> windows without this flag. Can you please add comments to explain why it's
> required for broadcast sockets?
> There is also problem with consistency across platforms. Having this flag on
> Mac, but not on Windows means that broadcast sockets may behave differently
> depending on OS.

[Sergey Ulanov, 2012-09-13 18:54:18]

On 2012/09/13 18:31:33, ygorshenin1 wrote:
> We have this flag only on Mac, and it's required only for Mac. This flag is
> unavailable for Linux or Windows.

Actually it is available on Linux with newer kernel. E.g. on Ubuntu 12.4:
sergeyu@veles:~$ grep -R SO_REUSEPORT /usr/include/
/usr/include/asm-generic/socket.h:/* To add :#define SO_REUSEPORT 15 */

From what I know implementation may not be consistent with OSX. That's why I
don't like #if defined(SO_REUSEPORT) - it means that behavior depends on which
version of headers we have when compiling this code (i.e. we could get ourselves
in the state when ChromeOS and Linux builds work differently).

But anyway, first of all I want to understand _why_ we need this flag on OSX?
What happens if you remove it? please add comments in the code to explain why
and when we need it.


> 
> On 2012/09/13 18:20:27, sergeyu wrote:
> > Thanks for following up on this.
> > It's still not clear to me why we need this flag at all, and  how it works
on
> > windows without this flag. Can you please add comments to explain why it's
> > required for broadcast sockets?
> > There is also problem with consistency across platforms. Having this flag on
> > Mac, but not on Windows means that broadcast sockets may behave differently
> > depending on OS.

[Sergey Ulanov, 2012-09-13 19:56:58]

On 2012/09/13 18:54:18, sergeyu wrote:
> On 2012/09/13 18:31:33, ygorshenin1 wrote:
> > We have this flag only on Mac, and it's required only for Mac. This flag is
> > unavailable for Linux or Windows.
> 
> Actually it is available on Linux with newer kernel. E.g. on Ubuntu 12.4:
> sergeyu@veles:~$ grep -R SO_REUSEPORT /usr/include/
> /usr/include/asm-generic/socket.h:/* To add :#define SO_REUSEPORT 15 */
> 

I'm wrong here. Looking in the kernel source it looks like SO_REUSEPORT it's not
actually implemented. Still it's better to keep this option platform-specific
i.e. Mac-only, and not enabled-disable it based on the presence of that option.


> From what I know implementation may not be consistent with OSX. That's why I
> don't like #if defined(SO_REUSEPORT) - it means that behavior depends on which
> version of headers we have when compiling this code (i.e. we could get
ourselves
> in the state when ChromeOS and Linux builds work differently).
> 
> But anyway, first of all I want to understand _why_ we need this flag on OSX?
> What happens if you remove it? please add comments in the code to explain why
> and when we need it.
> 
> 
> > 
> > On 2012/09/13 18:20:27, sergeyu wrote:
> > > Thanks for following up on this.
> > > It's still not clear to me why we need this flag at all, and  how it works
> on
> > > windows without this flag. Can you please add comments to explain why it's
> > > required for broadcast sockets?
> > > There is also problem with consistency across platforms. Having this flag
on
> > > Mac, but not on Windows means that broadcast sockets may behave
differently
> > > depending on OS.

[ygorshenin1, 2012-09-14 14:09:40]

PTAL

[Sergey Ulanov, 2012-09-14 19:38:45]

FYI: I will be offline next week. Please add willchan@ or wtc@ to finish this
review.

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:471: #if defined(OS_MACOSX) &&
defined(SO_REUSEPORT)
You don't need defined(SO_REUSEPORT) here - we expect to always have it on Mac.

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:472: rv = setsockopt(socket_, SOL_SOCKET,
SO_REUSEPORT, &true_value,
Please add comments here to explain why we need this.

[wtc, 2012-09-17 22:58:30]

ygorshenin: I reviewed patch set 3, but you haven't made the
two changes that sergeyu suggested. I will wait for a new
patch set.

[ygorshenin1, 2012-09-18 11:34:05]

Sorry for the slow response, PTAL

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent.cc
File net/udp/udp_socket_libevent.cc (right):

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:471: #if defined(OS_MACOSX) &&
defined(SO_REUSEPORT)
On 2012/09/14 19:38:45, sergeyu wrote:
> You don't need defined(SO_REUSEPORT) here - we expect to always have it on
Mac.

Done.

http://codereview.chromium.org/10917235/diff/3002/net/udp/udp_socket_libevent...
net/udp/udp_socket_libevent.cc:472: rv = setsockopt(socket_, SOL_SOCKET,
SO_REUSEPORT, &true_value,
On 2012/09/14 19:38:45, sergeyu wrote:
> Please add comments here to explain why we need this.

Done.

[wtc, 2012-09-18 14:40:32]

Patch set 4 LGTM.

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

http://codereview.chromium.org/10917235/diff/15001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:472: // SO_REUSEPORT on OSX permits multiple
instances of a program to

Did you mean "multiple instances of the same program" or
"multiple programs"?

[ygorshenin1, 2012-09-18 14:54:22]

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

http://codereview.chromium.org/10917235/diff/15001/net/udp/udp_socket_libeven...
net/udp/udp_socket_libevent.cc:472: // SO_REUSEPORT on OSX permits multiple
instances of a program to
I thought "multiple processes" will be better. Thanks!

On 2012/09/18 14:40:32, wtc wrote:
> 
> Did you mean "multiple instances of the same program" or
> "multiple programs"?

[wtc, 2012-09-18 15:07:21]

Patch set 5 LGTM. Thanks.

[commit-bot: I haz the power, 2012-09-18 15:23:47]

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

[commit-bot: I haz the power, 2012-09-18 17:29:31]

Change committed as 157373