From bbaa33e1d31c66ea1203a226523caf174bb701e6 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 19 Nov 2019 19:03:53 +0900 Subject: [PATCH] [NS A14] Move code notifying battery stats in its right place This should be done once every time an interface comes online. Doing this in updateLinkProperties guarantees this happens every time a new interface comes online, but it doesn't do it more often than needed. Test: FrameworksNetTests NetworkStackTests Change-Id: I0613c23f44192944266d76107308da8d1c541d1c --- .../android/server/ConnectivityService.java | 48 +++++++------------ .../server/ConnectivityServiceTest.java | 25 +++------- 2 files changed, 24 insertions(+), 49 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2bbcf0dfa851d..05ce2aef86c6b 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5651,7 +5651,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // are accurate. networkAgent.clatd.fixupLinkProperties(oldLp, newLp); - updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities); + updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities, + networkAgent.networkInfo.getType()); // update filtering rules, need to happen after the interface update so netd knows about the // new interface (the interface name -> index map becomes initialized) @@ -5730,21 +5731,26 @@ public class ConnectivityService extends IConnectivityManager.Stub } - private void updateInterfaces(LinkProperties newLp, LinkProperties oldLp, int netId, - NetworkCapabilities caps) { - CompareResult interfaceDiff = new CompareResult<>( + private void updateInterfaces(final @Nullable LinkProperties newLp, + final @Nullable LinkProperties oldLp, final int netId, + final @Nullable NetworkCapabilities caps, final int legacyType) { + final CompareResult interfaceDiff = new CompareResult<>( oldLp != null ? oldLp.getAllInterfaceNames() : null, newLp != null ? newLp.getAllInterfaceNames() : null); - for (String iface : interfaceDiff.added) { - try { - if (DBG) log("Adding iface " + iface + " to network " + netId); - mNMS.addInterfaceToNetwork(iface, netId); - wakeupModifyInterface(iface, caps, true); - } catch (Exception e) { - loge("Exception adding interface: " + e); + if (!interfaceDiff.added.isEmpty()) { + final IBatteryStats bs = mDeps.getBatteryStatsService(); + for (final String iface : interfaceDiff.added) { + try { + if (DBG) log("Adding iface " + iface + " to network " + netId); + mNMS.addInterfaceToNetwork(iface, netId); + wakeupModifyInterface(iface, caps, true); + bs.noteNetworkInterfaceType(iface, legacyType); + } catch (Exception e) { + loge("Exception adding interface: " + e); + } } } - for (String iface : interfaceDiff.removed) { + for (final String iface : interfaceDiff.removed) { try { if (DBG) log("Removing iface " + iface + " from network " + netId); wakeupModifyInterface(iface, caps, false); @@ -6503,24 +6509,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mLegacyTypeTracker.add(newNetwork.networkInfo.getType(), newNetwork); notifyLockdownVpn(newNetwork); } - - if (reassignedRequests.containsValue(newNetwork) || newNetwork.isVPN()) { - // Notify battery stats service about this network, both the normal - // interface and any stacked links. - // TODO: Avoid redoing this; this must only be done once when a network comes online. - try { - final IBatteryStats bs = mDeps.getBatteryStatsService(); - final int type = newNetwork.networkInfo.getType(); - - final String baseIface = newNetwork.linkProperties.getInterfaceName(); - bs.noteNetworkInterfaceType(baseIface, type); - for (LinkProperties stacked : newNetwork.linkProperties.getStackedLinks()) { - final String stackedIface = stacked.getInterfaceName(); - bs.noteNetworkInterfaceType(stackedIface, type); - } - } catch (RemoteException ignored) { - } - } } /** diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index da83397b49adb..9b0ccdd4451d3 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5664,14 +5664,6 @@ public class ConnectivityServiceTest { TYPE_WIFI); reset(mBatteryStatsService); - // TODO : In the current code, ConnectivityService only tells BatteryStatsService about - // the type of networks that satisfy a request. That is a bug in a sense, but it has no - // consequences because a network that never satisfies any request gets torn down right - // away. Because of this, in the context of this test, the cell network agent does not - // satisfy any request as long as WiFi is connected, so the test below would fail if - // the WiFi network agent is not disconnected first. When this bug is fixed, remove the - // WiFi disconnect for more precise testing. - mWiFiNetworkAgent.disconnect(); mCellNetworkAgent.disconnect(); cellLp.setInterfaceName("wifi0"); @@ -5722,13 +5714,12 @@ public class ConnectivityServiceTest { mCm.registerNetworkCallback(networkRequest, networkCallback); // Prepare ipv6 only link properties. - mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR); - final int cellNetId = mCellNetworkAgent.getNetwork().netId; final LinkProperties cellLp = new LinkProperties(); cellLp.setInterfaceName(MOBILE_IFNAME); cellLp.addLinkAddress(myIpv6); cellLp.addRoute(new RouteInfo((IpPrefix) null, myIpv6.getAddress(), MOBILE_IFNAME)); cellLp.addRoute(new RouteInfo(myIpv6, null, MOBILE_IFNAME)); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); reset(mNetworkManagementService); reset(mMockDnsResolver); reset(mMockNetd); @@ -5737,8 +5728,8 @@ public class ConnectivityServiceTest { .thenReturn(getClatInterfaceConfig(myIpv4)); // Connect with ipv6 link properties. Expect prefix discovery to be started. - mCellNetworkAgent.sendLinkProperties(cellLp); mCellNetworkAgent.connect(true); + final int cellNetId = mCellNetworkAgent.getNetwork().netId; waitForIdle(); verify(mMockNetd, times(1)).networkCreatePhysical(eq(cellNetId), anyInt()); @@ -5811,14 +5802,10 @@ public class ConnectivityServiceTest { assertEquals(1, resolvrParams.servers.length); assertTrue(ArrayUtils.contains(resolvrParams.servers, "8.8.8.8")); - // TODO : this should be invoked but in the current code there is no path to invoke - // it. In practice, it will be invoked next time this network changes what requests it - // satisfies through rematchNetworkAndRequests, which may in fact be too late. This code - // should be reinstated when the bug is fixed. -// for (final LinkProperties stackedLp : stackedLpsAfterChange) { -// verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), -// TYPE_MOBILE); -// } + for (final LinkProperties stackedLp : stackedLpsAfterChange) { + verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), + TYPE_MOBILE); + } // Add ipv4 address, expect that clatd and prefix discovery are stopped and stacked // linkproperties are cleaned up.