[net (linux)] Don't send OnIPAddressChanged when the AddressMap is first populated.

[net (linux)] Don't send OnIPAddressChanged when the AddressMap is first populated.

The only observer that cares about the initial signal is AddressSorterPosix.

This CL:
- Adds ReadMessages to AddressTrackerLinux::Init so that the initial enumeration of
  interfaces does not trigger the notification.
- Moves AddressTrackerLinux::Init before DnsConfigService::WatchConfig so that
  HostResolverImpl won't create a DnsClient until AddressTrackerLinux assembles the
  AddressMap.

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

[mmenke, 2012-09-04 19:40:15]

http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
net/base/address_tracker_linux.cc:146: ReadMessages();
Since this is a non-blocking socket, is there a chance that netlink hasn't
answered our first query yet?

[szym, 2012-09-04 20:10:59]

http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
File net/base/address_tracker_linux.cc (right):

http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
net/base/address_tracker_linux.cc:146: ReadMessages();
On 2012/09/04 19:40:16, Matt Menke wrote:
> Since this is a non-blocking socket, is there a chance that netlink hasn't
> answered our first query yet?

I just tested this and it seems to be working as intended.

I looked through the kernel source code. The entry point is rtnetlink_rcv_msg.
The handler for (PF_UNSPEC, RTM_GETADDR, NLM_F_DUMP) is rtnl_dump_all which
calls through all registered RTM_GETADDR dump handlers. For example:
inet_dump_ifaddr (inet)
int_dump_addr (inet6)
getaddr_dumpit (phonet)
dn_nl_dump_ifaddr (decnet)

So the kernel will create the sk_buff for the whole dump before calling through
netlink_dump_start. netlink_dump cannot fail with anything other than EINVAL,
ENOBUFS, ECONNREFUSED, or EBUSY. It will return EBUSY iff we send another
request while the previous dump is in progress.

So my conclusion is that it will not block until the initial dump is complete.

[mmenke, 2012-09-04 20:29:55]

On 2012/09/04 20:10:59, szym wrote:
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
> File net/base/address_tracker_linux.cc (right):
> 
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
> net/base/address_tracker_linux.cc:146: ReadMessages();
> On 2012/09/04 19:40:16, Matt Menke wrote:
> > Since this is a non-blocking socket, is there a chance that netlink hasn't
> > answered our first query yet?
> 
> I just tested this and it seems to be working as intended.
> 
> I looked through the kernel source code. The entry point is rtnetlink_rcv_msg.
> The handler for (PF_UNSPEC, RTM_GETADDR, NLM_F_DUMP) is rtnl_dump_all which
> calls through all registered RTM_GETADDR dump handlers. For example:
> inet_dump_ifaddr (inet)
> int_dump_addr (inet6)
> getaddr_dumpit (phonet)
> dn_nl_dump_ifaddr (decnet)
> 
> So the kernel will create the sk_buff for the whole dump before calling
through
> netlink_dump_start. netlink_dump cannot fail with anything other than EINVAL,
> ENOBUFS, ECONNREFUSED, or EBUSY. It will return EBUSY iff we send another
> request while the previous dump is in progress.
> 
> So my conclusion is that it will not block until the initial dump is complete.

What matters is when rtnetlink_rcv_msg is called.  If it's called in a separate
process when we send a message to netlink, and then sends data to a real socket,
seems like there's a potential problem.

If, instead, requests are queued in the consumer process, and created / returned
synchronously on calls to recv, then there's no problem.  Looking at the netlink
code, I can only conclude that anyone who uses  function name of
"netlink_rcv_skb" should be deemed insane, and a potential risk to himself and
others.  Or at least a risk to hapless future generations who try to read
through his code.

[mmenke, 2012-09-04 20:31:04]

On 2012/09/04 20:29:55, Matt Menke wrote:
> On 2012/09/04 20:10:59, szym wrote:
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
> > File net/base/address_tracker_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
> > net/base/address_tracker_linux.cc:146: ReadMessages();
> > On 2012/09/04 19:40:16, Matt Menke wrote:
> > > Since this is a non-blocking socket, is there a chance that netlink hasn't
> > > answered our first query yet?
> > 
> > I just tested this and it seems to be working as intended.
> > 
> > I looked through the kernel source code. The entry point is
rtnetlink_rcv_msg.
> > The handler for (PF_UNSPEC, RTM_GETADDR, NLM_F_DUMP) is rtnl_dump_all which
> > calls through all registered RTM_GETADDR dump handlers. For example:
> > inet_dump_ifaddr (inet)
> > int_dump_addr (inet6)
> > getaddr_dumpit (phonet)
> > dn_nl_dump_ifaddr (decnet)
> > 
> > So the kernel will create the sk_buff for the whole dump before calling
> through
> > netlink_dump_start. netlink_dump cannot fail with anything other than
EINVAL,
> > ENOBUFS, ECONNREFUSED, or EBUSY. It will return EBUSY iff we send another
> > request while the previous dump is in progress.
> > 
> > So my conclusion is that it will not block until the initial dump is
complete.
> 
> What matters is when rtnetlink_rcv_msg is called.  If it's called in a
separate
> process when we send a message to netlink, and then sends data to a real
socket,
> seems like there's a potential problem.
> 
> If, instead, requests are queued in the consumer process, and created /
returned
> synchronously on calls to recv, then there's no problem.  Looking at the
netlink
> code, I can only conclude that anyone who uses  function name of
> "netlink_rcv_skb" should be deemed insane, and a potential risk to himself and
> others.  Or at least a risk to hapless future generations who try to read
> through his code.

Oh, and the code LGTM.  I just can't be confident it never sends an
OnIPAddressChanged event on initialization.

[szym, 2012-09-04 22:10:59]

On 2012/09/04 20:29:55, Matt Menke wrote:
> On 2012/09/04 20:10:59, szym wrote:
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
> > File net/base/address_tracker_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
> > net/base/address_tracker_linux.cc:146: ReadMessages();
> > On 2012/09/04 19:40:16, Matt Menke wrote:
> > > Since this is a non-blocking socket, is there a chance that netlink hasn't
> > > answered our first query yet?
> > 
> > I just tested this and it seems to be working as intended.
> > 
> > I looked through the kernel source code. The entry point is
rtnetlink_rcv_msg.
> > The handler for (PF_UNSPEC, RTM_GETADDR, NLM_F_DUMP) is rtnl_dump_all which
> > calls through all registered RTM_GETADDR dump handlers. For example:
> > inet_dump_ifaddr (inet)
> > int_dump_addr (inet6)
> > getaddr_dumpit (phonet)
> > dn_nl_dump_ifaddr (decnet)
> > 
> > So the kernel will create the sk_buff for the whole dump before calling
> through
> > netlink_dump_start. netlink_dump cannot fail with anything other than
EINVAL,
> > ENOBUFS, ECONNREFUSED, or EBUSY. It will return EBUSY iff we send another
> > request while the previous dump is in progress.
> > 
> > So my conclusion is that it will not block until the initial dump is
complete.
> 
> What matters is when rtnetlink_rcv_msg is called.  If it's called in a
separate
> process when we send a message to netlink, and then sends data to a real
socket,
> seems like there's a potential problem.

AddressTrackerLinux sends the packet to the kernel (pid == 0). So the call graph
is:

-> netlink_sendmsg 
-> netlink_unicast 
-> netlink_unicast_kernel 
-> netlink_rcv_skb 
-> netlink_dump_start 
-> netlink_dump 
-> rtnl_dump_all

This will send only one family per message. However, the subsequent calls to
netlink_recvmsg go directly into netlink_dump because at this point nlk->cb !=
NULL. This is reset to NULL only after there is no more data produced by the
dump callback.

[commit-bot: I haz the power, 2012-09-04 22:11:54]

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

[mmenke, 2012-09-04 22:26:53]

On 2012/09/04 22:10:59, szym wrote:
> On 2012/09/04 20:29:55, Matt Menke wrote:
> > On 2012/09/04 20:10:59, szym wrote:
> > >
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux.cc
> > > File net/base/address_tracker_linux.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10910076/diff/1/net/base/address_tracker_linux...
> > > net/base/address_tracker_linux.cc:146: ReadMessages();
> > > On 2012/09/04 19:40:16, Matt Menke wrote:
> > > > Since this is a non-blocking socket, is there a chance that netlink
hasn't
> > > > answered our first query yet?
> > > 
> > > I just tested this and it seems to be working as intended.
> > > 
> > > I looked through the kernel source code. The entry point is
> rtnetlink_rcv_msg.
> > > The handler for (PF_UNSPEC, RTM_GETADDR, NLM_F_DUMP) is rtnl_dump_all
which
> > > calls through all registered RTM_GETADDR dump handlers. For example:
> > > inet_dump_ifaddr (inet)
> > > int_dump_addr (inet6)
> > > getaddr_dumpit (phonet)
> > > dn_nl_dump_ifaddr (decnet)
> > > 
> > > So the kernel will create the sk_buff for the whole dump before calling
> > through
> > > netlink_dump_start. netlink_dump cannot fail with anything other than
> EINVAL,
> > > ENOBUFS, ECONNREFUSED, or EBUSY. It will return EBUSY iff we send another
> > > request while the previous dump is in progress.
> > > 
> > > So my conclusion is that it will not block until the initial dump is
> complete.
> > 
> > What matters is when rtnetlink_rcv_msg is called.  If it's called in a
> separate
> > process when we send a message to netlink, and then sends data to a real
> socket,
> > seems like there's a potential problem.
> 
> AddressTrackerLinux sends the packet to the kernel (pid == 0). So the call
graph
> is:
> 
> -> netlink_sendmsg 
> -> netlink_unicast 
> -> netlink_unicast_kernel 
> -> netlink_rcv_skb 
> -> netlink_dump_start 
> -> netlink_dump 
> -> rtnl_dump_all
> 
> This will send only one family per message. However, the subsequent calls to
> netlink_recvmsg go directly into netlink_dump because at this point nlk->cb !=
> NULL. This is reset to NULL only after there is no more data produced by the
> dump callback.

So netlink_rcv_skb is called when messages are sent to netlink, but
netlink_recvmsg is called when we try to read from a netlink connection?  And
they're called directly from the kernel, as opposed to listening to actual
sockets/socket-like things.  Ok, you've convinced me.

[commit-bot: I haz the power, 2012-09-05 00:03:43]

Try job failure for 10910076-1 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...