Add support of IP_MULTICAST_IF in UDP sockets.

Add support of IP_MULTICAST_IF in UDP sockets.

BUG=319068
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238939

[Vitaly Buka (NO REVIEWS), 2013-12-02 21:43:40]

On 2013/12/02 21:25:28, Vitaly Buka wrote:

Nees some work, it shoulbe be done with SetSocketOptions

[Noam Samuel, 2013-12-02 22:05:53]

https://codereview.chromium.org/99923004/diff/20001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/99923004/diff/20001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:540: address.s_addr =
htonl(multicast_interface_);
I know that the 0.x.x.x address convention for numbered interfaces is used in
Windows, but are you sure it exists in Linux/Mac OSX?

https://codereview.chromium.org/99923004/diff/20001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:597: mreq.imr_interface.s_addr =
htonl(multicast_interface_);
Same question

[szym, 2013-12-03 07:42:11]

https://codereview.chromium.org/99923004/diff/20001/net/udp/udp_socket_libeve...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/99923004/diff/20001/net/udp/udp_socket_libeve...
net/udp/udp_socket_libevent.cc:540: address.s_addr =
htonl(multicast_interface_);
On 2013/12/02 22:05:54, Noam Samuel wrote:
> I know that the 0.x.x.x address convention for numbered interfaces is used in
> Windows, but are you sure it exists in Linux/Mac OSX?

In Linux, IP_MULTICAST_IF option has to be one of:
struct in_addr
struct ip_mreq
struct ip_mreqn

In the first two, the interface is looked up by IP address (ip_dev_find). In the
last one, if imr_ifindex is non-zero, then it is used as the interface index
(dev_get_by_index).

See: http://lxr.free-electrons.com/source/net/ipv4/ip_sockglue.c#L666

[szym, 2013-12-03 07:44:58]

Can you explain how adding this option will address the linked BUG?

In particular, how will the mDNS code determine the interface index to use in
the method call?

[Vitaly Buka (NO REVIEWS), 2013-12-03 09:40:41]

On 2013/12/03 07:44:58, szym wrote:
> Can you explain how adding this option will address the linked BUG?
> 

this CL is not enough, it's just part that could be submitted separately. 

> In particular, how will the mDNS code determine the interface index to use in
> the method call?

Plan was to extend GetNetworkList to return indexes.
https://codereview.chromium.org/100703002/

[Vitaly Buka (NO REVIEWS), 2013-12-03 11:17:04]

done

[szym, 2013-12-03 11:29:51]

On 2013/12/03 09:40:41, Vitaly Buka wrote:
> On 2013/12/03 07:44:58, szym wrote:
> > Can you explain how adding this option will address the linked BUG?
> > 
> 
> this CL is not enough, it's just part that could be submitted separately. 

I understand, but I couldn't find a clear picture of the whole solution
anywhere.

Could you please elaborate on the complete fix? Which component is going to
decide which interface index to pass to the multicast socket? How are network
changes going to be handled?

> 
> > In particular, how will the mDNS code determine the interface index to use
in
> > the method call?
> 
> Plan was to extend GetNetworkList to return indexes.
> https://codereview.chromium.org/100703002/

[Vitaly Buka (NO REVIEWS), 2013-12-03 11:38:44]

On 2013/12/03 11:29:51, szym wrote:
> On 2013/12/03 09:40:41, Vitaly Buka wrote:
> > On 2013/12/03 07:44:58, szym wrote:
> > > Can you explain how adding this option will address the linked BUG?
> > > 
> > 
> > this CL is not enough, it's just part that could be submitted separately. 
> 
> I understand, but I couldn't find a clear picture of the whole solution
> anywhere.
> 
> Could you please elaborate on the complete fix? Which component is going to
> decide which interface index to pass to the multicast socket? How are network
> changes going to be handled?

We are going to use all interfaces.

service_discovery_client.cc will enumerate interfaces and send list over IPC to
service_discovery_message_handler.cc	

service_discovery_message_handler.cc will implement MDnsSocketFactory and return
list of bound sockets

network changes handled buy full restart of service_discovery_client, with
re-binding of all sockets.

> 
> > 
> > > In particular, how will the mDNS code determine the interface index to use
> in
> > > the method call?
> > 
> > Plan was to extend GetNetworkList to return indexes.
> > https://codereview.chromium.org/100703002/

[szym, 2013-12-04 00:52:14]

I am concerned that passing invalid interface index will fail silently with bind
to "default".

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:36: uint32 GetIPv4AddressFromIndex(int socket,
uint32 index){
Add comment:
// Returns IPv4 address in network order.

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:44: return htonl(INADDR_ANY);
I am a bit concerned about this failure mode.

If the interface index is invalid (because for example the interface was turned
down), then we will end up binding to the default interface.

So, we might end up having multiple redundant sockets bound to the same
interface.

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h...
net/udp/udp_socket_win.h:135: int SetMulticastInterface(uint32 interface_index);
// Should be called before Bind().
// Returns a network error code.

[Vitaly Buka (NO REVIEWS), 2013-12-04 01:00:48]

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:44: return htonl(INADDR_ANY);
For current use-cases returning error is not a problem.
I'll update to return error so upper layer can decide what to do.

On 2013/12/04 00:52:15, szym wrote:
> I am a bit concerned about this failure mode.
> 
> If the interface index is invalid (because for example the interface was
turned
> down), then we will end up binding to the default interface.
> 
> So, we might end up having multiple redundant sockets bound to the same
> interface.

[Vitaly Buka (NO REVIEWS), 2013-12-04 09:39:36]

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:36: uint32 GetIPv4AddressFromIndex(int socket,
uint32 index){
On 2013/12/04 00:52:15, szym wrote:
> Add comment:
> // Returns IPv4 address in network order.

Done.

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
net/udp/udp_socket_libevent.cc:44: return htonl(INADDR_ANY);
On 2013/12/04 01:00:49, Vitaly Buka wrote:
> For current use-cases returning error is not a problem.
> I'll update to return error so upper layer can decide what to do.
> 
> On 2013/12/04 00:52:15, szym wrote:
> > I am a bit concerned about this failure mode.
> > 
> > If the interface index is invalid (because for example the interface was
> turned
> > down), then we will end up binding to the default interface.
> > 
> > So, we might end up having multiple redundant sockets bound to the same
> > interface.
> 

Done.

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h...
net/udp/udp_socket_win.h:135: int SetMulticastInterface(uint32 interface_index);
On 2013/12/04 00:52:15, szym wrote:
> // Should be called before Bind().
> // Returns a network error code.

Done.

[Vitaly Buka (NO REVIEWS), 2013-12-04 21:55:26]

On 2013/12/04 09:39:36, Vitaly Buka wrote:
>
https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
> File net/udp/udp_socket_libevent.cc (right):
> 
>
https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
> net/udp/udp_socket_libevent.cc:36: uint32 GetIPv4AddressFromIndex(int socket,
> uint32 index){
> On 2013/12/04 00:52:15, szym wrote:
> > Add comment:
> > // Returns IPv4 address in network order.
> 
> Done.
> 
>
https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_libev...
> net/udp/udp_socket_libevent.cc:44: return htonl(INADDR_ANY);
> On 2013/12/04 01:00:49, Vitaly Buka wrote:
> > For current use-cases returning error is not a problem.
> > I'll update to return error so upper layer can decide what to do.
> > 
> > On 2013/12/04 00:52:15, szym wrote:
> > > I am a bit concerned about this failure mode.
> > > 
> > > If the interface index is invalid (because for example the interface was
> > turned
> > > down), then we will end up binding to the default interface.
> > > 
> > > So, we might end up having multiple redundant sockets bound to the same
> > > interface.
> > 
> 
> Done.
> 
> https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h
> File net/udp/udp_socket_win.h (right):
> 
>
https://codereview.chromium.org/99923004/diff/160001/net/udp/udp_socket_win.h...
> net/udp/udp_socket_win.h:135: int SetMulticastInterface(uint32
interface_index);
> On 2013/12/04 00:52:15, szym wrote:
> > // Should be called before Bind().
> > // Returns a network error code.
> 
> Done.

+hclam for content/browser/renderer_host/p2p

[Alpha Left Google, 2013-12-04 21:56:43]

On 2013/12/04 21:55:35, Vitaly Buka wrote:

LGTM.

[szym, 2013-12-04 22:06:00]

lgtm

[commit-bot: I haz the power, 2013-12-04 22:07:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/99923004/180001

[commit-bot: I haz the power, 2013-12-04 22:29:00]

Retried try job too often on mac_rel for step(s) app_list_unittests,
base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests,
check_deps, chromedriver2_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
message_center_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-05 07:58:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/99923004/200001

[commit-bot: I haz the power, 2013-12-05 11:19:44]

Change committed as 238939