From 84f8c4842093e827edb22fbab969b41c6a339d10 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Tue, 26 Jan 2021 06:19:46 +0000 Subject: [PATCH] Revert "Revert "Note network interfaces based on transport"" Instead of classifying interfaces by network type in BatteryStats, classify them based on the transports array provided by the NetworkAgent. Network types are deprecated and transports should be used instead. This change allows BatteryStats to stop depending on unstable APIs such as isNetworkTypeMobile. This change also updates nullability annotations in ConnectivityService and NetworkAgentInfo to show that the NetworkCapabilities are non-null (as provided by the network agent) when calling noteNetworkInterfaceTransports. This is rolling forward a previously reverted change. The new change also fixes MobileRadioPowerCalculatorTest that was broken when submitting the previous change. Bug: 174436414 Test: atest atest ConnectivityServiceTest#testBatteryStatsNetworkType \ --rerun-until-failure 40 Merged-In: I4e928fac8a57a9b1fc758a44af2a5719b8c871b8 Change-Id: I4e928fac8a57a9b1fc758a44af2a5719b8c871b8 (cherry picked from commit 2d17d202bc709c205506c03149d847f00f92d78d) --- .../android/internal/app/IBatteryStats.aidl | 2 +- .../internal/os/BatteryStatsHelper.java | 9 +++---- .../android/internal/os/BatteryStatsImpl.java | 11 +++++--- .../os/MobileRadioPowerCalculatorTest.java | 5 ++-- .../android/server/ConnectivityService.java | 10 +++---- .../server/am/BatteryStatsService.java | 4 +-- .../server/connectivity/NetworkAgentInfo.java | 2 +- .../server/ConnectivityServiceTest.java | 26 ++++++++++--------- 8 files changed, 37 insertions(+), 32 deletions(-) diff --git a/core/java/com/android/internal/app/IBatteryStats.aidl b/core/java/com/android/internal/app/IBatteryStats.aidl index 7a30ee2376ac3..55f8c400022f5 100644 --- a/core/java/com/android/internal/app/IBatteryStats.aidl +++ b/core/java/com/android/internal/app/IBatteryStats.aidl @@ -134,7 +134,7 @@ interface IBatteryStats { void noteWifiBatchedScanStartedFromSource(in WorkSource ws, int csph); void noteWifiBatchedScanStoppedFromSource(in WorkSource ws); void noteWifiRadioPowerState(int powerState, long timestampNs, int uid); - void noteNetworkInterfaceType(String iface, int type); + void noteNetworkInterfaceForTransports(String iface, in int[] transportTypes); void noteNetworkStatsEnabled(); void noteDeviceIdleMode(int mode, String activeReason, int activeUid); void setBatteryState(int status, int health, int plugType, int level, int temp, int volt, diff --git a/core/java/com/android/internal/os/BatteryStatsHelper.java b/core/java/com/android/internal/os/BatteryStatsHelper.java index 4a24358ad5d5c..a21c68b4f01a9 100644 --- a/core/java/com/android/internal/os/BatteryStatsHelper.java +++ b/core/java/com/android/internal/os/BatteryStatsHelper.java @@ -23,7 +23,6 @@ import android.content.IntentFilter; import android.content.pm.PackageManager; import android.content.res.Resources; import android.hardware.SensorManager; -import android.net.ConnectivityManager; import android.os.BatteryStats; import android.os.BatteryStats.Uid; import android.os.Build; @@ -37,6 +36,7 @@ import android.os.SELinux; import android.os.ServiceManager; import android.os.SystemClock; import android.os.UserHandle; +import android.telephony.TelephonyManager; import android.text.format.DateUtils; import android.util.ArrayMap; import android.util.Log; @@ -120,12 +120,11 @@ public class BatteryStatsHelper { private double mMaxDrainedPower; public static boolean checkWifiOnly(Context context) { - ConnectivityManager cm = (ConnectivityManager) context.getSystemService( - Context.CONNECTIVITY_SERVICE); - if (cm == null) { + final TelephonyManager tm = context.getSystemService(TelephonyManager.class); + if (tm == null) { return false; } - return !cm.isNetworkSupported(ConnectivityManager.TYPE_MOBILE); + return !tm.isDataCapable(); } @UnsupportedAppUsage diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index 37b621d781d1c..d475c658a3a06 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -16,6 +16,8 @@ package com.android.internal.os; +import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; +import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.os.BatteryStatsManager.NUM_WIFI_STATES; import static android.os.BatteryStatsManager.NUM_WIFI_SUPPL_STATES; @@ -36,7 +38,6 @@ import android.content.IntentFilter; import android.database.ContentObserver; import android.hardware.usb.UsbManager; import android.location.GnssSignalQuality; -import android.net.ConnectivityManager; import android.net.INetworkStatsService; import android.net.NetworkStats; import android.net.Uri; @@ -111,6 +112,7 @@ import com.android.internal.util.ArrayUtils; import com.android.internal.util.FastPrintWriter; import com.android.internal.util.FrameworkStatsLog; import com.android.internal.util.XmlUtils; +import com.android.net.module.util.NetworkCapabilitiesUtils; import libcore.util.EmptyArray; @@ -6711,11 +6713,12 @@ public class BatteryStatsImpl extends BatteryStats { } /** @hide */ - public void noteNetworkInterfaceType(String iface, int networkType) { + public void noteNetworkInterfaceForTransports(String iface, int[] transportTypes) { if (TextUtils.isEmpty(iface)) return; + final int displayTransport = NetworkCapabilitiesUtils.getDisplayTransport(transportTypes); synchronized (mModemNetworkLock) { - if (ConnectivityManager.isNetworkTypeMobile(networkType)) { + if (displayTransport == TRANSPORT_CELLULAR) { mModemIfaces = includeInStringArray(mModemIfaces, iface); if (DEBUG) Slog.d(TAG, "Note mobile iface " + iface + ": " + mModemIfaces); } else { @@ -6725,7 +6728,7 @@ public class BatteryStatsImpl extends BatteryStats { } synchronized (mWifiNetworkLock) { - if (ConnectivityManager.isNetworkTypeWifi(networkType)) { + if (displayTransport == TRANSPORT_WIFI) { mWifiIfaces = includeInStringArray(mWifiIfaces, iface); if (DEBUG) Slog.d(TAG, "Note wifi iface " + iface + ": " + mWifiIfaces); } else { diff --git a/core/tests/coretests/src/com/android/internal/os/MobileRadioPowerCalculatorTest.java b/core/tests/coretests/src/com/android/internal/os/MobileRadioPowerCalculatorTest.java index 4230066f32a18..66a8379177c20 100644 --- a/core/tests/coretests/src/com/android/internal/os/MobileRadioPowerCalculatorTest.java +++ b/core/tests/coretests/src/com/android/internal/os/MobileRadioPowerCalculatorTest.java @@ -21,7 +21,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import android.net.ConnectivityManager; +import android.net.NetworkCapabilities; import android.net.NetworkStats; import android.os.BatteryConsumer; import android.os.Process; @@ -78,7 +78,8 @@ public class MobileRadioPowerCalculatorTest { 8_000_000_000L, APP_UID, 8000, 8000); // Note established network - stats.noteNetworkInterfaceType("cellular", ConnectivityManager.TYPE_MOBILE); + stats.noteNetworkInterfaceForTransports("cellular", + new int[] { NetworkCapabilities.TRANSPORT_CELLULAR }); // Note application network activity NetworkStats networkStats = new NetworkStats(10000, 1) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b999b7ece355f..f323bbf67ff32 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6196,7 +6196,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nai.networkAgentPortalData = lp.getCaptivePortalData(); } - private void updateLinkProperties(NetworkAgentInfo networkAgent, LinkProperties newLp, + private void updateLinkProperties(NetworkAgentInfo networkAgent, @NonNull LinkProperties newLp, @NonNull LinkProperties oldLp) { int netId = networkAgent.network.getNetId(); @@ -6205,8 +6205,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // the LinkProperties for the network are accurate. networkAgent.clatd.fixupLinkProperties(oldLp, newLp); - updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities, - networkAgent.networkInfo.getType()); + updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities); // update filtering rules, need to happen after the interface update so netd knows about the // new interface (the interface name -> index map becomes initialized) @@ -6345,7 +6344,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private void updateInterfaces(final @Nullable LinkProperties newLp, final @Nullable LinkProperties oldLp, final int netId, - final @Nullable NetworkCapabilities caps, final int legacyType) { + final @NonNull NetworkCapabilities caps) { final CompareResult interfaceDiff = new CompareResult<>( oldLp != null ? oldLp.getAllInterfaceNames() : null, newLp != null ? newLp.getAllInterfaceNames() : null); @@ -6356,7 +6355,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (DBG) log("Adding iface " + iface + " to network " + netId); mNetd.networkAddInterface(netId, iface); wakeupModifyInterface(iface, caps, true); - bs.noteNetworkInterfaceType(iface, legacyType); + bs.noteNetworkInterfaceForTransports(iface, caps.getTransportTypes()); } catch (Exception e) { loge("Exception adding interface: " + e); } @@ -6628,6 +6627,7 @@ public class ConnectivityService extends IConnectivityManager.Stub * maintained here that the NetworkAgent is not aware of (e.g., validated, captive portal, * and foreground status). */ + @NonNull private NetworkCapabilities mixInCapabilities(NetworkAgentInfo nai, NetworkCapabilities nc) { // Once a NetworkAgent is connected, complain if some immutable capabilities are removed. // Don't complain for VPNs since they're not driven by requests and there is no risk of diff --git a/services/core/java/com/android/server/am/BatteryStatsService.java b/services/core/java/com/android/server/am/BatteryStatsService.java index b1cbb4acdb130..50e55579dd741 100644 --- a/services/core/java/com/android/server/am/BatteryStatsService.java +++ b/services/core/java/com/android/server/am/BatteryStatsService.java @@ -1674,11 +1674,11 @@ public final class BatteryStatsService extends IBatteryStats.Stub } @Override - public void noteNetworkInterfaceType(final String iface, final int networkType) { + public void noteNetworkInterfaceForTransports(final String iface, int[] transportTypes) { enforceCallingPermission(); synchronized (mLock) { mHandler.post(() -> { - mStats.noteNetworkInterfaceType(iface, networkType); + mStats.noteNetworkInterfaceForTransports(iface, transportTypes); }); } } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index ab0360b0395a8..b2824846008ca 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -329,7 +329,7 @@ public class NetworkAgentInfo implements Comparable { private final QosCallbackTracker mQosCallbackTracker; public NetworkAgentInfo(INetworkAgent na, Network net, NetworkInfo info, - LinkProperties lp, NetworkCapabilities nc, int score, Context context, + @NonNull LinkProperties lp, @NonNull NetworkCapabilities nc, int score, Context context, Handler handler, NetworkAgentConfig config, ConnectivityService connService, INetd netd, IDnsResolver dnsResolver, INetworkManagementService nms, int factorySerialNumber, int creatorUid, QosCallbackTracker qosCallbackTracker) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index c5e6c3507ebfb..47f2b32f465ea 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -7367,7 +7367,6 @@ public class ConnectivityServiceTest { mCm.unregisterNetworkCallback(trustedCallback); } - @Ignore // 40%+ flakiness : figure out why and re-enable. @Test public final void testBatteryStatsNetworkType() throws Exception { final LinkProperties cellLp = new LinkProperties(); @@ -7375,8 +7374,8 @@ public class ConnectivityServiceTest { mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); mCellNetworkAgent.connect(true); waitForIdle(); - verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), - TYPE_MOBILE); + verify(mBatteryStatsService).noteNetworkInterfaceForTransports(cellLp.getInterfaceName(), + new int[] { TRANSPORT_CELLULAR }); reset(mBatteryStatsService); final LinkProperties wifiLp = new LinkProperties(); @@ -7384,18 +7383,20 @@ public class ConnectivityServiceTest { mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, wifiLp); mWiFiNetworkAgent.connect(true); waitForIdle(); - verify(mBatteryStatsService).noteNetworkInterfaceType(wifiLp.getInterfaceName(), - TYPE_WIFI); + verify(mBatteryStatsService).noteNetworkInterfaceForTransports(wifiLp.getInterfaceName(), + new int[] { TRANSPORT_WIFI }); reset(mBatteryStatsService); mCellNetworkAgent.disconnect(); + mWiFiNetworkAgent.disconnect(); cellLp.setInterfaceName("wifi0"); mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); mCellNetworkAgent.connect(true); waitForIdle(); - verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), - TYPE_MOBILE); + verify(mBatteryStatsService).noteNetworkInterfaceForTransports(cellLp.getInterfaceName(), + new int[] { TRANSPORT_CELLULAR }); + mCellNetworkAgent.disconnect(); } /** @@ -7468,8 +7469,8 @@ public class ConnectivityServiceTest { assertRoutesAdded(cellNetId, ipv6Subnet, defaultRoute); verify(mMockDnsResolver, times(1)).createNetworkCache(eq(cellNetId)); verify(mMockNetd, times(1)).networkAddInterface(cellNetId, MOBILE_IFNAME); - verify(mBatteryStatsService).noteNetworkInterfaceType(cellLp.getInterfaceName(), - TYPE_MOBILE); + verify(mBatteryStatsService).noteNetworkInterfaceForTransports(cellLp.getInterfaceName(), + new int[] { TRANSPORT_CELLULAR }); networkCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); verify(mMockDnsResolver, times(1)).startPrefix64Discovery(cellNetId); @@ -7489,7 +7490,8 @@ public class ConnectivityServiceTest { // Make sure BatteryStats was not told about any v4- interfaces, as none should have // come online yet. waitForIdle(); - verify(mBatteryStatsService, never()).noteNetworkInterfaceType(startsWith("v4-"), anyInt()); + verify(mBatteryStatsService, never()).noteNetworkInterfaceForTransports(startsWith("v4-"), + any()); verifyNoMoreInteractions(mMockNetd); verifyNoMoreInteractions(mMockDnsResolver); @@ -7542,8 +7544,8 @@ public class ConnectivityServiceTest { assertTrue(ArrayUtils.contains(resolvrParams.servers, "8.8.8.8")); for (final LinkProperties stackedLp : stackedLpsAfterChange) { - verify(mBatteryStatsService).noteNetworkInterfaceType(stackedLp.getInterfaceName(), - TYPE_MOBILE); + verify(mBatteryStatsService).noteNetworkInterfaceForTransports( + stackedLp.getInterfaceName(), new int[] { TRANSPORT_CELLULAR }); } reset(mMockNetd); when(mMockNetd.interfaceGetCfg(CLAT_PREFIX + MOBILE_IFNAME))