[net] Adds AddressTrackerLinux which keeps track of interface addresses using rtnetlink.

[net] Adds AddressTrackerLinux which keeps track of interface addresses using rtnetlink.

BUG=100690, 113993
TEST=./net_unittests --gtest_filter=AddressTrackerLinuxTest.*

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

[szym, 2012-06-27 22:15:12]

This is essential for RFC3484, but should also improve IPv6 address tracking for
NetworkChangeNotifierLinux.

[willchan no longer on Chromium, 2012-06-28 01:36:44]

I'm happy to defer this to another Linux guy...if there's anyone else who'd
like to review it. Otherwise I'll hopefully get around to it later tomorrow
or Friday (I leave for vacation on Friday).


On Wed, Jun 27, 2012 at 3:15 PM, <szym@chromium.org> wrote:

> Reviewers: willchan,
>
> Message:
> This is essential for RFC3484, but should also improve IPv6 address
> tracking for
> NetworkChangeNotifierLinux.
>
>
> Description:
> [net] Adds AddressTrackerLinux which keeps track of interface addresses
> using
> rtnetlink.
>
> BUG=100690,113993
>
>
> Please review this at
http://codereview.chromium.**org/10689015/<http://codereview.chromium.org/106...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  A net/base/address_tracker_**linux.h
>  A net/base/address_tracker_**linux.cc
>  M net/base/network_change_**notifier_linux.h
>  M net/base/network_change_**notifier_linux.cc
>  D net/base/network_change_**notifier_netlink_linux.h
>  D net/base/network_change_**notifier_netlink_linux.cc
>  M net/net.gyp
>
>
>

[willchan no longer on Chromium, 2012-06-29 01:53:41]

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:36: unsigned char *address = NULL;
be consistent about type* vs type *. chromium prefers type*.

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:65: close(netlink_fd_);
HANDLE_EINTR()

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
net/base/address_tracker_linux.h:52: mutable base::Lock lock_;
Why is the lock necessary? What threads are accessing this?

[szym, 2012-06-29 03:06:09]

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/20001/net/base/address_tracker_l...
net/base/address_tracker_linux.h:52: mutable base::Lock lock_;
On 2012/06/29 01:53:41, willchan wrote:
> Why is the lock necessary? What threads are accessing this?

AddressTrackerLinux will live on the NetworkChangeNotifierLinux::Thread. But
GetMap() will be called from the network thread, by AddressSorter (in response
to OnIPAddressChanged). See:
http://codereview.chromium.org/10442098/

I plan to add NetworkChangeNotifier::GetAddressTracker() (#if
defined(OS_LINUX)).

[szym, 2012-06-29 04:20:15]

vandebo: Would you be able to review this?

[vandebo (ex-Chrome), 2012-06-29 17:52:03]

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:132: ReadMessages();
Do you need to call ReadMessages here?  Won't watch file descriptor call it in a
little bit?  If not (because you're racing the result), why not setup the
watcher before sending the dump request?

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:165: switch (header->nlmsg_type) {
So this logic is no worse than what we had before, but it doesn't solve the IPv6
privacy extension problem.  To address that, you'd have to compare arriving and
leaving addresses to other addresses to see if a new network is available.

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
net/base/network_change_notifier_linux.h:34: friend class
internal::AddressTrackerLinux;
friends make my teeth crawl, can you provide a delegate instead?

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
File net/base/network_change_notifier_netlink_linux.cc (left):

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
net/base/network_change_notifier_netlink_linux.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
You might try doing git mv/svn mv so that the diff is easier to read.

[szym, 2012-06-29 18:34:52]

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:132: ReadMessages();
On 2012/06/29 17:52:03, vandebo wrote:
> Do you need to call ReadMessages here?  Won't watch file descriptor call it in
a
> little bit?  If not (because you're racing the result), why not setup the
> watcher before sending the dump request?

The intention was to read all messages in response to the dump request before
calling NotifyObserversOfIPAddressChange. That said, I think it makes sense to
actually send the notification afterwards since the consumers of
AddressTrackerLinux::GetMap would like to get the info once it's complete.

I removed it from here.

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:165: switch (header->nlmsg_type) {
On 2012/06/29 17:52:03, vandebo wrote:
> So this logic is no worse than what we had before, but it doesn't solve the
IPv6
> privacy extension problem.  To address that, you'd have to compare arriving
and
> leaving addresses to other addresses to see if a new network is available.

The goal was to filter out RTM_NEWADDR announcements which don't change the IP
address. This code will only set |changed = true| if the address is completely
new or if the ifaddrmsg content has changed. I don't think the router
announcements mentioned in http://crbug.com/100690 change ifaddrmsg.

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
net/base/network_change_notifier_linux.h:34: friend class
internal::AddressTrackerLinux;
On 2012/06/29 17:52:03, vandebo wrote:
> friends make my teeth crawl, can you provide a delegate instead?

Done.

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
File net/base/network_change_notifier_netlink_linux.cc (left):

http://codereview.chromium.org/10689015/diff/15002/net/base/network_change_no...
net/base/network_change_notifier_netlink_linux.cc:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2012/06/29 17:52:03, vandebo wrote:
> You might try doing git mv/svn mv so that the diff is easier to read.

diff shows very little similarity between this file and address_tracker_linux.cc

[vandebo (ex-Chrome), 2012-06-29 21:01:18]

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:165: switch (header->nlmsg_type) {
On 2012/06/29 18:34:52, szym wrote:
> On 2012/06/29 17:52:03, vandebo wrote:
> > So this logic is no worse than what we had before, but it doesn't solve the
> IPv6
> > privacy extension problem.  To address that, you'd have to compare arriving
> and
> > leaving addresses to other addresses to see if a new network is available.
> 
> The goal was to filter out RTM_NEWADDR announcements which don't change the IP
> address. This code will only set |changed = true| if the address is completely
> new or if the ifaddrmsg content has changed. I don't think the router
> announcements mentioned in http://crbug.com/100690 change ifaddrmsg.

It's true that 100690 implies that only the time stamp is changing, but with the
privacy extension, you do get new addresses periodically and then some time
later the oldest one gets removed.  So instead of resetting the network state
every few seconds it might be every few hours.  A complete solution should
address this.

From my understanding the bottom 64bits of a IPv6 address will change for
privacy addresses, so a new address that only varies in the bottom 64bits from
an existing address shouldn't trigger a network change event.  Similarly for a
removed address.

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:66: : callback_(callback), netlink_fd_(-1) {
nit: style guide says that if they don't all fit on the first line, then they
each go on their own line.

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:162: bool
AddressTrackerLinux::HandleMessage(char* buf, size_t len) {
You should be able to write a unit test for this method.

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.h:33: AddressTrackerLinux(const base::Closure&
callback);
explicit

[szym, 2012-06-29 21:09:06]

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:165: switch (header->nlmsg_type) {
On 2012/06/29 21:01:18, vandebo wrote:
> It's true that 100690 implies that only the time stamp is changing, but with
the
> privacy extension, you do get new addresses periodically and then some time
> later the oldest one gets removed.  So instead of resetting the network state
> every few seconds it might be every few hours.  A complete solution should
> address this.

We must not simply ignore such a change. In particular, RFC3484 requires an
up-to-date information on source addresses and will need to receive an update
even if a change is due to privacy extension. I imagine there will be other
places (jingle?) which will want to know when the preferred IP address changes.
 
One solution is to add a |detail| field to OnIPAddressChanged (see OnDNSChanged)
which will be used to distinguish network changes from address changes.

[vandebo (ex-Chrome), 2012-06-29 21:20:46]

On 2012/06/29 21:09:06, szym wrote:
>
http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
> File net/base/address_tracker_linux.cc (right):
> 
>
http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_l...
> net/base/address_tracker_linux.cc:165: switch (header->nlmsg_type) {
> On 2012/06/29 21:01:18, vandebo wrote:
> > It's true that 100690 implies that only the time stamp is changing, but with
> the
> > privacy extension, you do get new addresses periodically and then some time
> > later the oldest one gets removed.  So instead of resetting the network
state
> > every few seconds it might be every few hours.  A complete solution should
> > address this.
> 
> We must not simply ignore such a change. In particular, RFC3484 requires an
> up-to-date information on source addresses and will need to receive an update
> even if a change is due to privacy extension. I imagine there will be other
> places (jingle?) which will want to know when the preferred IP address
changes.
>  
> One solution is to add a |detail| field to OnIPAddressChanged (see
OnDNSChanged)
> which will be used to distinguish network changes from address changes.

So it looks like we have both NetworkChangeNotifier and IPAddressObserver.  This
code is backend for those interfaces, so we can make do what ever is necessary
to communicate both pieces of state up to those interfaces. But a new privacy
address should not trigger a network change event.

BTW, I meant to say earlier that I'm no longer a net OWNER, so you'll need to
get sign off from soomeone else too, but someone else may be will to rubber
stamp it based on my review.

[willchan no longer on Chromium, 2012-06-29 21:31:50]

vandebo, szym is already an OWNER and I am very comfortable with your
approval here. so if you two agree, go ahead and ship it.


On Fri, Jun 29, 2012 at 2:20 PM, <vandebo@chromium.org> wrote:

> On 2012/06/29 21:09:06, szym wrote:
>
> http://codereview.chromium.**org/10689015/diff/15002/net/**
>
base/address_tracker_linux.cc<http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_linux.cc>
>
>> File net/base/address_tracker_**linux.cc (right):
>>
>
>
> http://codereview.chromium.**org/10689015/diff/15002/net/**
>
base/address_tracker_linux.cc#**newcode165<http://codereview.chromium.org/10689015/diff/15002/net/base/address_tracker_linux.cc#newcode165>
>
>> net/base/address_tracker_**linux.cc:165: switch (header->nlmsg_type) {
>> On 2012/06/29 21:01:18, vandebo wrote:
>> > It's true that 100690 implies that only the time stamp is changing, but
>> with
>> the
>> > privacy extension, you do get new addresses periodically and then some
>> time
>> > later the oldest one gets removed.  So instead of resetting the network
>>
> state
>
>> > every few seconds it might be every few hours.  A complete solution
>> should
>> > address this.
>>
>
>  We must not simply ignore such a change. In particular, RFC3484 requires
>> an
>> up-to-date information on source addresses and will need to receive an
>> update
>> even if a change is due to privacy extension. I imagine there will be
>> other
>> places (jingle?) which will want to know when the preferred IP address
>>
> changes.
>
>  One solution is to add a |detail| field to OnIPAddressChanged (see
>>
> OnDNSChanged)
>
>> which will be used to distinguish network changes from address changes.
>>
>
> So it looks like we have both NetworkChangeNotifier and IPAddressObserver.
>  This
> code is backend for those interfaces, so we can make do what ever is
> necessary
> to communicate both pieces of state up to those interfaces. But a new
> privacy
> address should not trigger a network change event.
>
> BTW, I meant to say earlier that I'm no longer a net OWNER, so you'll need
> to
> get sign off from soomeone else too, but someone else may be will to rubber
> stamp it based on my review.
>
>
http://codereview.chromium.**org/10689015/<http://codereview.chromium.org/106...
>

[szym, 2012-06-29 21:49:31]

On 2012/06/29 21:20:46, vandebo wrote:
> So it looks like we have both NetworkChangeNotifier and IPAddressObserver. 
This
> code is backend for those interfaces, so we can make do what ever is necessary
> to communicate both pieces of state up to those interfaces. But a new privacy
> address should not trigger a network change event.

I'm not entirely clear on what you mean. AddressTrackerLinux feeds into just one
interface: NetworkChangeNotifier::IPAddressObserver. This interface is currently
abused because historically it was the only interface provided by
NetworkChangeNotifier: http://crbug.com/36493
For privacy extensions, I suspect that only remoting::SingallingConnector has
the proper response to OnIPAddressChanged.

I think this is a more general problem with how NetworkChangeNotifier is used,
and I'd like to address it in separate CLs. After this CL, NCN will send fewer
OnIPAddressChanged signals than the status quo.

[vandebo (ex-Chrome), 2012-06-29 22:00:50]

On 2012/06/29 21:49:31, szym wrote:
> On 2012/06/29 21:20:46, vandebo wrote:
> > So it looks like we have both NetworkChangeNotifier and IPAddressObserver. 
> This
> > code is backend for those interfaces, so we can make do what ever is
necessary
> > to communicate both pieces of state up to those interfaces. But a new
privacy
> > address should not trigger a network change event.
> 
> I'm not entirely clear on what you mean. AddressTrackerLinux feeds into just
one
> interface: NetworkChangeNotifier::IPAddressObserver. This interface is
currently
> abused because historically it was the only interface provided by
> NetworkChangeNotifier: http://crbug.com/36493

Sorry, mis-skimmed. I think we're agreed that there should also be a
NetworkChangedObserver.  Do you think bug 36493 captures that sufficiently?  If
not, please file a bug for it?

> For privacy extensions, I suspect that only remoting::SingallingConnector has
> the proper response to OnIPAddressChanged.
> 
> I think this is a more general problem with how NetworkChangeNotifier is used,
> and I'd like to address it in separate CLs. After this CL, NCN will send fewer
> OnIPAddressChanged signals than the status quo.

[vandebo (ex-Chrome), 2012-06-29 22:24:29]

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:66: : callback_(callback), netlink_fd_(-1) {
On 2012/06/29 21:01:18, vandebo wrote:
> nit: style guide says that if they don't all fit on the first line, then they
> each go on their own line.

You seem to have missed this comment.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:41: local = reinterpret_cast<unsigned
char*>(RTA_DATA(attr));
you assign local in IFA_ADDRESS and address in IFA_LOCAL.  If this isn't a bug,
it's very confusing.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:50: if (local)
A comment about what's going on here would be helpful.  It looks like
http://stackoverflow.com/questions/4678637/what-is-difference-between-ifa-loc...
?

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:185: } else if (memcmp(&it->second, msg,
sizeof(*msg))) {
Why do we require that the struct ifaddrmsg be identical?  A comment would be
useful.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:30: typedef std::map<IPAddressNumber, struct
ifaddrmsg> AddressMap;
It's not clear to me why the consumer wants the entire struct ifaddrmsg and not
just the address?

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:37: // have a MessageLoopForIO. The signals
will be delivered directly to
nit: comment out of date? Signals will be delivered to the callback, right?

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:41: AddressMap GetMap() const;
It seems that this method should be NetworkChangeNotifier::GetIPAddresses(),
mirroring GetConnectionType and ConnectionTypeObserver.

[szym, 2012-06-30 00:00:57]

I am away from my workstation, so I will upload the updated CL shortly, but
please take a look at this comment I sent earlier to willchan, it establishes
the context for this CL:

AddressTrackerLinux will live on the NetworkChangeNotifierLinux::Thread. But
GetMap() will be called from the network thread, by AddressSorter (in response
to OnIPAddressChanged). See:
http://codereview.chromium.org/10442098/

I plan to add NetworkChangeNotifier::GetAddressTracker() (#if
defined(OS_LINUX)).

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:41: local = reinterpret_cast<unsigned
char*>(RTA_DATA(attr));
On 2012/06/29 22:24:29, vandebo wrote:
> you assign local in IFA_ADDRESS and address in IFA_LOCAL.  If this isn't a
bug,
> it's very confusing.

Definitely a typo.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:50: if (local)
On 2012/06/29 22:24:29, vandebo wrote:
> A comment about what's going on here would be helpful.  It looks like
>
http://stackoverflow.com/questions/4678637/what-is-difference-between-ifa-loc...
> ?

The behavior here is based on getaddrinfo in glibc (check_pf.c).

Looking at NETLINK implementation in the kernel, the problem is that IPv4
addresses have only the IFA_ADDRESS attribute, while IPv6 have the IFA_LOCAL
attribute.

I'll add a comment.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:185: } else if (memcmp(&it->second, msg,
sizeof(*msg))) {
On 2012/06/29 22:24:29, vandebo wrote:
> Why do we require that the struct ifaddrmsg be identical?  A comment would be
> useful.

We require that something in ifaddrmsg changes before we signal
OnIPAddressChanged. The reason for this behavior is that the future consumer of
GetAddressMap (old GetMap), AddressSorter (RFC3484), will need to re-fetch the
AddressMap when ifaddrmsg changes. Currently, it only needs 2 of the ifaddrmsg
members, but I think simplicity is preferred here.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:30: typedef std::map<IPAddressNumber, struct
ifaddrmsg> AddressMap;
On 2012/06/29 22:24:29, vandebo wrote:
> It's not clear to me why the consumer wants the entire struct ifaddrmsg and
not
> just the address?

See: http://codereview.chromium.org/10442098/

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:37: // have a MessageLoopForIO. The signals
will be delivered directly to
On 2012/06/29 22:24:29, vandebo wrote:
> nit: comment out of date? Signals will be delivered to the callback, right?

Right.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:41: AddressMap GetMap() const;
On 2012/06/29 22:24:29, vandebo wrote:
> It seems that this method should be NetworkChangeNotifier::GetIPAddresses(),
> mirroring GetConnectionType and ConnectionTypeObserver.

Will change to GetAddressMap.

[vandebo (ex-Chrome), 2012-06-30 19:44:03]

On 2012/06/30 00:00:57, szym wrote:
> I am away from my workstation, so I will upload the updated CL shortly, but
> please take a look at this comment I sent earlier to willchan, it establishes
> the context for this CL:
> 
> AddressTrackerLinux will live on the NetworkChangeNotifierLinux::Thread. But
> GetMap() will be called from the network thread, by AddressSorter (in response
> to OnIPAddressChanged). See:
> http://codereview.chromium.org/10442098/
> 
> I plan to add NetworkChangeNotifier::GetAddressTracker() (#if
> defined(OS_LINUX)).

I think I see how things will fit together and why you want to return the
address tracker instead of returning the map. But if you add a typedef, I think
it can work out ok.
i.e.

class AddressTracker {
typedef struct ifaddrmsg IPEntry;
};

std::vector<AddressTracker::IPEntry> NetworkChangeNotifier::GetIPAddresses();

This isn't much different from what you were planning, but it follows the
pattern of the other things in NetworkChangeNotifier as well as effectively
abstracting away AddressTracker.

I've taken vacation days for Mon&Tue, but I'm just at home. A gtalk message will
notify on my phone if you want to let me know when you've posted your next CL.

[szym, 2012-07-11 22:42:34]

On 2012/06/30 19:44:03, vandebo wrote:
> I think I see how things will fit together and why you want to return the
> address tracker instead of returning the map. But if you add a typedef, I
think
> it can work out ok.
> i.e.
> 
> class AddressTracker {
> typedef struct ifaddrmsg IPEntry;
> };
> 
> std::vector<AddressTracker::IPEntry> NetworkChangeNotifier::GetIPAddresses();

I don't quite understand this snippet, but the only way I found to expose the
map type without having to include <linux/rtnetlink.h> is to forward declare
|struct ifaddrmsg|. It feels like gory details, but I have no strong opinion
about it. If that's preferable, I'll change the forward declaration of |class
AddressTrackerLinux| (#if defined(OS_LINUX)) to |struct ifaddrmsg| in
network_change_notifier.h.
 
I have also added unit tests. PTAL.

[vandebo (ex-Chrome), 2012-07-12 18:34:26]

On 2012/07/11 22:42:34, szym wrote:
> On 2012/06/30 19:44:03, vandebo wrote:
> > I think I see how things will fit together and why you want to return the
> > address tracker instead of returning the map. But if you add a typedef, I
> think
> > it can work out ok.
> > i.e.
> > 
> > class AddressTracker {
> > typedef struct ifaddrmsg IPEntry;
> > };
> > 
> > std::vector<AddressTracker::IPEntry>
NetworkChangeNotifier::GetIPAddresses();
> 
> I don't quite understand this snippet, but the only way I found to expose the
> map type without having to include <linux/rtnetlink.h> is to forward declare
> |struct ifaddrmsg|. It feels like gory details, but I have no strong opinion
> about it. If that's preferable, I'll change the forward declaration of |class
> AddressTrackerLinux| (#if defined(OS_LINUX)) to |struct ifaddrmsg| in
> network_change_notifier.h.
>  
> I have also added unit tests. PTAL.

What I was suggesting is to define a type AddressTracker::IPEntry that differs
across platforms.  On Linux it would be struct ifaddrmsg.  NetworkChangeNotifier
could then return a vector of AddressTracker::IPEntry across all platforms and
not require any OS #defines.

The file that defines AddressTracker::IPEntry would have to include the
appropriate header for struct ifaddrmsg (which is linux/if_addr.h).

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:66: : callback_(callback), netlink_fd_(-1) {
On 2012/06/29 22:24:29, vandebo wrote:
> On 2012/06/29 21:01:18, vandebo wrote:
> > nit: style guide says that if they don't all fit on the first line, then
they
> > each go on their own line.
> 
> You seem to have missed this comment.

Please fix this code to adhere to the style guide.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:37: // have a MessageLoopForIO. The signals
will be delivered directly to
On 2012/06/30 00:00:57, szym wrote:
> On 2012/06/29 22:24:29, vandebo wrote:
> > nit: comment out of date? Signals will be delivered to the callback, right?
> 
> Right.

Please update the comment.

http://codereview.chromium.org/10689015/diff/31003/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/31003/net/base/address_tracker_l...
net/base/address_tracker_linux.h:43: friend class AddressTrackerLinuxTest;
HandleMessage and the friend declaration should be in the private section.

[szym, 2012-07-12 18:55:47]

> What I was suggesting is to define a type AddressTracker::IPEntry that differs
> across platforms.  On Linux it would be struct ifaddrmsg. 
NetworkChangeNotifier
> could then return a vector of AddressTracker::IPEntry across all platforms and
> not require any OS #defines.
> 
> The file that defines AddressTracker::IPEntry would have to include the
> appropriate header for struct ifaddrmsg (which is linux/if_addr.h).

I have no plans for AddressTracker on other platforms. On BSD (including
Darwin/OSX), AddressSorter will obtain the data relevant to RFC3484 via
getifaddrs in response to OnIPAddressChanged. Does it still make sense to define
AddressTracker for all platforms?

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:66: : callback_(callback), netlink_fd_(-1) {
On 2012/07/12 18:34:26, vandebo wrote:
> On 2012/06/29 22:24:29, vandebo wrote:
> > On 2012/06/29 21:01:18, vandebo wrote:
> > > nit: style guide says that if they don't all fit on the first line, then
> they
> > > each go on their own line.
> > 
> > You seem to have missed this comment.
> 
> Please fix this code to adhere to the style guide.

Done.

http://codereview.chromium.org/10689015/diff/16004/net/base/address_tracker_l...
net/base/address_tracker_linux.cc:162: bool
AddressTrackerLinux::HandleMessage(char* buf, size_t len) {
On 2012/06/29 21:01:18, vandebo wrote:
> You should be able to write a unit test for this method.

Done.

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/15006/net/base/address_tracker_l...
net/base/address_tracker_linux.h:37: // have a MessageLoopForIO. The signals
will be delivered directly to
On 2012/07/12 18:34:26, vandebo wrote:
> On 2012/06/30 00:00:57, szym wrote:
> > On 2012/06/29 22:24:29, vandebo wrote:
> > > nit: comment out of date? Signals will be delivered to the callback,
right?
> > 
> > Right.
> 
> Please update the comment.

Done.

[vandebo (ex-Chrome), 2012-07-12 19:05:09]

> I have no plans for AddressTracker on other platforms. On BSD (including
> Darwin/OSX), AddressSorter will obtain the data relevant to RFC3484 via
> getifaddrs in response to OnIPAddressChanged. Does it still make sense to
define
> AddressTracker for all platforms?

It sucks when the same things are implemented in different places on different
platforms.  Is the RFC3484 information on other platforms similar to struct
ifaddrmsg?  If so, could you provide static implementations on the other
platforms so that a uniform interface i.e.
NetworkChangeNotifier::GetIPAddresses() or
NetworkChangeNotifier::GetIPAddressInfo() is available on all platforms?

In other words, when there are platform differences, we should first provide an
abstraction on top of that, and then consume the abstraction so that we can
share as much code across platforms as possible.

http://codereview.chromium.org/10689015/diff/27012/net/base/address_tracker_l...
File net/base/address_tracker_linux.h (right):

http://codereview.chromium.org/10689015/diff/27012/net/base/address_tracker_l...
net/base/address_tracker_linux.h:45: bool HandleMessage(const char* buffer,
size_t length);
Declaration order and implantation order should match. i.e. you should move
HandleMessage below ReadMessages again.

[szym, 2012-07-13 22:37:21]

On 2012/07/12 19:05:09, vandebo wrote:
> It sucks when the same things are implemented in different places on different
> platforms.  Is the RFC3484 information on other platforms similar to struct
> ifaddrmsg?  If so, could you provide static implementations on the other
> platforms so that a uniform interface i.e.
> NetworkChangeNotifier::GetIPAddresses() or
> NetworkChangeNotifier::GetIPAddressInfo() is available on all platforms?

Regarding RFC3484:
There are four pieces of information needed for RFC3484: prefix length, is
deprecated, is home, is native. The ifaddrmsg struct provides: prefix length, is
deprecated, and is home. On BSD systems, ioctl(SIOCGIFAFLAG_IN6) provides "is
deprecated", and prefix len in ifaddrs.ifa_netmask (obtained via getifaddrs).

> In other words, when there are platform differences, we should first provide
an
> abstraction on top of that, and then consume the abstraction so that we can
> share as much code across platforms as possible.

The key issue here is that unlike on other platforms, on Linux the same API
which is useful for RFC3484 is also _the_ API for detecting changes to interface
addresses. So the way to keep things clean would be to keep AddressTrackerLinux
internal to NetworkChangeNotifierLinux. The planned AddressSorterPosix would
then spawn its own AddressTrackerLinux equivalent. (Perhaps even using the same
implementation.) The work would be redundant, but the code would be healthier.
Opinions?

[szym, 2012-07-13 22:38:55]

I forgot to mention that we have no need for AddressMap on Windows, where
RFC3484 is provided by system ioctl.

[vandebo (ex-Chrome), 2012-07-16 18:06:27]

On 2012/07/13 22:37:21, szym wrote:
> The key issue here is that unlike on other platforms, on Linux the same API
> which is useful for RFC3484 is also _the_ API for detecting changes to
interface
> addresses. So the way to keep things clean would be to keep
AddressTrackerLinux
> internal to NetworkChangeNotifierLinux. The planned AddressSorterPosix would
> then spawn its own AddressTrackerLinux equivalent. (Perhaps even using the
same
> implementation.) The work would be redundant, but the code would be healthier.
> Opinions?

It's silly to do redundant work to provide a uniform API that would only be used
on one platform anyway...

LGTM

[szym, 2012-07-16 18:46:56]

Agreed. That said, are you opposed to having two instances of
AddressTrackerLinux so that it doesn't need to be exposed from
NetworkChangeNotifier? (and it wouldn't need locking in that case)


On Mon, Jul 16, 2012 at 2:06 PM, <vandebo@chromium.org> wrote:

> On 2012/07/13 22:37:21, szym wrote:
>
>> The key issue here is that unlike on other platforms, on Linux the same
>> API
>> which is useful for RFC3484 is also _the_ API for detecting changes to
>>
> interface
>
>> addresses. So the way to keep things clean would be to keep
>>
> AddressTrackerLinux
>
>> internal to NetworkChangeNotifierLinux. The planned AddressSorterPosix
>> would
>> then spawn its own AddressTrackerLinux equivalent. (Perhaps even using the
>>
> same
>
>> implementation.) The work would be redundant, but the code would be
>> healthier.
>> Opinions?
>>
>
> It's silly to do redundant work to provide a uniform API that would only
> be used
> on one platform anyway...
>
> LGTM
>
>
http://codereview.chromium.**org/10689015/<http://codereview.chromium.org/106...
>

[vandebo (ex-Chrome), 2012-07-16 18:50:01]

On 2012/07/16 18:46:56, szym wrote:
> Agreed. That said, are you opposed to having two instances of
> AddressTrackerLinux so that it doesn't need to be exposed from
> NetworkChangeNotifier? (and it wouldn't need locking in that case)

Yes. If you want to consume the information in two places, then it should be
setup as a service instead of running two copies of the code. 
NetworkChangeNotifier seems like a reasonable place to obtain the service.

[commit-bot: I haz the power, 2012-07-16 20:41:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10689015/39002

[commit-bot: I haz the power, 2012-07-16 21:06:29]

Try job failure for 10689015-39002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-16 21:08:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10689015/42003

[commit-bot: I haz the power, 2012-07-16 22:27:25]

Change committed as 146907