From f0e3d9b1d7151aa978c5d800d7138875ac11d967 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Wed, 30 Apr 2025 10:34:30 +0800 Subject: [PATCH 1/2] Deduplicate fallback UID callbacks Currently, onUserAdded calls onRoleSmsChanged, which triggers a fallback UIDs callback. Updating opt-in UIDs also triggers a callback. The callback triggered by onRoleSmsChanged actually not necessary and should be removed. Flag: EXEMPT flagged with a kill switch Test: atest ConnectivityCoverageTests:android.net.connectivity.com.android.server.connectivity.SatelliteAccessControllerTest \ --update-device Bug: 410731080 Change-Id: Ifc6aa8d6b798cca2bfc575f986a012c52ea75667 --- .../SatelliteAccessController.java | 36 ++++++++++--------- .../SatelliteAccessControllerTest.kt | 18 ++++++++++ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/service/src/com/android/server/connectivity/SatelliteAccessController.java b/service/src/com/android/server/connectivity/SatelliteAccessController.java index 5634984233..1ae41011c3 100644 --- a/service/src/com/android/server/connectivity/SatelliteAccessController.java +++ b/service/src/com/android/server/connectivity/SatelliteAccessController.java @@ -91,9 +91,9 @@ public class SatelliteAccessController { implements OnRoleHoldersChangedListener { @Override public void onRoleHoldersChanged(String role, UserHandle userHandle) { - if (RoleManager.ROLE_SMS.equals(role)) { + if (RoleManager.ROLE_SMS.equals(role) && updateSatelliteRoleSmsUids(userHandle)) { Log.i(TAG, "ROLE_SMS Change detected "); - updateSatelliteRoleSmsUids(userHandle); + reportSatelliteNetworkFallbackUids(); } } @@ -195,11 +195,12 @@ public class SatelliteAccessController { return INVALID_UID; } - private void updateSatelliteRoleSmsUids(@NonNull UserHandle userHandle) { + @CheckReturnValue + private boolean updateSatelliteRoleSmsUids(@NonNull UserHandle userHandle) { int userId = userHandle.getIdentifier(); if (userId == INVALID_UID) { Log.wtf(TAG, "Invalid User Id"); - return; + return false; } //Returns empty list if no package exists @@ -214,13 +215,11 @@ public class SatelliteAccessController { // on Role change, update the multilayer request at ConnectivityService with updated // satellite network role-sms uid cache list of multiple users as applicable if (newUidsForUser.equals(prevUidsForUser)) { - return; + return false; } mSatelliteRoleSmsUids.put(userId, newUidsForUser); - - // Update all users fallback cache for user, send cs fallback to update ML request - reportSatelliteNetworkFallbackUids(); + return true; } private void reportSatelliteNetworkFallbackUids() { @@ -267,20 +266,25 @@ public class SatelliteAccessController { public void onUserAddedWithInstalledPackageList(@NonNull UserHandle userHandle, @NonNull List apps) { // Obtain uids with role sms and satellite communication permission for the added user. - updateSatelliteRoleSmsUids(userHandle); + final boolean roleSmsUidsChanged = updateSatelliteRoleSmsUids(userHandle); // Store PackageManager for user for later use. final PackageManager pmForUser = mContext.createContextAsUser(userHandle, 0 /* flag */).getPackageManager(); mUserPackageManagers.put(userHandle, pmForUser); - if (!mSupportConstrainedDataSatelliteOptIn) return; - - final Set satelliteDataOptInUidsForUser = getSatelliteDataOptInUidsForUser(apps); - Log.i(TAG, "Add SatelliteDataOptInUids for user " + userHandle + ": " - + satelliteDataOptInUidsForUser); - if (satelliteDataOptInUidsForUser.size() > 0) { - mSatelliteDataOptInUids.addAll(satelliteDataOptInUidsForUser); + boolean optInUidsChanged = false; + if (mSupportConstrainedDataSatelliteOptIn) { + final Set satelliteDataOptInUidsForUser = + getSatelliteDataOptInUidsForUser(apps); + if (satelliteDataOptInUidsForUser.size() > 0) { + Log.i(TAG, "Add SatelliteDataOptInUids for user " + userHandle + ": " + + satelliteDataOptInUidsForUser); + mSatelliteDataOptInUids.addAll(satelliteDataOptInUidsForUser); + optInUidsChanged = true; + } + } + if (roleSmsUidsChanged || optInUidsChanged) { reportSatelliteNetworkFallbackUids(); } } diff --git a/tests/unit/java/com/android/server/connectivity/SatelliteAccessControllerTest.kt b/tests/unit/java/com/android/server/connectivity/SatelliteAccessControllerTest.kt index 90cda29b9c..4c157d4370 100644 --- a/tests/unit/java/com/android/server/connectivity/SatelliteAccessControllerTest.kt +++ b/tests/unit/java/com/android/server/connectivity/SatelliteAccessControllerTest.kt @@ -454,6 +454,24 @@ class SatelliteAccessControllerTest { verify(callback).accept(setOf(SMS_APP_ID1.toUid(PRIMARY_USER)), setOf(TEST_UID1)) } + @FeatureFlag(name = CONSTRAINED_DATA_SATELLITE_OPTIN) + @Test + fun testSatelliteOptInUids_onUserAddedWithRoleSmsUids() { + mockIsSatelliteDataOptimizedApp(TEST_PACKAGE1, true) + val packageInfo1 = makePackageInfo(TEST_PACKAGE1, TEST_UID1) + doReturn(listOf(SMS_APP1)) + .`when`(deps).getRoleHoldersAsUser(RoleManager.ROLE_SMS, PRIMARY_USER_HANDLE) + + val inOrder = inOrder(callback) + satelliteAccessController + .onUserAddedWithInstalledPackageList(PRIMARY_USER_HANDLE, listOf(packageInfo1)) + // Verify the callback only fired once after both lists are ready. + inOrder.verify(callback, never()) + .accept(setOf(SMS_APP_ID1.toUid(PRIMARY_USER)), emptySet()) + inOrder.verify(callback).accept(setOf(SMS_APP_ID1.toUid(PRIMARY_USER)), setOf(TEST_UID1)) + inOrder.verifyNoMoreInteractions() + } + @FeatureFlag(name = CONSTRAINED_DATA_SATELLITE_OPTIN) @Test fun testSatelliteOptInUids_withRoleSmsUids_overlappedUid() { From a79574a9e5843868aa48404e955f38f7fc269c50 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Tue, 29 Apr 2025 15:02:36 +0800 Subject: [PATCH 2/2] Support SharedLog for SatelliteAccessController This is needed because user/package changed events occur rarely and might not be included in bugreports when satellite network problems occur. This change also prints the feature flag in the dumpsys and removing some logs which are redundant and not useful. Sample output: SatelliteAccessController: SupportConstrainedDataSatelliteOptIn: true Role-Sms Uids: {0={10138}} Opt-In Uids: {} Log: 2025-04-29T15:22:50.816963 - SmsRoleUids: {10138} Opt-InUids:{} 2025-04-29T15:22:49.164469 - SmsRoleUids: {10138} Opt-InUids:{10260} 2025-04-29T15:22:24.392085 - SmsRoleUids: {10138} Opt-InUids:{} Flag: EXEMPT flagged with a kill switch Test: atest ConnectivityCoverageTests:android.net.connectivity.com.android.server.connectivity.SatelliteAccessControllerTest \ --update-device Bug: 410731080 Change-Id: Ic80aff756a0552e7d0e26fb906460c31e67a960e --- .../SatelliteAccessController.java | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/service/src/com/android/server/connectivity/SatelliteAccessController.java b/service/src/com/android/server/connectivity/SatelliteAccessController.java index 1ae41011c3..3055b27eab 100644 --- a/service/src/com/android/server/connectivity/SatelliteAccessController.java +++ b/service/src/com/android/server/connectivity/SatelliteAccessController.java @@ -33,13 +33,13 @@ import android.os.UserHandle; import android.text.TextUtils; import android.util.ArrayMap; import android.util.ArraySet; -import android.util.Log; import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.net.module.util.CollectionUtils; import com.android.net.module.util.DeviceConfigUtils; +import com.android.net.module.util.SharedLog; import java.util.List; import java.util.Set; @@ -62,6 +62,7 @@ public class SatelliteAccessController { // This value is taken from android.os.UserHandle#PER_USER_RANGE. @VisibleForTesting public static final int PER_USER_RANGE = 100000; + private static final int MAX_LOG_ENTRIES = 50; private final Context mContext; private final Dependencies mDeps; private final DefaultMessageRoleListener mDefaultMessageRoleListener; @@ -69,6 +70,7 @@ public class SatelliteAccessController { private final Handler mConnectivityServiceHandler; private final PackageManager mPackageManager; private final boolean mSupportConstrainedDataSatelliteOptIn; + private final SharedLog mLog = new SharedLog(MAX_LOG_ENTRIES, TAG); // At this sparseArray, Key is userId and values are uids of SMS apps that are allowed // to use satellite network as fallback. @@ -92,7 +94,7 @@ public class SatelliteAccessController { @Override public void onRoleHoldersChanged(String role, UserHandle userHandle) { if (RoleManager.ROLE_SMS.equals(role) && updateSatelliteRoleSmsUids(userHandle)) { - Log.i(TAG, "ROLE_SMS Change detected "); + mLog.i("ROLE_SMS Change detected "); reportSatelliteNetworkFallbackUids(); } } @@ -102,7 +104,7 @@ public class SatelliteAccessController { mDeps.addOnRoleHoldersChangedListenerAsUser( mConnectivityServiceHandler::post, this, UserHandle.ALL); } catch (RuntimeException e) { - Log.wtf(TAG, "Could not register satellite controller listener due to " + e); + mLog.wtf("Could not register satellite controller listener due to " + e); } } } @@ -169,7 +171,7 @@ public class SatelliteAccessController { } } } else { - Log.wtf(TAG, "package manager found null"); + mLog.wtf("package manager found null for user " + userHandle); } return roleSmsUids; } @@ -190,7 +192,7 @@ public class SatelliteAccessController { ApplicationInfo applicationInfo = packageManager.getApplicationInfo(pkgName, 0); return applicationInfo.uid; } catch (PackageManager.NameNotFoundException exception) { - Log.e(TAG, "Unable to find uid for package: " + pkgName); + mLog.e("Unable to find uid for package: " + pkgName); } return INVALID_UID; } @@ -199,7 +201,7 @@ public class SatelliteAccessController { private boolean updateSatelliteRoleSmsUids(@NonNull UserHandle userHandle) { int userId = userHandle.getIdentifier(); if (userId == INVALID_UID) { - Log.wtf(TAG, "Invalid User Id"); + mLog.wtf("Invalid User Id for userHandle:" + userHandle); return false; } @@ -228,7 +230,7 @@ public class SatelliteAccessController { for (int i = 0; i < mSatelliteRoleSmsUids.size(); i++) { mergedSatelliteRoleSmsUids.addAll(mSatelliteRoleSmsUids.valueAt(i)); } - Log.i(TAG, "SmsRoleUids: " + mergedSatelliteRoleSmsUids + mLog.i("SmsRoleUids:" + mergedSatelliteRoleSmsUids + " Opt-InUids:" + mSatelliteDataOptInUids); // trigger multiple layer request for satellite network fallback of multi user uids @@ -246,7 +248,7 @@ public class SatelliteAccessController { @CheckReturnValue private boolean updateSatelliteRoleSmsUidListOnUserRemoval(int userIdRemoved) { - Log.i(TAG, "user id removed:" + userIdRemoved); + mLog.i("user id removed:" + userIdRemoved); if (mSatelliteRoleSmsUids.contains(userIdRemoved)) { mSatelliteRoleSmsUids.remove(userIdRemoved); return true; // Changed. @@ -278,7 +280,7 @@ public class SatelliteAccessController { final Set satelliteDataOptInUidsForUser = getSatelliteDataOptInUidsForUser(apps); if (satelliteDataOptInUidsForUser.size() > 0) { - Log.i(TAG, "Add SatelliteDataOptInUids for user " + userHandle + ": " + mLog.i("Add SatelliteDataOptInUids for user " + userHandle + ": " + satelliteDataOptInUidsForUser); mSatelliteDataOptInUids.addAll(satelliteDataOptInUidsForUser); optInUidsChanged = true; @@ -340,7 +342,7 @@ public class SatelliteAccessController { public void onExternalApplicationsAvailable(String[] pkgList) { if (!mSupportConstrainedDataSatelliteOptIn) return; if (CollectionUtils.isEmpty(pkgList)) { - Log.e(TAG, "No available external application."); + mLog.e("No available external application."); return; } @@ -414,12 +416,16 @@ public class SatelliteAccessController { public void dump(@NonNull IndentingPrintWriter pw) { pw.println("SatelliteAccessController:"); pw.increaseIndent(); + pw.println("SupportConstrainedDataSatelliteOptIn: " + + mSupportConstrainedDataSatelliteOptIn); pw.print("Role-Sms Uids: "); pw.print(mSatelliteRoleSmsUids); pw.println(); pw.print("Opt-In Uids: "); pw.print(mSatelliteDataOptInUids); pw.println(); + pw.println("Log:"); + mLog.reverseDump(pw); pw.decreaseIndent(); pw.println(); }