Detect IPv6 interface is temporary or not.

Detect IPv6 interface is temporary or not.

If a interface has both permanent and temporary address, then based
on the policy setting provide both or temporary address only.

Due to privacy concerns webrtc don't want to expose permanent addresses to remote endpoints.

[agl, 2014-03-27 14:52:52]

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:534: enum HostScopeVirtualInterfacePolicy {
The name and comment of this enum are no longer correct.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:536: EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES       = 0x2,
why not assign zero to EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:537: // Include temp address only when interface both
permanent and temp addresses.
missing "has"?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:538: INCLUDE_IPV6_TEMP_ADDRESS_IF_AVAILABLE      = 0x4,
INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE ?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:45: // IPv6 and it's temporary this field will be set
to false.
remove "set to"

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:49: // This method will remove permanent IPv6
address, if a temporary address
remove comma

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:51: void
PruneIPv6Addresses(std::vector<NetworkInterfaceInfo>* infos) {
I would go nuts with the name in order to make this clear, Blink style:

RemovePermanentIPv6AddressesWhereTemporaryExists

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:57: std::vector<NetworkInterfaceInfo>::iterator iter
= infos->begin();
If you just just |i| for iter then you can use |j| rather than |piter|, below.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:58: for (; iter != infos->end(); ++iter) {
do the assignment of iter in the first for clause.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:59: if (iter->permanent &&
iter->interface.address.size() == kIPv6AddressSize) {
This is pushing permanent addresses into a list called temp_ipv6_list and also
contradicts the comment on L55.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:66: iter = temp_ipv6_list.begin();
move into the for loop.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:68: std::string name = iter->interface.name;
const &?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:73: if (piter->interface.name !=
iter->interface.name)
RHS should be |name|?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:78: }
Isn't it easier to do one pass building a set?:

set<string> ifaces_with_temp_addrs

for addr in addrs {
  if is_temp(addr) {
    ifaces_with_temp_addrs.add(addr.iface)
  }
}

for addr in addrs {
  if is_permanent(addr) && ifaces_with_temp_addrs.contains(addr.iface) {
    addrs.remove(addr)
  }
}

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:224: ioctl_socket > 0 && addr->sa_family == AF_INET6)
{
ioctl_socket >= 0

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:257: close(ioctl_socket);
guard this by an if - we shouldn't call close(-1) if we can help it.

[Mallinath (Gone from Chromium), 2014-03-27 19:04:18]

PTAL

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:534: enum HostScopeVirtualInterfacePolicy {
On 2014/03/27 14:52:53, agl wrote:
> The name and comment of this enum are no longer correct.

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:536: EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES       = 0x2,
On 2014/03/27 14:52:53, agl wrote:
> why not assign zero to EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES?

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:537: // Include temp address only when interface both
permanent and temp addresses.
On 2014/03/27 14:52:53, agl wrote:
> missing "has"?

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util.h#newc...
net/base/net_util.h:538: INCLUDE_IPV6_TEMP_ADDRESS_IF_AVAILABLE      = 0x4,
On 2014/03/27 14:52:53, agl wrote:
> INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE ?

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:45: // IPv6 and it's temporary this field will be set
to false.
On 2014/03/27 14:52:53, agl wrote:
> remove "set to"

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:49: // This method will remove permanent IPv6
address, if a temporary address
On 2014/03/27 14:52:53, agl wrote:
> remove comma

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:51: void
PruneIPv6Addresses(std::vector<NetworkInterfaceInfo>* infos) {
I Agree. 
On 2014/03/27 14:52:53, agl wrote:
> I would go nuts with the name in order to make this clear, Blink style:
> 
> RemovePermanentIPv6AddressesWhereTemporaryExists

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:57: std::vector<NetworkInterfaceInfo>::iterator iter
= infos->begin();
On 2014/03/27 14:52:53, agl wrote:
> If you just just |i| for iter then you can use |j| rather than |piter|, below.

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:58: for (; iter != infos->end(); ++iter) {
On 2014/03/27 14:52:53, agl wrote:
> do the assignment of iter in the first for clause.

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:59: if (iter->permanent &&
iter->interface.address.size() == kIPv6AddressSize) {
Yea, I discovered that later.
On 2014/03/27 14:52:53, agl wrote:
> This is pushing permanent addresses into a list called temp_ipv6_list and also
> contradicts the comment on L55.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:66: iter = temp_ipv6_list.begin();
On 2014/03/27 14:52:53, agl wrote:
> move into the for loop.

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:68: std::string name = iter->interface.name;
not needed.
On 2014/03/27 14:52:53, agl wrote:
> const &?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:73: if (piter->interface.name !=
iter->interface.name)
removed now.
On 2014/03/27 14:52:53, agl wrote:
> RHS should be |name|?

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:78: }
Cool, that makes life bit easy.
On 2014/03/27 14:52:53, agl wrote:
> Isn't it easier to do one pass building a set?:
> 
> set<string> ifaces_with_temp_addrs
> 
> for addr in addrs {
>   if is_temp(addr) {
>     ifaces_with_temp_addrs.add(addr.iface)
>   }
> }
> 
> for addr in addrs {
>   if is_permanent(addr) && ifaces_with_temp_addrs.contains(addr.iface) {
>     addrs.remove(addr)
>   }
> }

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:224: ioctl_socket > 0 && addr->sa_family == AF_INET6)
{
On 2014/03/27 14:52:53, agl wrote:
> ioctl_socket >= 0

Done.

https://codereview.chromium.org/213423004/diff/40002/net/base/net_util_posix....
net/base/net_util_posix.cc:257: close(ioctl_socket);
On 2014/03/27 14:52:53, agl wrote:
> guard this by an if - we shouldn't call close(-1) if we can help it.

Done.

[agl, 2014-03-27 19:11:40]

LGTM

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:50: // This method will remove permanent IPv6 address
if a temporary address
s/address/addresses/ (just the first instance)

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:57: // Pushing all IPv6 temp addresses interface name
to a different list.
// Build a set containing the names of interfaces with a temp IPv6 address.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:66: // If the temp ipv6 list is empty, no work to be
done further.
// The there are no such interfaces then there's no further work.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:70: // Search for permenent address belonging to same
network interface.
s/address/addresses.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:207: if (policy &
EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES &&
( ) around "policy & EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES" because & is
surprisingly weak in C.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:207: if (policy &
EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES &&
This will never run because EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES is zero. The
value that is tested needs to be the non-zero one.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:217: if (policy &
INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE &&
( ) around "policy & INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE"

[Mallinath (Gone from Chromium), 2014-03-27 19:54:18]

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix.cc
File net/base/net_util_posix.cc (right):

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:50: // This method will remove permanent IPv6 address
if a temporary address
On 2014/03/27 19:11:40, agl wrote:
> s/address/addresses/ (just the first instance)

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:57: // Pushing all IPv6 temp addresses interface name
to a different list.
On 2014/03/27 19:11:40, agl wrote:
> // Build a set containing the names of interfaces with a temp IPv6 address.

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:66: // If the temp ipv6 list is empty, no work to be
done further.
On 2014/03/27 19:11:40, agl wrote:
> // The there are no such interfaces then there's no further work.

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:66: // If the temp ipv6 list is empty, no work to be
done further.
On 2014/03/27 19:11:40, agl wrote:
> // The there are no such interfaces then there's no further work.

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:70: // Search for permenent address belonging to same
network interface.
On 2014/03/27 19:11:40, agl wrote:
> s/address/addresses.

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:207: if (policy &
EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES &&
On 2014/03/27 19:11:40, agl wrote:
> ( ) around "policy & EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES" because & is
> surprisingly weak in C.

Done.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:207: if (policy &
EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES &&
oops. Yea. Changed the value in .h to non-zero.
On 2014/03/27 19:11:40, agl wrote:
> This will never run because EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES is zero. The
> value that is tested needs to be the non-zero one.

https://codereview.chromium.org/213423004/diff/110001/net/base/net_util_posix...
net/base/net_util_posix.cc:217: if (policy &
INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE &&
On 2014/03/27 19:11:40, agl wrote:
> ( ) around "policy & INCLUDE_ONLY_TEMP_IPV6_ADDRESS_IF_POSSIBLE"

Done.

[agl, 2014-03-28 15:31:41]

LGTM

https://codereview.chromium.org/213423004/diff/170001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/213423004/diff/170001/net/base/net_util.h#new...
net/base/net_util.h:536: INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES           = 0x2,
These are again two different flags, but I believe that only two possibilities
are reasonable, so it's really just one flag:

EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES = 1,
INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES = 0,

Before you wanted to test EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES - but that was
the zero valued one, that's all.

[Mallinath (Gone from Chromium), 2014-03-28 17:43:46]

On 2014/03/28 15:31:41, agl wrote:
> LGTM
> 
> https://codereview.chromium.org/213423004/diff/170001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/213423004/diff/170001/net/base/net_util.h#new...
> net/base/net_util.h:536: INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES           =
0x2,
> These are again two different flags, but I believe that only two possibilities
> are reasonable, so it's really just one flag:
> 
> EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES = 1,
> INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES = 0,
> 
> Before you wanted to test EXCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES - but that was
> the zero valued one, that's all.

I am not sure why this CL is not closed yet, as yesterday I committed this CL.

Thanks for the review, I will try to submit your suggestion in a different CL. I
am thinking to 
remove INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES all together from the code as we
really don't need it.