From 94311aa902f5d4e8396b7bf5980c08e237b1399b Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Sun, 18 Apr 2021 00:06:08 +0900 Subject: [PATCH] Remove mPublicSync. This is no longer necessary as all the methods that take it are running on on the handler thread, either in Tethering itself or on the state machine thread in IpServer, which runs on the same thread as Tethering. Specifically: - interfaceStatusChanged, interfaceAdded, interfaceRemoved, interfaceLinkStateChanged: run from mNetdCallback, which always posts them to mHandler. - setWifiTethering: only called by enableTetheringInternal, which is called by the following: - startTethering, stopTethering: via lambda posted to mHandler - IpServerCallback#requestEnableTethering: called by IpServer while processing a command. - setEthernetTethering: only called by enableTetheringInternal. - EthernetCallback: runs on mExecutor, which posts to mHandler. - getLastTetherError: only used by the test. Renamed to getLastErrorForTest to ensure no other callers. - sendTetherStateChangedBroadcast: called only by notifyInterfaceStateChange, which is called only by - IpServerCallback#updateInterfaceState, which is called only by sendInterfaceState, which is called by various IpServer state enter methods. - notifyLinkPropertiesChanged: called only by IpServerCallback#updateLinkProperties, which is only called by IpServer#sendLinkProperties, which is only called by: - Code that processes CMD_IPV6_TETHER_UPDATE - IpServer#handleNewPrefixRequest: only called when processing CMD_NEW_PREFIX_REQUEST. - IpServer#sendInterfaceState (see above) - handleWifiApAction, handleWifiP2pAction: only called by mStateReceiver, which runs on the handler thread - tether(String, int): called by: - tether(String, IIntResultListener): posted to mHandler - changeInterfaceState: called by: - EthernetCallback (see above) - enableWifiIpServingLocked: called by handleWifiApAction and handleWifiP2pAction (see above) - tetherMatchingInterfaces: only called by handleUsbAction, which is run from mStateReceiver on the handler thread. - untether(String): called by: - untether(String, IIntResultListener): posted to mHandler - changeInterfaceState (see above) - setUsbTethering: called by: - setUsbTethering(boolean, IIntResultListener): posted to mHandler - enableTetheringInternal (see above) - setNcmTethering: called by enableTetheringInternal (see above) - getTetheredIfaces: called only by TetheringTest. Renamed to getTetheredIfacesForTest to ensure no other callers. - getErroredIfaces: unused, deleted in this CL - getTetheredIfaces: called by: - isTetheringActive: called by onUserRestrictionsChanged, which is only called by mStateReceiver - TetheringTest - dump(): changed to run on handler thread - upstreamWanted: called by - TetherModeAliveState#enter - TetherModeAliveState#updateUpstreamWanted, which is called only by TetherModeAliveState#processMessage. Test: atest TetheringCoverageTests Test: enabled/disabled hotspot, USB tethering Change-Id: Id49d33f027b8df4c97fda480ff239c0ba90bb96a --- .../networkstack/tethering/Tethering.java | 494 ++++++++---------- .../tethering/TetheringService.java | 4 +- .../tethering/TetheringServiceTest.java | 11 +- .../networkstack/tethering/TetheringTest.java | 20 +- 4 files changed, 236 insertions(+), 293 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index d2e726e023..1311e7b461 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -128,7 +128,6 @@ import android.util.SparseArray; import androidx.annotation.NonNull; import androidx.annotation.Nullable; -import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.MessageUtils; @@ -218,9 +217,6 @@ public class Tethering { private final SparseArray mActiveTetheringRequests = new SparseArray<>(); - // used to synchronize public access to members - // TODO(b/153621704): remove mPublicSync to make Tethering lock free - private final Object mPublicSync; private final Context mContext; private final ArrayMap mTetherStates; private final BroadcastReceiver mStateReceiver; @@ -246,8 +242,6 @@ public class Tethering { private final BpfCoordinator mBpfCoordinator; private final PrivateAddressCoordinator mPrivateAddressCoordinator; private int mActiveDataSubId = INVALID_SUBSCRIPTION_ID; - // All the usage of mTetheringEventCallback should run in the same thread. - private ITetheringEventCallback mTetheringEventCallback = null; private volatile TetheringConfiguration mConfig; private InterfaceSet mCurrentUpstreamIfaceSet; @@ -261,11 +255,8 @@ public class Tethering { private String mWifiP2pTetherInterface = null; private int mOffloadStatus = TETHER_HARDWARE_OFFLOAD_STOPPED; - @GuardedBy("mPublicSync") private EthernetManager.TetheredInterfaceRequest mEthernetIfaceRequest; - @GuardedBy("mPublicSync") private String mConfiguredEthernetIface; - @GuardedBy("mPublicSync") private EthernetCallback mEthernetCallback; public Tethering(TetheringDependencies deps) { @@ -276,8 +267,6 @@ public class Tethering { mLooper = mDeps.getTetheringLooper(); mNotificationUpdater = mDeps.getNotificationUpdater(mContext, mLooper); - mPublicSync = new Object(); - mTetherStates = new ArrayMap<>(); mConnectedClientsTracker = new ConnectedClientsTracker(); @@ -504,20 +493,18 @@ public class Tethering { // Never called directly: only called from interfaceLinkStateChanged. // See NetlinkHandler.cpp: notifyInterfaceChanged. if (VDBG) Log.d(TAG, "interfaceStatusChanged " + iface + ", " + up); - synchronized (mPublicSync) { - if (up) { - maybeTrackNewInterfaceLocked(iface); + if (up) { + maybeTrackNewInterfaceLocked(iface); + } else { + if (ifaceNameToType(iface) == TETHERING_BLUETOOTH + || ifaceNameToType(iface) == TETHERING_WIGIG) { + stopTrackingInterfaceLocked(iface); } else { - if (ifaceNameToType(iface) == TETHERING_BLUETOOTH - || ifaceNameToType(iface) == TETHERING_WIGIG) { - stopTrackingInterfaceLocked(iface); - } else { - // Ignore usb0 down after enabling RNDIS. - // We will handle disconnect in interfaceRemoved. - // Similarly, ignore interface down for WiFi. We monitor WiFi AP status - // through the WifiManager.WIFI_AP_STATE_CHANGED_ACTION intent. - if (VDBG) Log.d(TAG, "ignore interface down for " + iface); - } + // Ignore usb0 down after enabling RNDIS. + // We will handle disconnect in interfaceRemoved. + // Similarly, ignore interface down for WiFi. We monitor WiFi AP status + // through the WifiManager.WIFI_AP_STATE_CHANGED_ACTION intent. + if (VDBG) Log.d(TAG, "ignore interface down for " + iface); } } } @@ -547,16 +534,12 @@ public class Tethering { void interfaceAdded(String iface) { if (VDBG) Log.d(TAG, "interfaceAdded " + iface); - synchronized (mPublicSync) { - maybeTrackNewInterfaceLocked(iface); - } + maybeTrackNewInterfaceLocked(iface); } void interfaceRemoved(String iface) { if (VDBG) Log.d(TAG, "interfaceRemoved " + iface); - synchronized (mPublicSync) { - stopTrackingInterfaceLocked(iface); - } + stopTrackingInterfaceLocked(iface); } void startTethering(final TetheringRequestParcel request, final IIntResultListener listener) { @@ -641,17 +624,15 @@ public class Tethering { private int setWifiTethering(final boolean enable) { final long ident = Binder.clearCallingIdentity(); try { - synchronized (mPublicSync) { - final WifiManager mgr = getWifiManager(); - if (mgr == null) { - mLog.e("setWifiTethering: failed to get WifiManager!"); - return TETHER_ERROR_SERVICE_UNAVAIL; - } - if ((enable && mgr.startTetheredHotspot(null /* use existing softap config */)) - || (!enable && mgr.stopSoftAp())) { - mWifiTetherRequested = enable; - return TETHER_ERROR_NO_ERROR; - } + final WifiManager mgr = getWifiManager(); + if (mgr == null) { + mLog.e("setWifiTethering: failed to get WifiManager!"); + return TETHER_ERROR_SERVICE_UNAVAIL; + } + if ((enable && mgr.startTetheredHotspot(null /* use existing softap config */)) + || (!enable && mgr.stopSoftAp())) { + mWifiTetherRequested = enable; + return TETHER_ERROR_NO_ERROR; } } finally { Binder.restoreCallingIdentity(ident); @@ -703,18 +684,16 @@ public class Tethering { private int setEthernetTethering(final boolean enable) { final EthernetManager em = (EthernetManager) mContext.getSystemService( Context.ETHERNET_SERVICE); - synchronized (mPublicSync) { - if (enable) { - if (mEthernetCallback != null) { - Log.d(TAG, "Ethernet tethering already started"); - return TETHER_ERROR_NO_ERROR; - } - - mEthernetCallback = new EthernetCallback(); - mEthernetIfaceRequest = em.requestTetheredInterface(mExecutor, mEthernetCallback); - } else { - stopEthernetTetheringLocked(); + if (enable) { + if (mEthernetCallback != null) { + Log.d(TAG, "Ethernet tethering already started"); + return TETHER_ERROR_NO_ERROR; } + + mEthernetCallback = new EthernetCallback(); + mEthernetIfaceRequest = em.requestTetheredInterface(mExecutor, mEthernetCallback); + } else { + stopEthernetTetheringLocked(); } return TETHER_ERROR_NO_ERROR; } @@ -734,26 +713,22 @@ public class Tethering { private class EthernetCallback implements EthernetManager.TetheredInterfaceCallback { @Override public void onAvailable(String iface) { - synchronized (mPublicSync) { - if (this != mEthernetCallback) { - // Ethernet callback arrived after Ethernet tethering stopped. Ignore. - return; - } - maybeTrackNewInterfaceLocked(iface, TETHERING_ETHERNET); - changeInterfaceState(iface, getRequestedState(TETHERING_ETHERNET)); - mConfiguredEthernetIface = iface; + if (this != mEthernetCallback) { + // Ethernet callback arrived after Ethernet tethering stopped. Ignore. + return; } + maybeTrackNewInterfaceLocked(iface, TETHERING_ETHERNET); + changeInterfaceState(iface, getRequestedState(TETHERING_ETHERNET)); + mConfiguredEthernetIface = iface; } @Override public void onUnavailable() { - synchronized (mPublicSync) { - if (this != mEthernetCallback) { - // onAvailable called after stopping Ethernet tethering. - return; - } - stopEthernetTetheringLocked(); + if (this != mEthernetCallback) { + // onAvailable called after stopping Ethernet tethering. + return; } + stopEthernetTetheringLocked(); } } @@ -767,57 +742,53 @@ public class Tethering { private int tether(String iface, int requestedState) { if (DBG) Log.d(TAG, "Tethering " + iface); - synchronized (mPublicSync) { - TetherState tetherState = mTetherStates.get(iface); - if (tetherState == null) { - Log.e(TAG, "Tried to Tether an unknown iface: " + iface + ", ignoring"); - return TETHER_ERROR_UNKNOWN_IFACE; - } - // Ignore the error status of the interface. If the interface is available, - // the errors are referring to past tethering attempts anyway. - if (tetherState.lastState != IpServer.STATE_AVAILABLE) { - Log.e(TAG, "Tried to Tether an unavailable iface: " + iface + ", ignoring"); - return TETHER_ERROR_UNAVAIL_IFACE; - } - // NOTE: If a CMD_TETHER_REQUESTED message is already in the TISM's queue but not yet - // processed, this will be a no-op and it will not return an error. - // - // This code cannot race with untether() because they both synchronize on mPublicSync. - // TODO: reexamine the threading and messaging model to totally remove mPublicSync. - final int type = tetherState.ipServer.interfaceType(); - final TetheringRequestParcel request = mActiveTetheringRequests.get(type, null); - if (request != null) { - mActiveTetheringRequests.delete(type); - } - tetherState.ipServer.sendMessage(IpServer.CMD_TETHER_REQUESTED, requestedState, 0, - request); - return TETHER_ERROR_NO_ERROR; + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { + Log.e(TAG, "Tried to Tether an unknown iface: " + iface + ", ignoring"); + return TETHER_ERROR_UNKNOWN_IFACE; } + // Ignore the error status of the interface. If the interface is available, + // the errors are referring to past tethering attempts anyway. + if (tetherState.lastState != IpServer.STATE_AVAILABLE) { + Log.e(TAG, "Tried to Tether an unavailable iface: " + iface + ", ignoring"); + return TETHER_ERROR_UNAVAIL_IFACE; + } + // NOTE: If a CMD_TETHER_REQUESTED message is already in the TISM's queue but not yet + // processed, this will be a no-op and it will not return an error. + // + // This code cannot race with untether() because they both run on the handler thread. + final int type = tetherState.ipServer.interfaceType(); + final TetheringRequestParcel request = mActiveTetheringRequests.get(type, null); + if (request != null) { + mActiveTetheringRequests.delete(type); + } + tetherState.ipServer.sendMessage(IpServer.CMD_TETHER_REQUESTED, requestedState, 0, + request); + return TETHER_ERROR_NO_ERROR; } void untether(String iface, final IIntResultListener listener) { mHandler.post(() -> { try { listener.onResult(untether(iface)); - } catch (RemoteException e) { } + } catch (RemoteException e) { + } }); } int untether(String iface) { if (DBG) Log.d(TAG, "Untethering " + iface); - synchronized (mPublicSync) { - TetherState tetherState = mTetherStates.get(iface); - if (tetherState == null) { - Log.e(TAG, "Tried to Untether an unknown iface :" + iface + ", ignoring"); - return TETHER_ERROR_UNKNOWN_IFACE; - } - if (!tetherState.isCurrentlyServing()) { - Log.e(TAG, "Tried to untether an inactive iface :" + iface + ", ignoring"); - return TETHER_ERROR_UNAVAIL_IFACE; - } - tetherState.ipServer.sendMessage(IpServer.CMD_TETHER_UNREQUESTED); - return TETHER_ERROR_NO_ERROR; + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { + Log.e(TAG, "Tried to Untether an unknown iface :" + iface + ", ignoring"); + return TETHER_ERROR_UNKNOWN_IFACE; } + if (!tetherState.isCurrentlyServing()) { + Log.e(TAG, "Tried to untether an inactive iface :" + iface + ", ignoring"); + return TETHER_ERROR_UNAVAIL_IFACE; + } + tetherState.ipServer.sendMessage(IpServer.CMD_TETHER_UNREQUESTED); + return TETHER_ERROR_NO_ERROR; } void untetherAll() { @@ -828,16 +799,15 @@ public class Tethering { stopTethering(TETHERING_ETHERNET); } - int getLastTetherError(String iface) { - synchronized (mPublicSync) { - TetherState tetherState = mTetherStates.get(iface); - if (tetherState == null) { - Log.e(TAG, "Tried to getLastTetherError on an unknown iface :" + iface - + ", ignoring"); - return TETHER_ERROR_UNKNOWN_IFACE; - } - return tetherState.lastError; + @VisibleForTesting + int getLastErrorForTest(String iface) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { + Log.e(TAG, "Tried to getLastErrorForTest on an unknown iface :" + iface + + ", ignoring"); + return TETHER_ERROR_UNKNOWN_IFACE; } + return tetherState.lastError; } private boolean isProvisioningNeededButUnavailable() { @@ -894,27 +864,25 @@ public class Tethering { mTetherStatesParcel = new TetherStatesParcel(); int downstreamTypesMask = DOWNSTREAM_NONE; - synchronized (mPublicSync) { - for (int i = 0; i < mTetherStates.size(); i++) { - TetherState tetherState = mTetherStates.valueAt(i); - String iface = mTetherStates.keyAt(i); - if (tetherState.lastError != TETHER_ERROR_NO_ERROR) { - erroredList.add(iface); - lastErrorList.add(tetherState.lastError); - } else if (tetherState.lastState == IpServer.STATE_AVAILABLE) { - availableList.add(iface); - } else if (tetherState.lastState == IpServer.STATE_LOCAL_ONLY) { - localOnlyList.add(iface); - } else if (tetherState.lastState == IpServer.STATE_TETHERED) { - if (cfg.isUsb(iface)) { - downstreamTypesMask |= (1 << TETHERING_USB); - } else if (cfg.isWifi(iface)) { - downstreamTypesMask |= (1 << TETHERING_WIFI); - } else if (cfg.isBluetooth(iface)) { - downstreamTypesMask |= (1 << TETHERING_BLUETOOTH); - } - tetherList.add(iface); + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + String iface = mTetherStates.keyAt(i); + if (tetherState.lastError != TETHER_ERROR_NO_ERROR) { + erroredList.add(iface); + lastErrorList.add(tetherState.lastError); + } else if (tetherState.lastState == IpServer.STATE_AVAILABLE) { + availableList.add(iface); + } else if (tetherState.lastState == IpServer.STATE_LOCAL_ONLY) { + localOnlyList.add(iface); + } else if (tetherState.lastState == IpServer.STATE_TETHERED) { + if (cfg.isUsb(iface)) { + downstreamTypesMask |= (1 << TETHERING_USB); + } else if (cfg.isWifi(iface)) { + downstreamTypesMask |= (1 << TETHERING_WIFI); + } else if (cfg.isBluetooth(iface)) { + downstreamTypesMask |= (1 << TETHERING_BLUETOOTH); } + tetherList.add(iface); } } @@ -1012,21 +980,19 @@ public class Tethering { // functions are ready to use. // // For more explanation, see b/62552150 . - synchronized (Tethering.this.mPublicSync) { - if (!usbConnected && mRndisEnabled) { - // Turn off tethering if it was enabled and there is a disconnect. - tetherMatchingInterfaces(IpServer.STATE_AVAILABLE, TETHERING_USB); - mEntitlementMgr.stopProvisioningIfNeeded(TETHERING_USB); - } else if (usbConfigured && rndisEnabled) { - // Tether if rndis is enabled and usb is configured. - final int state = getRequestedState(TETHERING_USB); - tetherMatchingInterfaces(state, TETHERING_USB); - } else if (usbConnected && ncmEnabled) { - final int state = getRequestedState(TETHERING_NCM); - tetherMatchingInterfaces(state, TETHERING_NCM); - } - mRndisEnabled = usbConfigured && rndisEnabled; + if (!usbConnected && mRndisEnabled) { + // Turn off tethering if it was enabled and there is a disconnect. + tetherMatchingInterfaces(IpServer.STATE_AVAILABLE, TETHERING_USB); + mEntitlementMgr.stopProvisioningIfNeeded(TETHERING_USB); + } else if (usbConfigured && rndisEnabled) { + // Tether if rndis is enabled and usb is configured. + final int state = getRequestedState(TETHERING_USB); + tetherMatchingInterfaces(state, TETHERING_USB); + } else if (usbConnected && ncmEnabled) { + final int state = getRequestedState(TETHERING_NCM); + tetherMatchingInterfaces(state, TETHERING_NCM); } + mRndisEnabled = usbConfigured && rndisEnabled; } private void handleWifiApAction(Intent intent) { @@ -1034,23 +1000,21 @@ public class Tethering { final String ifname = intent.getStringExtra(EXTRA_WIFI_AP_INTERFACE_NAME); final int ipmode = intent.getIntExtra(EXTRA_WIFI_AP_MODE, IFACE_IP_MODE_UNSPECIFIED); - synchronized (Tethering.this.mPublicSync) { - switch (curState) { - case WifiManager.WIFI_AP_STATE_ENABLING: - // We can see this state on the way to both enabled and failure states. - break; - case WifiManager.WIFI_AP_STATE_ENABLED: - enableWifiIpServingLocked(ifname, ipmode); - break; - case WifiManager.WIFI_AP_STATE_DISABLING: - // We can see this state on the way to disabled. - break; - case WifiManager.WIFI_AP_STATE_DISABLED: - case WifiManager.WIFI_AP_STATE_FAILED: - default: - disableWifiIpServingLocked(ifname, curState); - break; - } + switch (curState) { + case WifiManager.WIFI_AP_STATE_ENABLING: + // We can see this state on the way to both enabled and failure states. + break; + case WifiManager.WIFI_AP_STATE_ENABLED: + enableWifiIpServingLocked(ifname, ipmode); + break; + case WifiManager.WIFI_AP_STATE_DISABLING: + // We can see this state on the way to disabled. + break; + case WifiManager.WIFI_AP_STATE_DISABLED: + case WifiManager.WIFI_AP_STATE_FAILED: + default: + disableWifiIpServingLocked(ifname, curState); + break; } } @@ -1071,32 +1035,30 @@ public class Tethering { Log.d(TAG, "WifiP2pAction: P2pInfo: " + p2pInfo + " Group: " + group); } - synchronized (Tethering.this.mPublicSync) { - // if no group is formed, bring it down if needed. - if (p2pInfo == null || !p2pInfo.groupFormed) { - disableWifiP2pIpServingLockedIfNeeded(mWifiP2pTetherInterface); - mWifiP2pTetherInterface = null; - return; - } - - // If there is a group but the device is not the owner, bail out. - if (!isGroupOwner(group)) return; - - // If already serving from the correct interface, nothing to do. - if (group.getInterface().equals(mWifiP2pTetherInterface)) return; - - // If already serving from another interface, turn it down first. - if (!TextUtils.isEmpty(mWifiP2pTetherInterface)) { - mLog.w("P2P tethered interface " + mWifiP2pTetherInterface - + "is different from current interface " - + group.getInterface() + ", re-tether it"); - disableWifiP2pIpServingLockedIfNeeded(mWifiP2pTetherInterface); - } - - // Finally bring up serving on the new interface - mWifiP2pTetherInterface = group.getInterface(); - enableWifiIpServingLocked(mWifiP2pTetherInterface, IFACE_IP_MODE_LOCAL_ONLY); + // if no group is formed, bring it down if needed. + if (p2pInfo == null || !p2pInfo.groupFormed) { + disableWifiP2pIpServingLockedIfNeeded(mWifiP2pTetherInterface); + mWifiP2pTetherInterface = null; + return; } + + // If there is a group but the device is not the owner, bail out. + if (!isGroupOwner(group)) return; + + // If already serving from the correct interface, nothing to do. + if (group.getInterface().equals(mWifiP2pTetherInterface)) return; + + // If already serving from another interface, turn it down first. + if (!TextUtils.isEmpty(mWifiP2pTetherInterface)) { + mLog.w("P2P tethered interface " + mWifiP2pTetherInterface + + "is different from current interface " + + group.getInterface() + ", re-tether it"); + disableWifiP2pIpServingLockedIfNeeded(mWifiP2pTetherInterface); + } + + // Finally bring up serving on the new interface + mWifiP2pTetherInterface = group.getInterface(); + enableWifiIpServingLocked(mWifiP2pTetherInterface, IFACE_IP_MODE_LOCAL_ONLY); } private void handleUserRestrictionAction() { @@ -1131,14 +1093,14 @@ public class Tethering { @VisibleForTesting protected static class UserRestrictionActionListener { private final UserManager mUserMgr; - private final Tethering mWrapper; + private final Tethering mTethering; private final TetheringNotificationUpdater mNotificationUpdater; public boolean mDisallowTethering; - public UserRestrictionActionListener(@NonNull UserManager um, @NonNull Tethering wrapper, + public UserRestrictionActionListener(@NonNull UserManager um, @NonNull Tethering tethering, @NonNull TetheringNotificationUpdater updater) { mUserMgr = um; - mWrapper = wrapper; + mTethering = tethering; mNotificationUpdater = updater; mDisallowTethering = false; } @@ -1165,13 +1127,13 @@ public class Tethering { return; } - if (mWrapper.isTetheringActive()) { + if (mTethering.isTetheringActive()) { // Restricted notification is shown when tethering function is disallowed on // user's device. mNotificationUpdater.notifyTetheringDisabledByRestriction(); // Untether from all downstreams since tethering is disallowed. - mWrapper.untetherAll(); + mTethering.untetherAll(); } // TODO(b/148139325): send tetheringSupported on restriction change } @@ -1332,53 +1294,51 @@ public class Tethering { return copy(mConfig.tetherableBluetoothRegexs); } - int setUsbTethering(boolean enable) { + void setUsbTethering(boolean enable, IIntResultListener listener) { + mHandler.post(() -> { + try { + listener.onResult(setUsbTethering(enable)); + } catch (RemoteException e) { } + }); + } + + private int setUsbTethering(boolean enable) { if (VDBG) Log.d(TAG, "setUsbTethering(" + enable + ")"); UsbManager usbManager = (UsbManager) mContext.getSystemService(Context.USB_SERVICE); if (usbManager == null) { mLog.e("setUsbTethering: failed to get UsbManager!"); return TETHER_ERROR_SERVICE_UNAVAIL; } - - synchronized (mPublicSync) { - usbManager.setCurrentFunctions(enable ? UsbManager.FUNCTION_RNDIS - : UsbManager.FUNCTION_NONE); - } + usbManager.setCurrentFunctions(enable ? UsbManager.FUNCTION_RNDIS + : UsbManager.FUNCTION_NONE); return TETHER_ERROR_NO_ERROR; } private int setNcmTethering(boolean enable) { if (VDBG) Log.d(TAG, "setNcmTethering(" + enable + ")"); UsbManager usbManager = (UsbManager) mContext.getSystemService(Context.USB_SERVICE); - synchronized (mPublicSync) { - usbManager.setCurrentFunctions(enable ? UsbManager.FUNCTION_NCM - : UsbManager.FUNCTION_NONE); - } + usbManager.setCurrentFunctions(enable ? UsbManager.FUNCTION_NCM : UsbManager.FUNCTION_NONE); return TETHER_ERROR_NO_ERROR; } // TODO review API - figure out how to delete these entirely. String[] getTetheredIfaces() { ArrayList list = new ArrayList(); - synchronized (mPublicSync) { - for (int i = 0; i < mTetherStates.size(); i++) { - TetherState tetherState = mTetherStates.valueAt(i); - if (tetherState.lastState == IpServer.STATE_TETHERED) { - list.add(mTetherStates.keyAt(i)); - } + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.lastState == IpServer.STATE_TETHERED) { + list.add(mTetherStates.keyAt(i)); } } return list.toArray(new String[list.size()]); } - String[] getTetherableIfaces() { + String[] getTetherableIfacesForTest() { ArrayList list = new ArrayList(); - synchronized (mPublicSync) { - for (int i = 0; i < mTetherStates.size(); i++) { - TetherState tetherState = mTetherStates.valueAt(i); - if (tetherState.lastState == IpServer.STATE_AVAILABLE) { - list.add(mTetherStates.keyAt(i)); - } + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.lastState == IpServer.STATE_AVAILABLE) { + list.add(mTetherStates.keyAt(i)); } } return list.toArray(new String[list.size()]); @@ -1390,29 +1350,13 @@ public class Tethering { return mConfig.legacyDhcpRanges; } - String[] getErroredIfaces() { - ArrayList list = new ArrayList(); - synchronized (mPublicSync) { - for (int i = 0; i < mTetherStates.size(); i++) { - TetherState tetherState = mTetherStates.valueAt(i); - if (tetherState.lastError != TETHER_ERROR_NO_ERROR) { - list.add(mTetherStates.keyAt(i)); - } - } - } - return list.toArray(new String[list.size()]); - } - private void logMessage(State state, int what) { mLog.log(state.getName() + " got " + sMagicDecoderRing.get(what, Integer.toString(what))); } private boolean upstreamWanted() { if (!mForwardedDownstreams.isEmpty()) return true; - - synchronized (mPublicSync) { - return mWifiTetherRequested; - } + return mWifiTetherRequested; } // Needed because the canonical source of upstream truth is just the @@ -2308,37 +2252,35 @@ public class Tethering { mEntitlementMgr.dump(pw); pw.decreaseIndent(); - synchronized (mPublicSync) { - pw.println("Tether state:"); - pw.increaseIndent(); - for (int i = 0; i < mTetherStates.size(); i++) { - final String iface = mTetherStates.keyAt(i); - final TetherState tetherState = mTetherStates.valueAt(i); - pw.print(iface + " - "); + pw.println("Tether state:"); + pw.increaseIndent(); + for (int i = 0; i < mTetherStates.size(); i++) { + final String iface = mTetherStates.keyAt(i); + final TetherState tetherState = mTetherStates.valueAt(i); + pw.print(iface + " - "); - switch (tetherState.lastState) { - case IpServer.STATE_UNAVAILABLE: - pw.print("UnavailableState"); - break; - case IpServer.STATE_AVAILABLE: - pw.print("AvailableState"); - break; - case IpServer.STATE_TETHERED: - pw.print("TetheredState"); - break; - case IpServer.STATE_LOCAL_ONLY: - pw.print("LocalHotspotState"); - break; - default: - pw.print("UnknownState"); - break; - } - pw.println(" - lastError = " + tetherState.lastError); + switch (tetherState.lastState) { + case IpServer.STATE_UNAVAILABLE: + pw.print("UnavailableState"); + break; + case IpServer.STATE_AVAILABLE: + pw.print("AvailableState"); + break; + case IpServer.STATE_TETHERED: + pw.print("TetheredState"); + break; + case IpServer.STATE_LOCAL_ONLY: + pw.print("LocalHotspotState"); + break; + default: + pw.print("UnknownState"); + break; } - pw.println("Upstream wanted: " + upstreamWanted()); - pw.println("Current upstream interface(s): " + mCurrentUpstreamIfaceSet); - pw.decreaseIndent(); + pw.println(" - lastError = " + tetherState.lastError); } + pw.println("Upstream wanted: " + upstreamWanted()); + pw.println("Current upstream interface(s): " + mCurrentUpstreamIfaceSet); + pw.decreaseIndent(); pw.println("Hardware offload:"); pw.increaseIndent(); @@ -2439,14 +2381,12 @@ public class Tethering { // TODO: Move into TetherMainSM. private void notifyInterfaceStateChange(IpServer who, int state, int error) { final String iface = who.interfaceName(); - synchronized (mPublicSync) { - final TetherState tetherState = mTetherStates.get(iface); - if (tetherState != null && tetherState.ipServer.equals(who)) { - tetherState.lastState = state; - tetherState.lastError = error; - } else { - if (DBG) Log.d(TAG, "got notification from stale iface " + iface); - } + final TetherState tetherState = mTetherStates.get(iface); + if (tetherState != null && tetherState.ipServer.equals(who)) { + tetherState.lastState = state; + tetherState.lastError = error; + } else { + if (DBG) Log.d(TAG, "got notification from stale iface " + iface); } mLog.log(String.format("OBSERVED iface=%s state=%s error=%s", iface, state, error)); @@ -2478,14 +2418,12 @@ public class Tethering { private void notifyLinkPropertiesChanged(IpServer who, LinkProperties newLp) { final String iface = who.interfaceName(); final int state; - synchronized (mPublicSync) { - final TetherState tetherState = mTetherStates.get(iface); - if (tetherState != null && tetherState.ipServer.equals(who)) { - state = tetherState.lastState; - } else { - mLog.log("got notification from stale iface " + iface); - return; - } + final TetherState tetherState = mTetherStates.get(iface); + if (tetherState != null && tetherState.ipServer.equals(who)) { + state = tetherState.lastState; + } else { + mLog.log("got notification from stale iface " + iface); + return; } mLog.log(String.format( diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringService.java b/Tethering/src/com/android/networkstack/tethering/TetheringService.java index e36df7fac2..745ebd0762 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringService.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringService.java @@ -121,9 +121,7 @@ public class TetheringService extends Service { IIntResultListener listener) { if (checkAndNotifyCommonError(callerPkg, callingAttributionTag, listener)) return; - try { - listener.onResult(mTethering.setUsbTethering(enable)); - } catch (RemoteException e) { } + mTethering.setUsbTethering(enable, listener); } @Override diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java index 7204ff6747..941cd78a62 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java @@ -25,7 +25,10 @@ import static android.net.TetheringManager.TETHER_ERROR_NO_CHANGE_TETHERING_PERM import static android.net.TetheringManager.TETHER_ERROR_NO_ERROR; import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -214,11 +217,15 @@ public final class TetheringServiceTest { } private void runSetUsbTethering(final TestTetheringResult result) throws Exception { - when(mTethering.setUsbTethering(true /* enable */)).thenReturn(TETHER_ERROR_NO_ERROR); + doAnswer((invocation) -> { + final IIntResultListener listener = invocation.getArgument(1); + listener.onResult(TETHER_ERROR_NO_ERROR); + return null; + }).when(mTethering).setUsbTethering(anyBoolean(), any(IIntResultListener.class)); mTetheringConnector.setUsbTethering(true /* enable */, TEST_CALLER_PKG, TEST_ATTRIBUTION_TAG, result); verify(mTethering).isTetheringSupported(); - verify(mTethering).setUsbTethering(true /* enable */); + verify(mTethering).setUsbTethering(eq(true) /* enable */, any(IIntResultListener.class)); result.assertResult(TETHER_ERROR_NO_ERROR); } diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index faa5ac7039..fedeeeb609 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -941,7 +941,7 @@ public class TetheringTest { verifyNoMoreInteractions(mWifiManager); // Asking for the last error after the per-interface state machine // has been reaped yields an unknown interface error. - assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastTetherError(TEST_WLAN_IFNAME)); + assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastErrorForTest(TEST_WLAN_IFNAME)); } /** @@ -1474,7 +1474,7 @@ public class TetheringTest { verifyNoMoreInteractions(mWifiManager); // Asking for the last error after the per-interface state machine // has been reaped yields an unknown interface error. - assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastTetherError(TEST_WLAN_IFNAME)); + assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastErrorForTest(TEST_WLAN_IFNAME)); } // TODO: Test with and without interfaceStatusChanged(). @@ -1942,7 +1942,7 @@ public class TetheringTest { // There are 2 IpServer state change events: STATE_AVAILABLE -> STATE_LOCAL_ONLY verify(mNotificationUpdater, times(2)).onDownstreamChanged(DOWNSTREAM_NONE); - assertEquals(TETHER_ERROR_NO_ERROR, mTethering.getLastTetherError(TEST_P2P_IFNAME)); + assertEquals(TETHER_ERROR_NO_ERROR, mTethering.getLastErrorForTest(TEST_P2P_IFNAME)); // Emulate externally-visible WifiP2pManager effects, when wifi p2p group // is being removed. @@ -1961,7 +1961,7 @@ public class TetheringTest { verifyNoMoreInteractions(mNetd); // Asking for the last error after the per-interface state machine // has been reaped yields an unknown interface error. - assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastTetherError(TEST_P2P_IFNAME)); + assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastErrorForTest(TEST_P2P_IFNAME)); } private void workingWifiP2pGroupClient( @@ -1991,7 +1991,7 @@ public class TetheringTest { verifyNoMoreInteractions(mNetd); // Asking for the last error after the per-interface state machine // has been reaped yields an unknown interface error. - assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastTetherError(TEST_P2P_IFNAME)); + assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastErrorForTest(TEST_P2P_IFNAME)); } @Test @@ -2022,7 +2022,7 @@ public class TetheringTest { verify(mNetd, never()).networkAddInterface(INetd.LOCAL_NET_ID, TEST_P2P_IFNAME); verify(mNetd, never()).ipfwdEnableForwarding(TETHERING_NAME); verify(mNetd, never()).tetherStartWithConfiguration(any()); - assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastTetherError(TEST_P2P_IFNAME)); + assertEquals(TETHER_ERROR_UNKNOWN_IFACE, mTethering.getLastErrorForTest(TEST_P2P_IFNAME)); } @Test public void workingWifiP2pGroupOwnerLegacyModeWithIfaceChanged() throws Exception { @@ -2387,10 +2387,10 @@ public class TetheringTest { mTethering.interfaceStatusChanged(TEST_USB_IFNAME, true); sendUsbBroadcast(true, true, true, TETHERING_USB); - assertContains(Arrays.asList(mTethering.getTetherableIfaces()), TEST_USB_IFNAME); - assertContains(Arrays.asList(mTethering.getTetherableIfaces()), TEST_ETH_IFNAME); - assertEquals(TETHER_ERROR_IFACE_CFG_ERROR, mTethering.getLastTetherError(TEST_USB_IFNAME)); - assertEquals(TETHER_ERROR_IFACE_CFG_ERROR, mTethering.getLastTetherError(TEST_ETH_IFNAME)); + assertContains(Arrays.asList(mTethering.getTetherableIfacesForTest()), TEST_USB_IFNAME); + assertContains(Arrays.asList(mTethering.getTetherableIfacesForTest()), TEST_ETH_IFNAME); + assertEquals(TETHER_ERROR_IFACE_CFG_ERROR, mTethering.getLastErrorForTest(TEST_USB_IFNAME)); + assertEquals(TETHER_ERROR_IFACE_CFG_ERROR, mTethering.getLastErrorForTest(TEST_ETH_IFNAME)); } @Test