Address review comments at aosp/3249945
This change includes:
1. Rename binder interfaces.
2. Use a dedicated class to pass the result of
get{Total|Iface|Uid}Stats, as it is 25% faster than passing
a NetworkStats object through the binder interface.
3. Remove the binder interface only used by the test.
4. Remove NetworkStatsBinderTest on R and S, since the binder
interface is removed, the mainlined service does not run on
those versions and the chance of breaking these tests on
R and S is low.
5. Remove useless TODO comments.
Test: atest FrameworksNetTests:android.net.connectivity.com.android.server.net.NetworkStatsServiceTest \
CtsNetTestCases:android.net.cts.NetworkStatsBinderTest \
FrameworksNetIntegrationTests:com.android.server.net.integrationtests.NetworkStatsIntegrationTest
Bug: 372851500
Bug: 343260158
Change-Id: Ibd79a17218f54f0117d8ef5d7b9ec6073356f923
diff --git a/framework-t/src/android/net/INetworkStatsService.aidl b/framework-t/src/android/net/INetworkStatsService.aidl
index 01ac106..ce57a4a 100644
--- a/framework-t/src/android/net/INetworkStatsService.aidl
+++ b/framework-t/src/android/net/INetworkStatsService.aidl
@@ -21,10 +21,10 @@
import android.net.Network;
import android.net.NetworkStateSnapshot;
import android.net.NetworkStats;
-import android.net.NetworkStatsHistory;
import android.net.NetworkTemplate;
import android.net.UnderlyingNetworkInfo;
import android.net.netstats.IUsageCallback;
+import android.net.netstats.StatsResult;
import android.net.netstats.provider.INetworkStatsProvider;
import android.net.netstats.provider.INetworkStatsProviderCallback;
import android.os.IBinder;
@@ -78,16 +78,13 @@
void unregisterUsageRequest(in DataUsageRequest request);
/** Get the uid stats information since boot */
- NetworkStats getTypelessUidStats(int uid);
+ StatsResult getUidStats(int uid);
/** Get the iface stats information since boot */
- NetworkStats getTypelessIfaceStats(String iface);
+ StatsResult getIfaceStats(String iface);
/** Get the total network stats information since boot */
- NetworkStats getTypelessTotalStats();
-
- /** Get the uid stats information (with specified type) since boot */
- long getUidStats(int uid, int type);
+ StatsResult getTotalStats();
/** Registers a network stats provider */
INetworkStatsProviderCallback registerNetworkStatsProvider(String tag,
diff --git a/framework-t/src/android/net/TrafficStats.java b/framework-t/src/android/net/TrafficStats.java
index 3b6a69b..ab0aaa0 100644
--- a/framework-t/src/android/net/TrafficStats.java
+++ b/framework-t/src/android/net/TrafficStats.java
@@ -19,6 +19,7 @@
import static android.annotation.SystemApi.Client.MODULE_LIBRARIES;
import android.annotation.NonNull;
+import android.annotation.Nullable;
import android.annotation.RequiresPermission;
import android.annotation.SuppressLint;
import android.annotation.SystemApi;
@@ -29,6 +30,7 @@
import android.compat.annotation.UnsupportedAppUsage;
import android.content.Context;
import android.media.MediaPlayer;
+import android.net.netstats.StatsResult;
import android.os.Binder;
import android.os.Build;
import android.os.RemoteException;
@@ -40,8 +42,6 @@
import java.net.DatagramSocket;
import java.net.Socket;
import java.net.SocketException;
-import java.util.Iterator;
-import java.util.Objects;
/**
@@ -963,13 +963,13 @@
|| android.os.Process.myUid() != uid) {
return UNSUPPORTED;
}
- final NetworkStats stats;
+ final StatsResult stats;
try {
- stats = getStatsService().getTypelessUidStats(uid);
+ stats = getStatsService().getUidStats(uid);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
- return getValueForTypeFromFirstEntry(stats, type);
+ return getEntryValueForType(stats, type);
}
/** @hide */
@@ -977,13 +977,13 @@
if (!isEntryValueTypeValid(type)) {
return UNSUPPORTED;
}
- final NetworkStats stats;
+ final StatsResult stats;
try {
- stats = getStatsService().getTypelessTotalStats();
+ stats = getStatsService().getTotalStats();
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
- return getValueForTypeFromFirstEntry(stats, type);
+ return getEntryValueForType(stats, type);
}
/** @hide */
@@ -991,13 +991,13 @@
if (!isEntryValueTypeValid(type)) {
return UNSUPPORTED;
}
- final NetworkStats stats;
+ final StatsResult stats;
try {
- stats = getStatsService().getTypelessIfaceStats(iface);
+ stats = getStatsService().getIfaceStats(iface);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
- return getValueForTypeFromFirstEntry(stats, type);
+ return getEntryValueForType(stats, type);
}
/**
@@ -1127,18 +1127,18 @@
public static final int TYPE_TX_PACKETS = 3;
/** @hide */
- private static long getEntryValueForType(@NonNull NetworkStats.Entry entry, int type) {
- Objects.requireNonNull(entry);
+ private static long getEntryValueForType(@Nullable StatsResult stats, int type) {
+ if (stats == null) return UNSUPPORTED;
if (!isEntryValueTypeValid(type)) return UNSUPPORTED;
switch (type) {
case TYPE_RX_BYTES:
- return entry.getRxBytes();
+ return stats.rxBytes;
case TYPE_RX_PACKETS:
- return entry.getRxPackets();
+ return stats.rxPackets;
case TYPE_TX_BYTES:
- return entry.getTxBytes();
+ return stats.txBytes;
case TYPE_TX_PACKETS:
- return entry.getTxPackets();
+ return stats.txPackets;
default:
throw new IllegalStateException("Bug: Invalid type: "
+ type + " should not reach here.");
@@ -1157,13 +1157,5 @@
return false;
}
}
-
- /** @hide */
- public static long getValueForTypeFromFirstEntry(@NonNull NetworkStats stats, int type) {
- Objects.requireNonNull(stats);
- Iterator<NetworkStats.Entry> iter = stats.iterator();
- if (!iter.hasNext()) return UNSUPPORTED;
- return getEntryValueForType(iter.next(), type);
- }
}
diff --git a/framework-t/src/android/net/netstats/StatsResult.aidl b/framework-t/src/android/net/netstats/StatsResult.aidl
new file mode 100644
index 0000000..3f09566
--- /dev/null
+++ b/framework-t/src/android/net/netstats/StatsResult.aidl
@@ -0,0 +1,31 @@
+/**
+ * Copyright (c) 2024, The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.net.netstats;
+
+/**
+ * A lightweight class to pass result of TrafficStats#get{Total|Iface|Uid}Stats.
+ *
+ * @hide
+ */
+@JavaDerive(equals=true, toString=true)
+@JavaOnlyImmutable
+parcelable StatsResult {
+ long rxBytes;
+ long rxPackets;
+ long txBytes;
+ long txPackets;
+}
\ No newline at end of file
diff --git a/service-t/src/com/android/server/net/NetworkStatsService.java b/service-t/src/com/android/server/net/NetworkStatsService.java
index 294a85a..7572f0e 100644
--- a/service-t/src/com/android/server/net/NetworkStatsService.java
+++ b/service-t/src/com/android/server/net/NetworkStatsService.java
@@ -52,7 +52,6 @@
import static android.net.TrafficStats.KB_IN_BYTES;
import static android.net.TrafficStats.MB_IN_BYTES;
import static android.net.TrafficStats.UID_TETHERING;
-import static android.net.TrafficStats.getValueForTypeFromFirstEntry;
import static android.net.connectivity.ConnectivityCompatChanges.ENABLE_TRAFFICSTATS_RATE_LIMIT_CACHE;
import static android.net.netstats.NetworkStatsDataMigrationUtils.PREFIX_UID;
import static android.net.netstats.NetworkStatsDataMigrationUtils.PREFIX_UID_TAG;
@@ -127,6 +126,7 @@
import android.net.Uri;
import android.net.netstats.IUsageCallback;
import android.net.netstats.NetworkStatsDataMigrationUtils;
+import android.net.netstats.StatsResult;
import android.net.netstats.provider.INetworkStatsProvider;
import android.net.netstats.provider.INetworkStatsProviderCallback;
import android.net.netstats.provider.NetworkStatsProvider;
@@ -2133,18 +2133,12 @@
}
}
+ @Nullable
@Override
- public long getUidStats(int uid, int type) {
- return getValueForTypeFromFirstEntry(getTypelessUidStats(uid), type);
- }
-
- @NonNull
- @Override
- public NetworkStats getTypelessUidStats(int uid) {
- final NetworkStats stats = new NetworkStats(0, 0);
+ public StatsResult getUidStats(int uid) {
final int callingUid = Binder.getCallingUid();
if (callingUid != android.os.Process.SYSTEM_UID && callingUid != uid) {
- return stats;
+ return null;
}
final NetworkStats.Entry entry;
if (mAlwaysUseTrafficStatsServiceRateLimitCache
@@ -2153,10 +2147,7 @@
() -> mDeps.nativeGetUidStat(uid));
} else entry = mDeps.nativeGetUidStat(uid);
- if (entry != null) {
- stats.insertEntry(entry);
- }
- return stats;
+ return getStatsResultFromEntryOrNull(entry);
}
@Nullable
@@ -2173,9 +2164,9 @@
return entry;
}
- @NonNull
+ @Nullable
@Override
- public NetworkStats getTypelessIfaceStats(@NonNull String iface) {
+ public StatsResult getIfaceStats(@NonNull String iface) {
Objects.requireNonNull(iface);
final NetworkStats.Entry entry;
@@ -2186,11 +2177,13 @@
() -> getIfaceStatsInternal(iface));
} else entry = getIfaceStatsInternal(iface);
- NetworkStats stats = new NetworkStats(0, 0);
- if (entry != null) {
- stats.insertEntry(entry);
- }
- return stats;
+ return getStatsResultFromEntryOrNull(entry);
+ }
+
+ @Nullable
+ private StatsResult getStatsResultFromEntryOrNull(@Nullable NetworkStats.Entry entry) {
+ if (entry == null) return null;
+ return new StatsResult(entry.rxBytes, entry.rxPackets, entry.txBytes, entry.txPackets);
}
@Nullable
@@ -2203,9 +2196,9 @@
return entry;
}
- @NonNull
+ @Nullable
@Override
- public NetworkStats getTypelessTotalStats() {
+ public StatsResult getTotalStats() {
final NetworkStats.Entry entry;
if (mAlwaysUseTrafficStatsServiceRateLimitCache
|| mDeps.isChangeEnabled(
@@ -2214,11 +2207,7 @@
IFACE_ALL, UID_ALL, () -> getTotalStatsInternal());
} else entry = getTotalStatsInternal();
- final NetworkStats stats = new NetworkStats(0, 0);
- if (entry != null) {
- stats.insertEntry(entry);
- }
- return stats;
+ return getStatsResultFromEntryOrNull(entry);
}
@Override
diff --git a/tests/cts/net/src/android/net/cts/NetworkStatsBinderTest.java b/tests/cts/net/src/android/net/cts/NetworkStatsBinderTest.java
index 1a48983..10adee0 100644
--- a/tests/cts/net/src/android/net/cts/NetworkStatsBinderTest.java
+++ b/tests/cts/net/src/android/net/cts/NetworkStatsBinderTest.java
@@ -19,28 +19,28 @@
import static android.os.Process.INVALID_UID;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
import android.annotation.NonNull;
+import android.annotation.Nullable;
import android.content.Context;
import android.content.pm.PackageInfo;
import android.content.pm.PackageManager;
import android.net.INetworkStatsService;
import android.net.TrafficStats;
+import android.net.connectivity.android.net.netstats.StatsResult;
import android.os.Build;
import android.os.IBinder;
import android.os.Process;
import android.os.RemoteException;
-import android.test.AndroidTestCase;
-import android.util.SparseArray;
import androidx.test.InstrumentationRegistry;
-import androidx.test.runner.AndroidJUnit4;
import com.android.internal.util.CollectionUtils;
+import com.android.testutils.ConnectivityModuleTest;
import com.android.testutils.DevSdkIgnoreRule;
+import com.android.testutils.DevSdkIgnoreRunner;
-import org.junit.Before;
-import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -48,37 +48,20 @@
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.List;
-import java.util.function.Function;
import java.util.function.Predicate;
-@RunWith(AndroidJUnit4.class)
+@ConnectivityModuleTest
+@DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.S_V2) // Mainline NetworkStats starts from T.
+@RunWith(DevSdkIgnoreRunner.class)
public class NetworkStatsBinderTest {
- // NOTE: These are shamelessly copied from TrafficStats.
- private static final int TYPE_RX_BYTES = 0;
- private static final int TYPE_RX_PACKETS = 1;
- private static final int TYPE_TX_BYTES = 2;
- private static final int TYPE_TX_PACKETS = 3;
-
- @Rule
- public DevSdkIgnoreRule mIgnoreRule = new DevSdkIgnoreRule(
- Build.VERSION_CODES.Q /* ignoreClassUpTo */);
-
- private final SparseArray<Function<Integer, Long>> mUidStatsQueryOpArray = new SparseArray<>();
-
- @Before
- public void setUp() throws Exception {
- mUidStatsQueryOpArray.put(TYPE_RX_BYTES, uid -> TrafficStats.getUidRxBytes(uid));
- mUidStatsQueryOpArray.put(TYPE_RX_PACKETS, uid -> TrafficStats.getUidRxPackets(uid));
- mUidStatsQueryOpArray.put(TYPE_TX_BYTES, uid -> TrafficStats.getUidTxBytes(uid));
- mUidStatsQueryOpArray.put(TYPE_TX_PACKETS, uid -> TrafficStats.getUidTxPackets(uid));
- }
-
- private long getUidStatsFromBinder(int uid, int type) throws Exception {
- Method getServiceMethod = Class.forName("android.os.ServiceManager")
+ @Nullable
+ private StatsResult getUidStatsFromBinder(int uid) throws Exception {
+ final Method getServiceMethod = Class.forName("android.os.ServiceManager")
.getDeclaredMethod("getService", new Class[]{String.class});
- IBinder binder = (IBinder) getServiceMethod.invoke(null, Context.NETWORK_STATS_SERVICE);
- INetworkStatsService nss = INetworkStatsService.Stub.asInterface(binder);
- return nss.getUidStats(uid, type);
+ final IBinder binder = (IBinder) getServiceMethod.invoke(
+ null, Context.NETWORK_STATS_SERVICE);
+ final INetworkStatsService nss = INetworkStatsService.Stub.asInterface(binder);
+ return nss.getUidStats(uid);
}
private int getFirstAppUidThat(@NonNull Predicate<Integer> predicate) {
@@ -108,38 +91,34 @@
if (notMyUid != INVALID_UID) testUidList.add(notMyUid);
for (final int uid : testUidList) {
- for (int i = 0; i < mUidStatsQueryOpArray.size(); i++) {
- final int type = mUidStatsQueryOpArray.keyAt(i);
- try {
- final long uidStatsFromBinder = getUidStatsFromBinder(uid, type);
- final long uidTrafficStats = mUidStatsQueryOpArray.get(type).apply(uid);
+ try {
+ final StatsResult uidStatsFromBinder = getUidStatsFromBinder(uid);
- // Verify that UNSUPPORTED is returned if the uid is not current app uid.
- if (uid != myUid) {
- assertEquals(uidStatsFromBinder, TrafficStats.UNSUPPORTED);
- }
+ if (uid != myUid) {
+ // Verify that null is returned if the uid is not current app uid.
+ assertNull(uidStatsFromBinder);
+ } else {
// Verify that returned result is the same with the result get from
// TrafficStats.
- // TODO: If the test is flaky then it should instead assert that the values
- // are approximately similar.
- assertEquals("uidStats is not matched for query type " + type
- + ", uid=" + uid + ", myUid=" + myUid, uidTrafficStats,
- uidStatsFromBinder);
- } catch (IllegalAccessException e) {
- /* Java language access prevents exploitation. */
- return;
- } catch (InvocationTargetException e) {
- /* Underlying method has been changed. */
- return;
- } catch (ClassNotFoundException e) {
- /* not vulnerable if hidden API no longer available */
- return;
- } catch (NoSuchMethodException e) {
- /* not vulnerable if hidden API no longer available */
- return;
- } catch (RemoteException e) {
- return;
+ assertEquals(uidStatsFromBinder.rxBytes, TrafficStats.getUidRxBytes(uid));
+ assertEquals(uidStatsFromBinder.rxPackets, TrafficStats.getUidRxPackets(uid));
+ assertEquals(uidStatsFromBinder.txBytes, TrafficStats.getUidTxBytes(uid));
+ assertEquals(uidStatsFromBinder.txPackets, TrafficStats.getUidTxPackets(uid));
}
+ } catch (IllegalAccessException e) {
+ /* Java language access prevents exploitation. */
+ return;
+ } catch (InvocationTargetException e) {
+ /* Underlying method has been changed. */
+ return;
+ } catch (ClassNotFoundException e) {
+ /* not vulnerable if hidden API no longer available */
+ return;
+ } catch (NoSuchMethodException e) {
+ /* not vulnerable if hidden API no longer available */
+ return;
+ } catch (RemoteException e) {
+ return;
}
}
}
diff --git a/tests/unit/java/android/net/TrafficStatsTest.kt b/tests/unit/java/android/net/TrafficStatsTest.kt
deleted file mode 100644
index c61541e..0000000
--- a/tests/unit/java/android/net/TrafficStatsTest.kt
+++ /dev/null
@@ -1,46 +0,0 @@
-/*
-* Copyright (C) 2024 The Android Open Source Project
-*
-* Licensed under the Apache License, Version 2.0 (the "License");
-* you may not use this file except in compliance with the License.
-* You may obtain a copy of the License at
-*
-* http://www.apache.org/licenses/LICENSE-2.0
-*
-* Unless required by applicable law or agreed to in writing, software
-* distributed under the License is distributed on an "AS IS" BASIS,
-* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-* See the License for the specific language governing permissions and
-* limitations under the License.
-*/
-
-package android.net
-
-import android.net.TrafficStats.getValueForTypeFromFirstEntry
-import android.net.TrafficStats.TYPE_RX_BYTES
-import android.net.TrafficStats.UNSUPPORTED
-import android.os.Build
-import com.android.testutils.DevSdkIgnoreRule
-import com.android.testutils.DevSdkIgnoreRunner
-import org.junit.Test
-import org.junit.runner.RunWith
-import kotlin.test.assertEquals
-
-private const val TEST_IFACE1 = "test_iface1"
-
-@RunWith(DevSdkIgnoreRunner::class)
-@DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.TIRAMISU)
-class TrafficStatsTest {
-
- @Test
- fun testGetValueForTypeFromFirstEntry() {
- var stats: NetworkStats = NetworkStats(0, 0)
- // empty stats
- assertEquals(getValueForTypeFromFirstEntry(stats, TYPE_RX_BYTES), UNSUPPORTED.toLong())
- // invalid type
- stats.insertEntry(TEST_IFACE1, 1, 2, 3, 4)
- assertEquals(getValueForTypeFromFirstEntry(stats, 1000), UNSUPPORTED.toLong())
- // valid type
- assertEquals(getValueForTypeFromFirstEntry(stats, TYPE_RX_BYTES), 1)
- }
-}
\ No newline at end of file
diff --git a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
index ef4c44d..e6f8d21 100644
--- a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
+++ b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java
@@ -56,7 +56,6 @@
import static android.net.TrafficStats.MB_IN_BYTES;
import static android.net.TrafficStats.UID_REMOVED;
import static android.net.TrafficStats.UID_TETHERING;
-import static android.net.TrafficStats.getValueForTypeFromFirstEntry;
import static android.net.connectivity.ConnectivityCompatChanges.ENABLE_TRAFFICSTATS_RATE_LIMIT_CACHE;
import static android.net.netstats.NetworkStatsDataMigrationUtils.PREFIX_UID;
import static android.net.netstats.NetworkStatsDataMigrationUtils.PREFIX_UID_TAG;
@@ -128,8 +127,8 @@
import android.net.TestNetworkSpecifier;
import android.net.TetherStatsParcel;
import android.net.TetheringManager;
-import android.net.TrafficStats;
import android.net.UnderlyingNetworkInfo;
+import android.net.netstats.StatsResult;
import android.net.netstats.provider.INetworkStatsProviderCallback;
import android.net.wifi.WifiInfo;
import android.os.DropBoxManager;
@@ -209,7 +208,6 @@
import java.util.Set;
import java.util.concurrent.Executor;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.function.Function;
/**
* Tests for {@link NetworkStatsService}.
@@ -2515,22 +2513,18 @@
// Assert for 3 different API return values respectively.
private void assertTrafficStatsValues(String iface, int uid, long rxBytes, long rxPackets,
long txBytes, long txPackets) {
- assertTrafficStatsValuesThat(rxBytes, rxPackets, txBytes, txPackets,
- (type) -> getValueForTypeFromFirstEntry(mService.getTypelessTotalStats(), type));
- assertTrafficStatsValuesThat(rxBytes, rxPackets, txBytes, txPackets,
- (type) -> getValueForTypeFromFirstEntry(
- mService.getTypelessIfaceStats(iface), type)
- );
- assertTrafficStatsValuesThat(rxBytes, rxPackets, txBytes, txPackets,
- (type) -> getValueForTypeFromFirstEntry(mService.getTypelessUidStats(uid), type));
+ assertStatsResultEquals(mService.getTotalStats(), rxBytes, rxPackets, txBytes, txPackets);
+ assertStatsResultEquals(mService.getIfaceStats(iface), rxBytes, rxPackets, txBytes,
+ txPackets);
+ assertStatsResultEquals(mService.getUidStats(uid), rxBytes, rxPackets, txBytes, txPackets);
}
- private void assertTrafficStatsValuesThat(long rxBytes, long rxPackets, long txBytes,
- long txPackets, Function<Integer, Long> fetcher) {
- assertEquals(rxBytes, (long) fetcher.apply(TrafficStats.TYPE_RX_BYTES));
- assertEquals(rxPackets, (long) fetcher.apply(TrafficStats.TYPE_RX_PACKETS));
- assertEquals(txBytes, (long) fetcher.apply(TrafficStats.TYPE_TX_BYTES));
- assertEquals(txPackets, (long) fetcher.apply(TrafficStats.TYPE_TX_PACKETS));
+ private void assertStatsResultEquals(StatsResult stats, long rxBytes, long rxPackets,
+ long txBytes, long txPackets) {
+ assertEquals(rxBytes, stats.rxBytes);
+ assertEquals(rxPackets, stats.rxPackets);
+ assertEquals(txBytes, stats.txBytes);
+ assertEquals(txPackets, stats.txPackets);
}
private void assertShouldRunComparison(boolean expected, boolean isDebuggable) {