Added NetworkInterface::network_prefix

Added NetworkInterface::network_prefix.

BUG=295997
NOTRY=true

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

R=szym@chromium.org

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

[szym, 2013-09-19 23:21:01]

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

https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
net/base/net_util.h:528: // network_prefix on Android is not easily accessible.
Why not? This is easily done with ioctl SIOCGIFNETMASK.

[szym, 2013-09-19 23:30:06]

On 2013/09/19 23:21:01, szym wrote:
> https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
> net/base/net_util.h:528: // network_prefix on Android is not easily
accessible.
> Why not? This is easily done with ioctl SIOCGIFNETMASK.

Nevermind. Turns out this ioctl is restricted (EACCESS).

[Vitaly Buka (NO REVIEWS), 2013-09-20 01:09:18]

On 2013/09/19 23:30:06, szym wrote:
> On 2013/09/19 23:21:01, szym wrote:
> > https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h
> > File net/base/net_util.h (right):
> > 
> >
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
> > net/base/net_util.h:528: // network_prefix on Android is not easily
> accessible.
> > Why not? This is easily done with ioctl SIOCGIFNETMASK.
> 
> Nevermind. Turns out this ioctl is restricted (EACCESS).

so can I get lgtm?

[Vitaly Buka (NO REVIEWS), 2013-09-20 01:15:30]

On 2013/09/20 01:09:18, Vitaly Buka wrote:
> On 2013/09/19 23:30:06, szym wrote:
> > On 2013/09/19 23:21:01, szym wrote:
> > > https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h
> > > File net/base/net_util.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
> > > net/base/net_util.h:528: // network_prefix on Android is not easily
> > accessible.
> > > Why not? This is easily done with ioctl SIOCGIFNETMASK.
> > 
> > Nevermind. Turns out this ioctl is restricted (EACCESS).
> 
> so can I get lgtm?

We are going to use this for Local Discovery to enable notification only small
/24 networks.

[szym, 2013-09-20 19:30:15]

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

https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
net/base/net_util.h:528: // network_prefix on Android is not easily accessible.
On 2013/09/19 23:21:01, szym wrote:
> Why not? This is easily done with ioctl SIOCGIFNETMASK.

While the ioctl is forbidden (because it would bypass the INTERNET permission),
I think we should update AndroidNetworkLibrary.getNetworkList to include network
prefix.

This just means using NetworkInterface.getInterfaceAddresses instead of
getInetAddresses.

I understand you don't currently need it on Android, but I don't think it's that
much more work.

https://codereview.chromium.org/23726043/diff/14001/net/base/net_util_unittes...
File net/base/net_util_unittest.cc (right):

https://codereview.chromium.org/23726043/diff/14001/net/base/net_util_unittes...
net/base/net_util_unittest.cc:3316: it->network_prefix <= 32u);
Point-to-point addresses have no netmask. Will they have network_prefix == 32 or
0?

[Vitaly Buka (NO REVIEWS), 2013-09-20 22:51:45]

On 2013/09/20 19:30:15, szym wrote:
> https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h
> File net/base/net_util.h (right):
> 
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util.h#newco...
> net/base/net_util.h:528: // network_prefix on Android is not easily
accessible.
> On 2013/09/19 23:21:01, szym wrote:
> > Why not? This is easily done with ioctl SIOCGIFNETMASK.
> 
> While the ioctl is forbidden (because it would bypass the INTERNET
permission),
> I think we should update AndroidNetworkLibrary.getNetworkList to include
network
> prefix.
> 
> This just means using NetworkInterface.getInterfaceAddresses instead of
> getInetAddresses.
> 
> I understand you don't currently need it on Android, but I don't think it's
that
> much more work.
> 
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util_unittes...
> File net/base/net_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/23726043/diff/14001/net/base/net_util_unittes...
> net/base/net_util_unittest.cc:3316: it->network_prefix <= 32u);
> Point-to-point addresses have no netmask. Will they have network_prefix == 32
or
> 0?

31 according http://tools.ietf.org/html/rfc3021

[Vitaly Buka (NO REVIEWS), 2013-09-21 01:56:34]

done

[szym, 2013-09-23 17:26:55]

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:184:
addressString.append(",");
Suggest "/" to follow the established convention. Ok, if you don't want it
because you want to use a single tokenizer.

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

https://codereview.chromium.org/23726043/diff/57001/net/base/net_util_posix.c...
net/base/net_util_posix.cc:81: continue;
I don't quite understand why the code here silently ignores unexpected format.
android::GetNetworkList comes from across the JNI barrier, but there's no reason
why it would be inconsistent (there's no hot updating on Android).

I really think these should all be CHECKed instead.

https://codereview.chromium.org/23726043/diff/57001/net/base/net_util_posix.c...
net/base/net_util_posix.cc:91: NOTREACHED();
This should be consistent with the rest of the code. I.e., either CHECK all,
DCHECK all (NOTREACHED), or |if (unexpected) continue;|

https://codereview.chromium.org/23726043/diff/57001/net/base/net_util_unittes...
File net/base/net_util_unittest.cc (right):

https://codereview.chromium.org/23726043/diff/57001/net/base/net_util_unittes...
net/base/net_util_unittest.cc:3313: LOG(WARNING) <<
IPAddressToString(it->address) << " " << it->network_prefix;
Don't forget to remove.

[Vitaly Buka (NO REVIEWS), 2013-09-23 17:57:16]

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:177:
addressString.append(",");
Actually there is could be bigger issue
do we know whether the name can have ',' or '/'?
It must be escaped somehow or returned as array<array<string>>

[szym, 2013-09-23 18:55:51]

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:177:
addressString.append(",");
On 2013/09/23 17:57:16, Vitaly Buka wrote:
> Actually there is could be bigger issue
> do we know whether the name can have ',' or '/'?
> It must be escaped somehow or returned as array<array<string>>

Interface names are Linux device names, so they definitely cannot have '/'.
AFAICT, they can include a..z, 0..9, and ".:_-"

[Vitaly Buka (NO REVIEWS), 2013-09-23 19:49:16]

done

On 2013/09/23 18:55:51, szym wrote:
>
https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
> File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):
> 
>
https://codereview.chromium.org/23726043/diff/57001/net/android/java/src/org/...
> net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:177:
> addressString.append(",");
> On 2013/09/23 17:57:16, Vitaly Buka wrote:
> > Actually there is could be bigger issue
> > do we know whether the name can have ',' or '/'?
> > It must be escaped somehow or returned as array<array<string>>
> 
> Interface names are Linux device names, so they definitely cannot have '/'.
> AFAICT, they can include a..z, 0..9, and ".:_-"

[szym, 2013-09-23 19:58:41]

lgtm

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

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_posix.c...
net/base/net_util_posix.cc:88: CHECK(net_mask <= 255);
CHECK_LE

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_unittes...
File net/base/net_util_unittest.cc (right):

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_unittes...
net/base/net_util_unittest.cc:3314: it->network_prefix <= 32u);
Let's make it:
EXPECT_LE(it->network_prefix, it->address.size() * 8)

[Vitaly Buka (NO REVIEWS), 2013-09-23 20:06:11]

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

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_posix.c...
net/base/net_util_posix.cc:88: CHECK(net_mask <= 255);
On 2013/09/23 19:58:42, szym wrote:
> CHECK_LE

Done.

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_unittes...
File net/base/net_util_unittest.cc (right):

https://codereview.chromium.org/23726043/diff/13001/net/base/net_util_unittes...
net/base/net_util_unittest.cc:3314: it->network_prefix <= 32u);
On 2013/09/23 19:58:42, szym wrote:
> Let's make it:
> EXPECT_LE(it->network_prefix, it->address.size() * 8)

Done.

[commit-bot: I haz the power, 2013-09-23 20:06:22]

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

[commit-bot: I haz the power, 2013-09-23 21:19:01]

Retried try job too often on linux_chromeos for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-23 21:39:47]

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

[commit-bot: I haz the power, 2013-09-24 05:03:07]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-09-25 05:48:33]

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

[commit-bot: I haz the power, 2013-09-25 06:43:10]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-25 06:58:03]

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

[commit-bot: I haz the power, 2013-09-25 10:26:04]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-25 17:24:54]

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

[commit-bot: I haz the power, 2013-09-25 21:30:29]

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

[commit-bot: I haz the power, 2013-09-26 03:14:19]

Change committed as 225303

[Vitaly Buka (NO REVIEWS), 2013-09-26 06:12:33]

Committed patchset #7 manually as r225353 (presubmit successful).