From 67430df457d747344088e08ed52a27e5acde3077 Mon Sep 17 00:00:00 2001 From: markchien Date: Tue, 14 Jan 2020 18:25:33 +0800 Subject: [PATCH 1/6] Use Context#getSystemService to get network stack binder Bug: 143195885 Test: -build, flash, boot -atest TetheringTests Change-Id: Ic5fabeae27677344d691449c31ad9337c0e6d92c --- .../server/connectivity/tethering/TetheringService.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringService.java b/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringService.java index e4e4a090603db..99130d20e614d 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringService.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringService.java @@ -42,7 +42,6 @@ import android.os.IBinder; import android.os.Looper; import android.os.RemoteException; import android.os.ResultReceiver; -import android.os.ServiceManager; import android.os.SystemProperties; import android.os.UserManager; import android.provider.Settings; @@ -363,7 +362,7 @@ public class TetheringService extends Service { IBinder connector; try { final long before = System.currentTimeMillis(); - while ((connector = ServiceManager.getService( + while ((connector = (IBinder) mContext.getSystemService( Context.NETWORK_STACK_SERVICE)) == null) { if (System.currentTimeMillis() - before > NETWORKSTACK_TIMEOUT_MS) { Log.wtf(TAG, "Timeout, fail to get INetworkStackConnector"); From fb7fb59a3131c633758d91b7c984064f63a19b04 Mon Sep 17 00:00:00 2001 From: junyulai Date: Thu, 9 Jan 2020 18:56:08 +0800 Subject: [PATCH 2/6] [SP06] Use system API to communicate with NSS in OffloadController Test: atest FrameworksNetTests OffloadControllerTest TetheringTest Bug: 130855321 Change-Id: I294be3a2874f8c8120857e308e629199af014fcd --- .../tethering/OffloadController.java | 150 +++++++++------- .../tethering/OffloadHardwareInterface.java | 8 + .../connectivity/tethering/Tethering.java | 6 +- .../tethering/OffloadControllerTest.java | 165 +++++++++++------- .../connectivity/tethering/TetheringTest.java | 2 +- 5 files changed, 205 insertions(+), 126 deletions(-) diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadController.java b/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadController.java index ce7c2a669f0a0..cc36f4a9c516f 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadController.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadController.java @@ -16,35 +16,40 @@ package com.android.server.connectivity.tethering; +import static android.net.NetworkStats.DEFAULT_NETWORK_NO; +import static android.net.NetworkStats.METERED_NO; +import static android.net.NetworkStats.ROAMING_NO; import static android.net.NetworkStats.SET_DEFAULT; -import static android.net.NetworkStats.STATS_PER_UID; import static android.net.NetworkStats.TAG_NONE; import static android.net.NetworkStats.UID_ALL; -import static android.net.TrafficStats.UID_TETHERING; +import static android.net.NetworkStats.UID_TETHERING; import static android.provider.Settings.Global.TETHER_OFFLOAD_DISABLED; +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.app.usage.NetworkStatsManager; import android.content.ContentResolver; -import android.net.ITetheringStatsProvider; import android.net.InetAddresses; import android.net.IpPrefix; import android.net.LinkAddress; import android.net.LinkProperties; import android.net.NetworkStats; +import android.net.NetworkStats.Entry; import android.net.RouteInfo; import android.net.netlink.ConntrackMessage; import android.net.netlink.NetlinkConstants; import android.net.netlink.NetlinkSocket; +import android.net.netstats.provider.AbstractNetworkStatsProvider; +import android.net.netstats.provider.NetworkStatsProviderCallback; import android.net.util.SharedLog; import android.os.Handler; -import android.os.INetworkManagementService; -import android.os.Looper; -import android.os.RemoteException; -import android.os.SystemClock; import android.provider.Settings; import android.system.ErrnoException; import android.system.OsConstants; import android.text.TextUtils; +import android.util.Log; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.server.connectivity.tethering.OffloadHardwareInterface.ForwardedStats; @@ -73,13 +78,19 @@ public class OffloadController { private static final String ANYIP = "0.0.0.0"; private static final ForwardedStats EMPTY_STATS = new ForwardedStats(); + @VisibleForTesting + enum StatsType { + STATS_PER_IFACE, + STATS_PER_UID, + } + private enum UpdateType { IF_NEEDED, FORCE }; private final Handler mHandler; private final OffloadHardwareInterface mHwInterface; private final ContentResolver mContentResolver; - private final INetworkManagementService mNms; - private final ITetheringStatsProvider mStatsProvider; + private final @NonNull OffloadTetheringStatsProvider mStatsProvider; + private final @Nullable NetworkStatsProviderCallback mStatsProviderCb; private final SharedLog mLog; private final HashMap mDownstreams; private boolean mConfigInitialized; @@ -109,22 +120,23 @@ public class OffloadController { private int mNatUpdateNetlinkErrors; public OffloadController(Handler h, OffloadHardwareInterface hwi, - ContentResolver contentResolver, INetworkManagementService nms, SharedLog log) { + ContentResolver contentResolver, NetworkStatsManager nsm, SharedLog log) { mHandler = h; mHwInterface = hwi; mContentResolver = contentResolver; - mNms = nms; mStatsProvider = new OffloadTetheringStatsProvider(); mLog = log.forSubComponent(TAG); mDownstreams = new HashMap<>(); mExemptPrefixes = new HashSet<>(); mLastLocalPrefixStrs = new HashSet<>(); - + NetworkStatsProviderCallback providerCallback = null; try { - mNms.registerTetheringStatsProvider(mStatsProvider, getClass().getSimpleName()); - } catch (RemoteException e) { - mLog.e("Cannot register offload stats provider: " + e); + providerCallback = nsm.registerNetworkStatsProvider( + getClass().getSimpleName(), mStatsProvider); + } catch (RuntimeException e) { + Log.wtf(TAG, "Cannot register offload stats provider: " + e); } + mStatsProviderCb = providerCallback; } /** Start hardware offload. */ @@ -173,7 +185,7 @@ public class OffloadController { // and we need to synchronize stats and limits between // software and hardware forwarding. updateStatsForAllUpstreams(); - forceTetherStatsPoll(); + mStatsProvider.pushTetherStats(); } @Override @@ -186,7 +198,7 @@ public class OffloadController { // limits set take into account any software tethering // traffic that has been happening in the meantime. updateStatsForAllUpstreams(); - forceTetherStatsPoll(); + mStatsProvider.pushTetherStats(); // [2] (Re)Push all state. computeAndPushLocalPrefixes(UpdateType.FORCE); pushAllDownstreamState(); @@ -204,14 +216,11 @@ public class OffloadController { // the HAL queued the callback. // TODO: rev the HAL so that it provides an interface name. - // Fetch current stats, so that when our notification reaches - // NetworkStatsService and triggers a poll, we will respond with - // current data (which will be above the limit that was reached). - // Note that if we just changed upstream, this is unnecessary but harmless. - // The stats for the previous upstream were already updated on this thread - // just after the upstream was changed, so they are also up-to-date. updateStatsForCurrentUpstream(); - forceTetherStatsPoll(); + mStatsProvider.pushTetherStats(); + // Push stats to service does not cause the service react to it immediately. + // Inform the service about limit reached. + if (mStatsProviderCb != null) mStatsProviderCb.onLimitReached(); } @Override @@ -253,42 +262,37 @@ public class OffloadController { return mConfigInitialized && mControlInitialized; } - private class OffloadTetheringStatsProvider extends ITetheringStatsProvider.Stub { - @Override - public NetworkStats getTetherStats(int how) { - // getTetherStats() is the only function in OffloadController that can be called from - // a different thread. Do not attempt to update stats by querying the offload HAL - // synchronously from a different thread than our Handler thread. http://b/64771555. - Runnable updateStats = () -> { - updateStatsForCurrentUpstream(); - }; - if (Looper.myLooper() == mHandler.getLooper()) { - updateStats.run(); - } else { - mHandler.post(updateStats); - } + @VisibleForTesting + class OffloadTetheringStatsProvider extends AbstractNetworkStatsProvider { + // These stats must only ever be touched on the handler thread. + @NonNull + private NetworkStats mIfaceStats = new NetworkStats(0L, 0); + @NonNull + private NetworkStats mUidStats = new NetworkStats(0L, 0); - NetworkStats stats = new NetworkStats(SystemClock.elapsedRealtime(), 0); - NetworkStats.Entry entry = new NetworkStats.Entry(); - entry.set = SET_DEFAULT; - entry.tag = TAG_NONE; - entry.uid = (how == STATS_PER_UID) ? UID_TETHERING : UID_ALL; + @VisibleForTesting + @NonNull + NetworkStats getTetherStats(@NonNull StatsType how) { + NetworkStats stats = new NetworkStats(0L, 0); + final int uid = (how == StatsType.STATS_PER_UID) ? UID_TETHERING : UID_ALL; - for (Map.Entry kv : mForwardedStats.entrySet()) { - ForwardedStats value = kv.getValue(); - entry.iface = kv.getKey(); - entry.rxBytes = value.rxBytes; - entry.txBytes = value.txBytes; - stats.addEntry(entry); + for (final Map.Entry kv : mForwardedStats.entrySet()) { + final ForwardedStats value = kv.getValue(); + final Entry entry = new Entry(kv.getKey(), uid, SET_DEFAULT, TAG_NONE, METERED_NO, + ROAMING_NO, DEFAULT_NETWORK_NO, value.rxBytes, 0L, value.txBytes, 0L, 0L); + stats = stats.addValues(entry); } return stats; } @Override - public void setInterfaceQuota(String iface, long quotaBytes) { + public void setLimit(String iface, long quotaBytes) { + mLog.i("setLimit: " + iface + "," + quotaBytes); + // Listen for all iface is necessary since upstream might be changed after limit + // is set. mHandler.post(() -> { - if (quotaBytes == ITetheringStatsProvider.QUOTA_UNLIMITED) { + if (quotaBytes == QUOTA_UNLIMITED) { mInterfaceQuotas.remove(iface); } else { mInterfaceQuotas.put(iface, quotaBytes); @@ -296,6 +300,42 @@ public class OffloadController { maybeUpdateDataLimit(iface); }); } + + /** + * Push stats to service, but does not cause a force polling. Note that this can only be + * called on the handler thread. + */ + public void pushTetherStats() { + // TODO: remove the accumulated stats and report the diff from HAL directly. + if (null == mStatsProviderCb) return; + final NetworkStats ifaceDiff = + getTetherStats(StatsType.STATS_PER_IFACE).subtract(mIfaceStats); + final NetworkStats uidDiff = + getTetherStats(StatsType.STATS_PER_UID).subtract(mUidStats); + try { + mStatsProviderCb.onStatsUpdated(0 /* token */, ifaceDiff, uidDiff); + mIfaceStats = mIfaceStats.add(ifaceDiff); + mUidStats = mUidStats.add(uidDiff); + } catch (RuntimeException e) { + mLog.e("Cannot report network stats: ", e); + } + } + + @Override + public void requestStatsUpdate(int token) { + mLog.i("requestStatsUpdate: " + token); + // Do not attempt to update stats by querying the offload HAL + // synchronously from a different thread than the Handler thread. http://b/64771555. + mHandler.post(() -> { + updateStatsForCurrentUpstream(); + pushTetherStats(); + }); + } + + @Override + public void setAlert(long quotaBytes) { + // TODO: Ask offload HAL to notify alert without stopping traffic. + } } private String currentUpstreamInterface() { @@ -353,14 +393,6 @@ public class OffloadController { } } - private void forceTetherStatsPoll() { - try { - mNms.tetherLimitReached(mStatsProvider); - } catch (RemoteException e) { - mLog.e("Cannot report data limit reached: " + e); - } - } - /** Set current tethering upstream LinkProperties. */ public void setUpstreamLinkProperties(LinkProperties lp) { if (!started() || Objects.equals(mUpstreamLinkProperties, lp)) return; diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadHardwareInterface.java b/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadHardwareInterface.java index 4a8ef1f927544..90b9d3f148dce 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadHardwareInterface.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/OffloadHardwareInterface.java @@ -29,6 +29,8 @@ import android.os.Handler; import android.os.RemoteException; import android.system.OsConstants; +import com.android.internal.annotations.VisibleForTesting; + import java.util.ArrayList; @@ -91,6 +93,12 @@ public class OffloadHardwareInterface { txBytes = 0; } + @VisibleForTesting + public ForwardedStats(long rxBytes, long txBytes) { + this.rxBytes = rxBytes; + this.txBytes = txBytes; + } + /** Add Tx/Rx bytes. */ public void add(ForwardedStats other) { rxBytes += other.rxBytes; diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java index 5b97f50f12088..0ee8164f3b906 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java @@ -53,6 +53,7 @@ import android.app.Notification; import android.app.NotificationChannel; import android.app.NotificationManager; import android.app.PendingIntent; +import android.app.usage.NetworkStatsManager; import android.bluetooth.BluetoothAdapter; import android.bluetooth.BluetoothPan; import android.bluetooth.BluetoothProfile; @@ -224,10 +225,11 @@ public class Tethering { mTetherMasterSM = new TetherMasterSM("TetherMaster", mLooper, deps); mTetherMasterSM.start(); + final NetworkStatsManager statsManager = new NetworkStatsManager(mContext, mStatsService); mHandler = mTetherMasterSM.getHandler(); mOffloadController = new OffloadController(mHandler, mDeps.getOffloadHardwareInterface(mHandler, mLog), mContext.getContentResolver(), - mDeps.getINetworkManagementService(), mLog); + statsManager, mLog); mUpstreamNetworkMonitor = mDeps.getUpstreamNetworkMonitor(mContext, mTetherMasterSM, mLog, TetherMasterSM.EVENT_UPSTREAM_CALLBACK); mForwardedDownstreams = new HashSet<>(); @@ -264,7 +266,7 @@ public class Tethering { } final UserManager userManager = (UserManager) mContext.getSystemService( - Context.USER_SERVICE); + Context.USER_SERVICE); mTetheringRestriction = new UserRestrictionActionListener(userManager, this); final TetheringThreadExecutor executor = new TetheringThreadExecutor(mHandler); mActiveDataSubIdListener = new ActiveDataSubIdListener(executor); diff --git a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/OffloadControllerTest.java b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/OffloadControllerTest.java index 7886ca6c132d9..7e62e5aca993d 100644 --- a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/OffloadControllerTest.java +++ b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/OffloadControllerTest.java @@ -16,21 +16,26 @@ package com.android.server.connectivity.tethering; +import static android.net.NetworkStats.DEFAULT_NETWORK_NO; +import static android.net.NetworkStats.METERED_NO; +import static android.net.NetworkStats.ROAMING_NO; import static android.net.NetworkStats.SET_DEFAULT; -import static android.net.NetworkStats.STATS_PER_IFACE; -import static android.net.NetworkStats.STATS_PER_UID; import static android.net.NetworkStats.TAG_NONE; import static android.net.NetworkStats.UID_ALL; +import static android.net.NetworkStats.UID_TETHERING; import static android.net.RouteInfo.RTN_UNICAST; -import static android.net.TrafficStats.UID_TETHERING; import static android.provider.Settings.Global.TETHER_OFFLOAD_DISABLED; +import static com.android.server.connectivity.tethering.OffloadController.StatsType.STATS_PER_IFACE; +import static com.android.server.connectivity.tethering.OffloadController.StatsType.STATS_PER_UID; import static com.android.server.connectivity.tethering.OffloadHardwareInterface.ForwardedStats; import static com.android.testutils.MiscAssertsKt.assertContainsAll; import static com.android.testutils.MiscAssertsKt.assertThrows; +import static com.android.testutils.NetworkStatsUtilsKt.orderInsensitiveEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyObject; @@ -39,11 +44,14 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import android.annotation.NonNull; +import android.app.usage.NetworkStatsManager; import android.content.Context; import android.content.pm.ApplicationInfo; import android.net.ITetheringStatsProvider; @@ -51,10 +59,12 @@ import android.net.IpPrefix; import android.net.LinkAddress; import android.net.LinkProperties; import android.net.NetworkStats; +import android.net.NetworkStats.Entry; import android.net.RouteInfo; +import android.net.netstats.provider.AbstractNetworkStatsProvider; +import android.net.netstats.provider.NetworkStatsProviderCallback; import android.net.util.SharedLog; import android.os.Handler; -import android.os.INetworkManagementService; import android.os.Looper; import android.provider.Settings; import android.provider.Settings.SettingNotFoundException; @@ -97,11 +107,13 @@ public class OffloadControllerTest { @Mock private OffloadHardwareInterface mHardware; @Mock private ApplicationInfo mApplicationInfo; @Mock private Context mContext; - @Mock private INetworkManagementService mNMService; + @Mock private NetworkStatsManager mStatsManager; + @Mock private NetworkStatsProviderCallback mTetherStatsProviderCb; private final ArgumentCaptor mStringArrayCaptor = ArgumentCaptor.forClass(ArrayList.class); - private final ArgumentCaptor mTetherStatsProviderCaptor = - ArgumentCaptor.forClass(ITetheringStatsProvider.Stub.class); + private final ArgumentCaptor + mTetherStatsProviderCaptor = + ArgumentCaptor.forClass(OffloadController.OffloadTetheringStatsProvider.class); private final ArgumentCaptor mControlCallbackCaptor = ArgumentCaptor.forClass(OffloadHardwareInterface.ControlCallback.class); private MockContentResolver mContentResolver; @@ -114,6 +126,8 @@ public class OffloadControllerTest { mContentResolver.addProvider(Settings.AUTHORITY, new FakeSettingsProvider()); when(mContext.getContentResolver()).thenReturn(mContentResolver); FakeSettingsProvider.clearSettingsProvider(); + when(mStatsManager.registerNetworkStatsProvider(anyString(), any())) + .thenReturn(mTetherStatsProviderCb); } @After public void tearDown() throws Exception { @@ -139,9 +153,9 @@ public class OffloadControllerTest { private OffloadController makeOffloadController() throws Exception { OffloadController offload = new OffloadController(new Handler(Looper.getMainLooper()), - mHardware, mContentResolver, mNMService, new SharedLog("test")); - verify(mNMService).registerTetheringStatsProvider( - mTetherStatsProviderCaptor.capture(), anyString()); + mHardware, mContentResolver, mStatsManager, new SharedLog("test")); + verify(mStatsManager).registerNetworkStatsProvider(anyString(), + mTetherStatsProviderCaptor.capture()); return offload; } @@ -384,12 +398,11 @@ public class OffloadControllerTest { inOrder.verifyNoMoreInteractions(); } - private void assertNetworkStats(String iface, ForwardedStats stats, NetworkStats.Entry entry) { - assertEquals(iface, entry.iface); - assertEquals(stats.rxBytes, entry.rxBytes); - assertEquals(stats.txBytes, entry.txBytes); - assertEquals(SET_DEFAULT, entry.set); - assertEquals(TAG_NONE, entry.tag); + private static @NonNull Entry buildTestEntry(@NonNull OffloadController.StatsType how, + @NonNull String iface, long rxBytes, long txBytes) { + return new Entry(iface, how == STATS_PER_IFACE ? UID_ALL : UID_TETHERING, SET_DEFAULT, + TAG_NONE, METERED_NO, ROAMING_NO, DEFAULT_NETWORK_NO, rxBytes, 0L, + txBytes, 0L, 0L); } @Test @@ -400,19 +413,16 @@ public class OffloadControllerTest { final OffloadController offload = makeOffloadController(); offload.start(); + final OffloadController.OffloadTetheringStatsProvider provider = + mTetherStatsProviderCaptor.getValue(); + final String ethernetIface = "eth1"; final String mobileIface = "rmnet_data0"; - ForwardedStats ethernetStats = new ForwardedStats(); - ethernetStats.rxBytes = 12345; - ethernetStats.txBytes = 54321; - - ForwardedStats mobileStats = new ForwardedStats(); - mobileStats.rxBytes = 999; - mobileStats.txBytes = 99999; - - when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn(ethernetStats); - when(mHardware.getForwardedStats(eq(mobileIface))).thenReturn(mobileStats); + when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn( + new ForwardedStats(12345, 54321)); + when(mHardware.getForwardedStats(eq(mobileIface))).thenReturn( + new ForwardedStats(999, 99999)); InOrder inOrder = inOrder(mHardware); @@ -432,10 +442,35 @@ public class OffloadControllerTest { // Expect that we fetch stats from the previous upstream. inOrder.verify(mHardware, times(1)).getForwardedStats(eq(mobileIface)); - ethernetStats = new ForwardedStats(); - ethernetStats.rxBytes = 100000; - ethernetStats.txBytes = 100000; - when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn(ethernetStats); + // Verify that the fetched stats are stored. + final NetworkStats ifaceStats = provider.getTetherStats(STATS_PER_IFACE); + final NetworkStats uidStats = provider.getTetherStats(STATS_PER_UID); + final NetworkStats expectedIfaceStats = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_IFACE, mobileIface, 999, 99999)) + .addValues(buildTestEntry(STATS_PER_IFACE, ethernetIface, 12345, 54321)); + + final NetworkStats expectedUidStats = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_UID, mobileIface, 999, 99999)) + .addValues(buildTestEntry(STATS_PER_UID, ethernetIface, 12345, 54321)); + + assertTrue(orderInsensitiveEquals(expectedIfaceStats, ifaceStats)); + assertTrue(orderInsensitiveEquals(expectedUidStats, uidStats)); + + final ArgumentCaptor ifaceStatsCaptor = ArgumentCaptor.forClass( + NetworkStats.class); + final ArgumentCaptor uidStatsCaptor = ArgumentCaptor.forClass( + NetworkStats.class); + + // Force pushing stats update to verify the stats reported. + provider.pushTetherStats(); + verify(mTetherStatsProviderCb, times(1)).onStatsUpdated(anyInt(), + ifaceStatsCaptor.capture(), uidStatsCaptor.capture()); + assertTrue(orderInsensitiveEquals(expectedIfaceStats, ifaceStatsCaptor.getValue())); + assertTrue(orderInsensitiveEquals(expectedUidStats, uidStatsCaptor.getValue())); + + + when(mHardware.getForwardedStats(eq(ethernetIface))).thenReturn( + new ForwardedStats(100000, 100000)); offload.setUpstreamLinkProperties(null); // Expect that we first clear the HAL's upstream parameters. inOrder.verify(mHardware, times(1)).setUpstreamParameters( @@ -443,37 +478,38 @@ public class OffloadControllerTest { // Expect that we fetch stats from the previous upstream. inOrder.verify(mHardware, times(1)).getForwardedStats(eq(ethernetIface)); - ITetheringStatsProvider provider = mTetherStatsProviderCaptor.getValue(); - NetworkStats stats = provider.getTetherStats(STATS_PER_IFACE); - NetworkStats perUidStats = provider.getTetherStats(STATS_PER_UID); - waitForIdle(); // There is no current upstream, so no stats are fetched. inOrder.verify(mHardware, never()).getForwardedStats(any()); inOrder.verifyNoMoreInteractions(); - assertEquals(2, stats.size()); - assertEquals(2, perUidStats.size()); + // Verify that the stored stats is accumulated. + final NetworkStats ifaceStatsAccu = provider.getTetherStats(STATS_PER_IFACE); + final NetworkStats uidStatsAccu = provider.getTetherStats(STATS_PER_UID); + final NetworkStats expectedIfaceStatsAccu = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_IFACE, mobileIface, 999, 99999)) + .addValues(buildTestEntry(STATS_PER_IFACE, ethernetIface, 112345, 154321)); - NetworkStats.Entry entry = null; - for (int i = 0; i < stats.size(); i++) { - assertEquals(UID_ALL, stats.getValues(i, entry).uid); - assertEquals(UID_TETHERING, perUidStats.getValues(i, entry).uid); - } + final NetworkStats expectedUidStatsAccu = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_UID, mobileIface, 999, 99999)) + .addValues(buildTestEntry(STATS_PER_UID, ethernetIface, 112345, 154321)); - int ethernetPosition = ethernetIface.equals(stats.getValues(0, entry).iface) ? 0 : 1; - int mobilePosition = 1 - ethernetPosition; + assertTrue(orderInsensitiveEquals(expectedIfaceStatsAccu, ifaceStatsAccu)); + assertTrue(orderInsensitiveEquals(expectedUidStatsAccu, uidStatsAccu)); - entry = stats.getValues(mobilePosition, entry); - assertNetworkStats(mobileIface, mobileStats, entry); - entry = perUidStats.getValues(mobilePosition, entry); - assertNetworkStats(mobileIface, mobileStats, entry); + // Verify that only diff of stats is reported. + reset(mTetherStatsProviderCb); + provider.pushTetherStats(); + final NetworkStats expectedIfaceStatsDiff = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_IFACE, mobileIface, 0, 0)) + .addValues(buildTestEntry(STATS_PER_IFACE, ethernetIface, 100000, 100000)); - ethernetStats.rxBytes = 12345 + 100000; - ethernetStats.txBytes = 54321 + 100000; - entry = stats.getValues(ethernetPosition, entry); - assertNetworkStats(ethernetIface, ethernetStats, entry); - entry = perUidStats.getValues(ethernetPosition, entry); - assertNetworkStats(ethernetIface, ethernetStats, entry); + final NetworkStats expectedUidStatsDiff = new NetworkStats(0L, 2) + .addValues(buildTestEntry(STATS_PER_UID, mobileIface, 0, 0)) + .addValues(buildTestEntry(STATS_PER_UID, ethernetIface, 100000, 100000)); + verify(mTetherStatsProviderCb, times(1)).onStatsUpdated(anyInt(), + ifaceStatsCaptor.capture(), uidStatsCaptor.capture()); + assertTrue(orderInsensitiveEquals(expectedIfaceStatsDiff, ifaceStatsCaptor.getValue())); + assertTrue(orderInsensitiveEquals(expectedUidStatsDiff, uidStatsCaptor.getValue())); } @Test @@ -493,19 +529,19 @@ public class OffloadControllerTest { lp.setInterfaceName(ethernetIface); offload.setUpstreamLinkProperties(lp); - ITetheringStatsProvider provider = mTetherStatsProviderCaptor.getValue(); + AbstractNetworkStatsProvider provider = mTetherStatsProviderCaptor.getValue(); final InOrder inOrder = inOrder(mHardware); when(mHardware.setUpstreamParameters(any(), any(), any(), any())).thenReturn(true); when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(true); // Applying an interface quota to the current upstream immediately sends it to the hardware. - provider.setInterfaceQuota(ethernetIface, ethernetLimit); + provider.setLimit(ethernetIface, ethernetLimit); waitForIdle(); inOrder.verify(mHardware).setDataLimit(ethernetIface, ethernetLimit); inOrder.verifyNoMoreInteractions(); // Applying an interface quota to another upstream does not take any immediate action. - provider.setInterfaceQuota(mobileIface, mobileLimit); + provider.setLimit(mobileIface, mobileLimit); waitForIdle(); inOrder.verify(mHardware, never()).setDataLimit(anyString(), anyLong()); @@ -518,7 +554,7 @@ public class OffloadControllerTest { // Setting a limit of ITetheringStatsProvider.QUOTA_UNLIMITED causes the limit to be set // to Long.MAX_VALUE. - provider.setInterfaceQuota(mobileIface, ITetheringStatsProvider.QUOTA_UNLIMITED); + provider.setLimit(mobileIface, ITetheringStatsProvider.QUOTA_UNLIMITED); waitForIdle(); inOrder.verify(mHardware).setDataLimit(mobileIface, Long.MAX_VALUE); @@ -526,7 +562,7 @@ public class OffloadControllerTest { when(mHardware.setUpstreamParameters(any(), any(), any(), any())).thenReturn(false); lp.setInterfaceName(ethernetIface); offload.setUpstreamLinkProperties(lp); - provider.setInterfaceQuota(mobileIface, mobileLimit); + provider.setLimit(mobileIface, mobileLimit); waitForIdle(); inOrder.verify(mHardware, never()).setDataLimit(anyString(), anyLong()); @@ -535,7 +571,7 @@ public class OffloadControllerTest { when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(false); lp.setInterfaceName(mobileIface); offload.setUpstreamLinkProperties(lp); - provider.setInterfaceQuota(mobileIface, mobileLimit); + provider.setLimit(mobileIface, mobileLimit); waitForIdle(); inOrder.verify(mHardware).getForwardedStats(ethernetIface); inOrder.verify(mHardware).stopOffloadControl(); @@ -551,7 +587,7 @@ public class OffloadControllerTest { OffloadHardwareInterface.ControlCallback callback = mControlCallbackCaptor.getValue(); callback.onStoppedLimitReached(); - verify(mNMService, times(1)).tetherLimitReached(mTetherStatsProviderCaptor.getValue()); + verify(mTetherStatsProviderCb, times(1)).onStatsUpdated(anyInt(), any(), any()); } @Test @@ -654,9 +690,10 @@ public class OffloadControllerTest { // Verify forwarded stats behaviour. verify(mHardware, times(1)).getForwardedStats(eq(RMNET0)); verify(mHardware, times(1)).getForwardedStats(eq(WLAN0)); + // TODO: verify the exact stats reported. + verify(mTetherStatsProviderCb, times(1)).onStatsUpdated(anyInt(), any(), any()); + verifyNoMoreInteractions(mTetherStatsProviderCb); verifyNoMoreInteractions(mHardware); - verify(mNMService, times(1)).tetherLimitReached(mTetherStatsProviderCaptor.getValue()); - verifyNoMoreInteractions(mNMService); } @Test @@ -719,8 +756,8 @@ public class OffloadControllerTest { // Verify forwarded stats behaviour. verify(mHardware, times(1)).getForwardedStats(eq(RMNET0)); verify(mHardware, times(1)).getForwardedStats(eq(WLAN0)); - verify(mNMService, times(1)).tetherLimitReached(mTetherStatsProviderCaptor.getValue()); - verifyNoMoreInteractions(mNMService); + verify(mTetherStatsProviderCb, times(1)).onStatsUpdated(anyInt(), any(), any()); + verifyNoMoreInteractions(mTetherStatsProviderCb); // TODO: verify local prefixes and downstreams are also pushed to the HAL. verify(mHardware, times(1)).setLocalPrefixes(mStringArrayCaptor.capture()); diff --git a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java index 0df32fd9f35bb..00afb97120973 100644 --- a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java +++ b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java @@ -457,7 +457,7 @@ public class TetheringTest { mServiceContext.registerReceiver(mBroadcastReceiver, new IntentFilter(ACTION_TETHER_STATE_CHANGED)); mTethering = makeTethering(); - verify(mNMService).registerTetheringStatsProvider(any(), anyString()); + verify(mStatsService, times(1)).registerNetworkStatsProvider(anyString(), any()); verify(mNetd).registerUnsolicitedEventListener(any()); final ArgumentCaptor phoneListenerCaptor = ArgumentCaptor.forClass(PhoneStateListener.class); From 37ed835f3ae141f1823c4d511269668d15ce38c5 Mon Sep 17 00:00:00 2001 From: junyulai Date: Fri, 10 Jan 2020 13:27:24 +0800 Subject: [PATCH 3/6] [SP06.1] Add UPDATE_DEVICE_STATS permission to Tethering Add corresponding permission to allow tether offload to update network usage through new API that requires UPDATE_DEVICE_STATS permission. This is the minimum permission that required to update network usage with the new system api. Test: atest FrameworksNetTests OffloadControllerTest TetheringTest Bug: 130855321 Change-Id: I9c0c86e20d4797ea3570feed741be3a07f839c7d --- data/etc/privapp-permissions-platform.xml | 1 + packages/Tethering/AndroidManifest.xml | 1 + 2 files changed, 2 insertions(+) diff --git a/data/etc/privapp-permissions-platform.xml b/data/etc/privapp-permissions-platform.xml index 3477aedefacff..a818119f81031 100644 --- a/data/etc/privapp-permissions-platform.xml +++ b/data/etc/privapp-permissions-platform.xml @@ -240,6 +240,7 @@ applications that come with the platform + diff --git a/packages/Tethering/AndroidManifest.xml b/packages/Tethering/AndroidManifest.xml index 5a71eb23abadd..c71d0d7bc543c 100644 --- a/packages/Tethering/AndroidManifest.xml +++ b/packages/Tethering/AndroidManifest.xml @@ -36,6 +36,7 @@ + Date: Tue, 3 Dec 2019 14:34:13 +0800 Subject: [PATCH 4/6] [SP07] Remove reference of NetworkStatsService in IpServer Currently NetworkStatsService is notified when downstream is updated. However, it seems unnecessary given that tether stats is persist since boot, and there is no any upstream change when downstream is changed. Test: atest NetworkStatsServiceTest IpServerTest Bug: 130855321 Change-Id: Ie300bfeb0a04678fcfcf300843b6f859af9df91d --- .../src/android/net/ip/IpServer.java | 12 +---- .../connectivity/tethering/Tethering.java | 2 +- .../unit/src/android/net/ip/IpServerTest.java | 50 ++++++++----------- 3 files changed, 23 insertions(+), 41 deletions(-) diff --git a/packages/Tethering/src/android/net/ip/IpServer.java b/packages/Tethering/src/android/net/ip/IpServer.java index 6ac467e39a9da..4306cf0bd5f30 100644 --- a/packages/Tethering/src/android/net/ip/IpServer.java +++ b/packages/Tethering/src/android/net/ip/IpServer.java @@ -26,7 +26,6 @@ import static android.net.util.TetheringMessageBase.BASE_IPSERVER; import android.net.INetd; import android.net.INetworkStackStatusCallback; -import android.net.INetworkStatsService; import android.net.IpPrefix; import android.net.LinkAddress; import android.net.LinkProperties; @@ -176,7 +175,6 @@ public class IpServer extends StateMachine { private final SharedLog mLog; private final INetd mNetd; - private final INetworkStatsService mStatsService; private final Callback mCallback; private final InterfaceController mInterfaceCtrl; @@ -208,12 +206,10 @@ public class IpServer extends StateMachine { public IpServer( String ifaceName, Looper looper, int interfaceType, SharedLog log, - INetd netd, INetworkStatsService statsService, Callback callback, - boolean usingLegacyDhcp, Dependencies deps) { + INetd netd, Callback callback, boolean usingLegacyDhcp, Dependencies deps) { super(ifaceName, looper); mLog = log.forSubComponent(ifaceName); mNetd = netd; - mStatsService = statsService; mCallback = callback; mInterfaceCtrl = new InterfaceController(ifaceName, mNetd, mLog); mIfaceName = ifaceName; @@ -881,12 +877,6 @@ public class IpServer extends StateMachine { // Sometimes interfaces are gone before we get // to remove their rules, which generates errors. // Just do the best we can. - try { - // About to tear down NAT; gather remaining statistics. - mStatsService.forceUpdate(); - } catch (Exception e) { - mLog.e("Exception in forceUpdate: " + e.toString()); - } try { mNetd.ipfwdRemoveInterfaceForward(mIfaceName, upstreamIface); } catch (RemoteException | ServiceSpecificException e) { diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java index 0ee8164f3b906..b2dd956c101cf 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java @@ -2056,7 +2056,7 @@ public class Tethering { mLog.log("adding TetheringInterfaceStateMachine for: " + iface); final TetherState tetherState = new TetherState( - new IpServer(iface, mLooper, interfaceType, mLog, mNetd, mStatsService, + new IpServer(iface, mLooper, interfaceType, mLog, mNetd, makeControlCallback(), mConfig.enableLegacyDhcpServer, mDeps.getIpServerDependencies())); mTetherStates.put(iface, tetherState); diff --git a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index 65a0ac13a84b5..1f50b6bf7fb32 100644 --- a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -52,7 +52,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.net.INetd; -import android.net.INetworkStatsService; import android.net.InterfaceConfigurationParcel; import android.net.IpPrefix; import android.net.LinkAddress; @@ -99,7 +98,6 @@ public class IpServerTest { private static final int MAKE_DHCPSERVER_TIMEOUT_MS = 1000; @Mock private INetd mNetd; - @Mock private INetworkStatsService mStatsService; @Mock private IpServer.Callback mCallback; @Mock private SharedLog mSharedLog; @Mock private IDhcpServer mDhcpServer; @@ -139,13 +137,13 @@ public class IpServerTest { mInterfaceConfiguration.prefixLength = BLUETOOTH_DHCP_PREFIX_LENGTH; } mIpServer = new IpServer( - IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog, mNetd, mStatsService, + IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog, mNetd, mCallback, usingLegacyDhcp, mDependencies); mIpServer.start(); // Starting the state machine always puts us in a consistent state and notifies // the rest of the world that we've changed from an unknown to available state. mLooper.dispatchAll(); - reset(mNetd, mStatsService, mCallback); + reset(mNetd, mCallback); when(mRaDaemon.start()).thenReturn(true); } @@ -162,7 +160,7 @@ public class IpServerTest { if (upstreamIface != null) { dispatchTetherConnectionChanged(upstreamIface); } - reset(mNetd, mStatsService, mCallback); + reset(mNetd, mCallback); } @Before public void setUp() throws Exception { @@ -173,13 +171,13 @@ public class IpServerTest { @Test public void startsOutAvailable() { mIpServer = new IpServer(IFACE_NAME, mLooper.getLooper(), TETHERING_BLUETOOTH, mSharedLog, - mNetd, mStatsService, mCallback, false /* usingLegacyDhcp */, mDependencies); + mNetd, mCallback, false /* usingLegacyDhcp */, mDependencies); mIpServer.start(); mLooper.dispatchAll(); verify(mCallback).updateInterfaceState( mIpServer, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); verify(mCallback).updateLinkProperties(eq(mIpServer), any(LinkProperties.class)); - verifyNoMoreInteractions(mCallback, mNetd, mStatsService); + verifyNoMoreInteractions(mCallback, mNetd); } @Test @@ -198,7 +196,7 @@ public class IpServerTest { // None of these commands should trigger us to request action from // the rest of the system. dispatchCommand(command); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } } @@ -210,7 +208,7 @@ public class IpServerTest { verify(mCallback).updateInterfaceState( mIpServer, STATE_UNAVAILABLE, TETHER_ERROR_NO_ERROR); verify(mCallback).updateLinkProperties(eq(mIpServer), any(LinkProperties.class)); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -228,7 +226,7 @@ public class IpServerTest { mIpServer, STATE_TETHERED, TETHER_ERROR_NO_ERROR); inOrder.verify(mCallback).updateLinkProperties( eq(mIpServer), any(LinkProperties.class)); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -236,7 +234,7 @@ public class IpServerTest { initTetheredStateMachine(TETHERING_BLUETOOTH, null); dispatchCommand(IpServer.CMD_TETHER_UNREQUESTED); - InOrder inOrder = inOrder(mNetd, mStatsService, mCallback); + InOrder inOrder = inOrder(mNetd, mCallback); inOrder.verify(mNetd).tetherApplyDnsInterfaces(); inOrder.verify(mNetd).tetherInterfaceRemove(IFACE_NAME); inOrder.verify(mNetd).networkRemoveInterface(INetd.LOCAL_NET_ID, IFACE_NAME); @@ -245,7 +243,7 @@ public class IpServerTest { mIpServer, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); inOrder.verify(mCallback).updateLinkProperties( eq(mIpServer), any(LinkProperties.class)); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -265,7 +263,7 @@ public class IpServerTest { inOrder.verify(mCallback).updateLinkProperties( eq(mIpServer), mLinkPropertiesCaptor.capture()); assertIPv4AddressAndDirectlyConnectedRoute(mLinkPropertiesCaptor.getValue()); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -285,7 +283,7 @@ public class IpServerTest { inOrder.verify(mCallback).updateLinkProperties( eq(mIpServer), mLinkPropertiesCaptor.capture()); assertIPv4AddressAndDirectlyConnectedRoute(mLinkPropertiesCaptor.getValue()); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -298,7 +296,7 @@ public class IpServerTest { InOrder inOrder = inOrder(mNetd); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -306,13 +304,12 @@ public class IpServerTest { initTetheredStateMachine(TETHERING_BLUETOOTH, UPSTREAM_IFACE); dispatchTetherConnectionChanged(UPSTREAM_IFACE2); - InOrder inOrder = inOrder(mNetd, mStatsService); - inOrder.verify(mStatsService).forceUpdate(); + InOrder inOrder = inOrder(mNetd); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -322,12 +319,10 @@ public class IpServerTest { doThrow(RemoteException.class).when(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); dispatchTetherConnectionChanged(UPSTREAM_IFACE2); - InOrder inOrder = inOrder(mNetd, mStatsService); - inOrder.verify(mStatsService).forceUpdate(); + InOrder inOrder = inOrder(mNetd); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); - inOrder.verify(mStatsService).forceUpdate(); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE2); } @@ -340,13 +335,11 @@ public class IpServerTest { IFACE_NAME, UPSTREAM_IFACE2); dispatchTetherConnectionChanged(UPSTREAM_IFACE2); - InOrder inOrder = inOrder(mNetd, mStatsService); - inOrder.verify(mStatsService).forceUpdate(); + InOrder inOrder = inOrder(mNetd); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherAddForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).ipfwdAddInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); - inOrder.verify(mStatsService).forceUpdate(); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE2); } @@ -356,8 +349,7 @@ public class IpServerTest { initTetheredStateMachine(TETHERING_BLUETOOTH, UPSTREAM_IFACE); dispatchCommand(IpServer.CMD_TETHER_UNREQUESTED); - InOrder inOrder = inOrder(mNetd, mStatsService, mCallback); - inOrder.verify(mStatsService).forceUpdate(); + InOrder inOrder = inOrder(mNetd, mCallback); inOrder.verify(mNetd).ipfwdRemoveInterfaceForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherRemoveForward(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNetd).tetherApplyDnsInterfaces(); @@ -368,7 +360,7 @@ public class IpServerTest { mIpServer, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); inOrder.verify(mCallback).updateLinkProperties( eq(mIpServer), any(LinkProperties.class)); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } @Test @@ -435,11 +427,11 @@ public class IpServerTest { public void ignoresDuplicateUpstreamNotifications() throws Exception { initTetheredStateMachine(TETHERING_WIFI, UPSTREAM_IFACE); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); for (int i = 0; i < 5; i++) { dispatchTetherConnectionChanged(UPSTREAM_IFACE); - verifyNoMoreInteractions(mNetd, mStatsService, mCallback); + verifyNoMoreInteractions(mNetd, mCallback); } } From 661daf9b9ac5cdd9e88c349cd42a091b62b2425a Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 3 Dec 2019 15:16:14 +0800 Subject: [PATCH 5/6] [SP08] re-calculate interface quota when limit reached Some providers might hit the limit reached event prior to others. Thus, re-calculate and update interface quota for every provider is needed. This change also removes the lock that held by NPMS while requesting NSS force update, which seems unnecessary and might cause potential deadlock in future. Test: atest NetworkStatsServiceTest Test: atest NetworkPolicyManagerServiceTest Bug: 130855321 Change-Id: I84a08ecfb14c61023fa9f1c98af73d3ee90acef6 --- .../net/NetworkPolicyManagerService.java | 25 +++++++++++++------ .../net/NetworkPolicyManagerServiceTest.java | 4 ++- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java index 04b51a93b9943..9cbcd329649b2 100644 --- a/services/core/java/com/android/server/net/NetworkPolicyManagerService.java +++ b/services/core/java/com/android/server/net/NetworkPolicyManagerService.java @@ -4547,7 +4547,11 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } case MSG_STATS_PROVIDER_LIMIT_REACHED: { mNetworkStats.forceUpdate(); + synchronized (mNetworkPoliciesSecondLock) { + // Some providers might hit the limit reached event prior to others. Thus, + // re-calculate and update interface quota for every provider is needed. + updateNetworkRulesNL(); updateNetworkEnabledNL(); updateNotificationsNL(); } @@ -4555,16 +4559,21 @@ public class NetworkPolicyManagerService extends INetworkPolicyManager.Stub { } case MSG_LIMIT_REACHED: { final String iface = (String) msg.obj; + synchronized (mNetworkPoliciesSecondLock) { + // fast return if not needed. + if (!mMeteredIfaces.contains(iface)) { + return true; + } + } + + // force stats update to make sure the service have the numbers that caused + // alert to trigger. + mNetworkStats.forceUpdate(); synchronized (mNetworkPoliciesSecondLock) { - if (mMeteredIfaces.contains(iface)) { - // force stats update to make sure we have - // numbers that caused alert to trigger. - mNetworkStats.forceUpdate(); - - updateNetworkEnabledNL(); - updateNotificationsNL(); - } + updateNetworkRulesNL(); + updateNetworkEnabledNL(); + updateNotificationsNL(); } return true; } diff --git a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java index 2c941c604f3f9..70683085bb65d 100644 --- a/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/net/NetworkPolicyManagerServiceTest.java @@ -1729,9 +1729,11 @@ public class NetworkPolicyManagerServiceTest { .getService(NetworkPolicyManagerInternal.class); npmi.onStatsProviderLimitReached("TEST"); - // Verifies that the limit reached leads to a force update. + // Verifies that the limit reached leads to a force update and new limit should be set. postMsgAndWaitForCompletion(); verify(mStatsService).forceUpdate(); + postMsgAndWaitForCompletion(); + verify(mStatsService).setStatsProviderLimit(TEST_IFACE, 10000L - 4999L - 1999L); } /** From caa810011c007265305a540754bb8c58694b1181 Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 11 Dec 2019 11:03:01 +0800 Subject: [PATCH 6/6] [SP09] Remove NetworkStatsService @hide usage in Tethering Test: atest TetheringTest Bug: 130855321 Change-Id: I0260524c57f7c0aa738eaff07f9b0ae8e785ed63 --- .../server/connectivity/tethering/Tethering.java | 6 ++---- .../tethering/TetheringDependencies.java | 10 ---------- .../server/connectivity/tethering/TetheringTest.java | 12 ++++-------- 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java index b2dd956c101cf..301f805d3d03b 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java @@ -66,7 +66,6 @@ import android.content.res.Resources; import android.hardware.usb.UsbManager; import android.net.INetd; import android.net.INetworkPolicyManager; -import android.net.INetworkStatsService; import android.net.ITetheringEventCallback; import android.net.IpPrefix; import android.net.LinkAddress; @@ -177,7 +176,6 @@ public class Tethering { private final Context mContext; private final ArrayMap mTetherStates; private final BroadcastReceiver mStateReceiver; - private final INetworkStatsService mStatsService; private final INetworkPolicyManager mPolicyManager; private final Looper mLooper; private final StateMachine mTetherMasterSM; @@ -213,7 +211,6 @@ public class Tethering { mLog.mark("Tethering.constructed"); mDeps = deps; mContext = mDeps.getContext(); - mStatsService = mDeps.getINetworkStatsService(); mPolicyManager = mDeps.getINetworkPolicyManager(); mNetd = mDeps.getINetd(mContext); mLooper = mDeps.getTetheringLooper(); @@ -225,7 +222,8 @@ public class Tethering { mTetherMasterSM = new TetherMasterSM("TetherMaster", mLooper, deps); mTetherMasterSM.start(); - final NetworkStatsManager statsManager = new NetworkStatsManager(mContext, mStatsService); + final NetworkStatsManager statsManager = + (NetworkStatsManager) mContext.getSystemService(Context.NETWORK_STATS_SERVICE); mHandler = mTetherMasterSM.getHandler(); mOffloadController = new OffloadController(mHandler, mDeps.getOffloadHardwareInterface(mHandler, mLog), mContext.getContentResolver(), diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringDependencies.java b/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringDependencies.java index b16b3294a1125..79e99e8023edb 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringDependencies.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/TetheringDependencies.java @@ -19,7 +19,6 @@ package com.android.server.connectivity.tethering; import android.content.Context; import android.net.INetd; import android.net.INetworkPolicyManager; -import android.net.INetworkStatsService; import android.net.NetworkRequest; import android.net.ip.IpServer; import android.net.util.SharedLog; @@ -106,15 +105,6 @@ public abstract class TetheringDependencies { ServiceManager.getService(Context.NETWORKMANAGEMENT_SERVICE)); } - /** - * Get a reference to INetworkStatsService to force update tethering usage. - * Note: This should be removed in R development cycle. - */ - public INetworkStatsService getINetworkStatsService() { - return INetworkStatsService.Stub.asInterface( - ServiceManager.getService(Context.NETWORK_STATS_SERVICE)); - } - /** * Get a reference to INetworkPolicyManager to be used by tethering. */ diff --git a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java index 00afb97120973..eaac159a87d8f 100644 --- a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java +++ b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java @@ -60,6 +60,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import android.app.usage.NetworkStatsManager; import android.content.BroadcastReceiver; import android.content.ContentResolver; import android.content.Context; @@ -70,7 +71,6 @@ import android.content.res.Resources; import android.hardware.usb.UsbManager; import android.net.INetd; import android.net.INetworkPolicyManager; -import android.net.INetworkStatsService; import android.net.ITetheringEventCallback; import android.net.InetAddresses; import android.net.InterfaceConfigurationParcel; @@ -152,7 +152,7 @@ public class TetheringTest { @Mock private ApplicationInfo mApplicationInfo; @Mock private Context mContext; @Mock private INetworkManagementService mNMService; - @Mock private INetworkStatsService mStatsService; + @Mock private NetworkStatsManager mStatsManager; @Mock private INetworkPolicyManager mPolicyManager; @Mock private OffloadHardwareInterface mOffloadHardwareInterface; @Mock private Resources mResources; @@ -217,6 +217,7 @@ public class TetheringTest { if (Context.USB_SERVICE.equals(name)) return mUsbManager; if (Context.TELEPHONY_SERVICE.equals(name)) return mTelephonyManager; if (Context.USER_SERVICE.equals(name)) return mUserManager; + if (Context.NETWORK_STATS_SERVICE.equals(name)) return mStatsManager; return super.getSystemService(name); } @@ -338,11 +339,6 @@ public class TetheringTest { return mNMService; } - @Override - public INetworkStatsService getINetworkStatsService() { - return mStatsService; - } - @Override public INetworkPolicyManager getINetworkPolicyManager() { return mPolicyManager; @@ -457,7 +453,7 @@ public class TetheringTest { mServiceContext.registerReceiver(mBroadcastReceiver, new IntentFilter(ACTION_TETHER_STATE_CHANGED)); mTethering = makeTethering(); - verify(mStatsService, times(1)).registerNetworkStatsProvider(anyString(), any()); + verify(mStatsManager, times(1)).registerNetworkStatsProvider(anyString(), any()); verify(mNetd).registerUnsolicitedEventListener(any()); final ArgumentCaptor phoneListenerCaptor = ArgumentCaptor.forClass(PhoneStateListener.class);