diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index f7e9c3ba99..4bbe130a8a 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -614,33 +614,40 @@ public class Tethering { if (type == TETHERING_BLUETOOTH && SdkLevel.isAtLeastT()) return; // Cannot happen: on S+, tetherableWigigRegexps is always empty. - if (type == TETHERING_WIGIG && SdkLevel.isAtLeastS()) return; - - // After V, disallow this legacy codepath from starting tethering of any type: - // everything must call ensureIpServerStarted directly. - // - // Don't touch the teardown path for now. It's more complicated because: - // - ensureIpServerStarted and ensureIpServerStopped act on different - // tethering types. - // - Depending on the type, ensureIpServerStopped is either called twice (once - // on interface down and once on interface removed) or just once (on - // interface removed). - // - // Note that this only affects WIFI and WIFI_P2P. The other types are either - // ignored above, or ignored by ensureIpServerStarted. Note that even for WIFI - // and WIFI_P2P, this code should not ever run in normal use, because the - // hotspot and p2p code do not call tether(). It's possible that this could - // happen in the field due to unforeseen OEM modifications. If it does happen, - // a terrible error is logged in tether(). - // TODO: fix the teardown path to stop depending on interface state notifications. - // These are not necessary since most/all link layers have their own teardown - // notifications, and can race with those notifications. - if (enabled && SdkLevel.isAtLeastB()) { + if (type == TETHERING_WIGIG + && (SdkLevel.isAtLeastS() || !hasSystemFeature(PackageManager.FEATURE_WIFI))) { return; } + // After V, don't allow this legacy codepath to create IpServers for any tethering type: + // everything must call ensureIpServerStarted directly. Also, on any release, don't create + // IpServers for WIFI or WIFI_P2P. These types always use the enableIpServing path. + // + // Don't touch the teardown path for now. It's more complicated because: + // - ensureIpServerStarted and ensureIpServerStopped act on different + // tethering types, and ensureIpServerStopped doesn't look at the type at all. + // - Depending on the type, ensureIpServerStopped is either called twice (once + // on interface down and once on interface removed) or just once (on + // interface removed). It's not clear whether it's possible to remove one of + // these two calls without causing breakage in the field - for example, it's + // possible that if the code did not call ensureIpServerStopped on wifi, then + // when wifi tethering is disabled and wifi goes into client mode, tethering + // has not brought down the IpServer because the WIFI_AP_STATE_CHANGED_ACTION + // was delayed. + // + // TODO: fix the teardown path to stop depending on interface state notifications. + // These are not necessary since most/all link layers have their own teardown + // notifications, and can race with those notifications. + if (enabled && SdkLevel.isAtLeastB()) return; + + if (enabled && (type == TETHERING_WIFI || type == TETHERING_WIFI_P2P)) return; + + // In this method, INVALID includes ETHERNET and VIRTUAL, because ifaceNameToType never + // returns these. The legacy codepath does not support these. + if (enabled && type == TETHERING_INVALID) return; + if (enabled) { - ensureIpServerStartedForInterface(iface); + ensureIpServerStarted(iface, type, false /* isNcm */); } else { ensureIpServerStopped(iface); } @@ -1662,7 +1669,7 @@ public class Tethering { } private void enableIpServing(@NonNull TetheringRequest request, String ifname, boolean isNcm) { - ensureIpServerStartedForType(ifname, request.getTetheringType(), isNcm); + ensureIpServerStarted(ifname, request.getTetheringType(), isNcm); if (tetherInternal(request, ifname) != TETHER_ERROR_NO_ERROR) { Log.e(TAG, "unable start tethering on iface " + ifname); } @@ -3053,20 +3060,7 @@ public class Tethering { return type != TETHERING_INVALID; } - private void ensureIpServerStartedForInterface(final String iface) { - // If we don't care about this type of interface, ignore. - final int interfaceType = ifaceNameToType(iface); - if (!checkTetherableType(interfaceType)) { - mLog.log(iface + " is used for " + interfaceType + " which is not tetherable" - + " (-1 == INVALID is expected on upstream interface)"); - return; - } - - ensureIpServerStartedForType(iface, interfaceType, false /* isNcm */); - } - - private void ensureIpServerStartedForType(final String iface, int interfaceType, - boolean isNcm) { + private void ensureIpServerStarted(final String iface, int interfaceType, boolean isNcm) { // If we have already started a TISM for this interface, skip. if (mTetherStates.containsKey(iface)) { mLog.log("active iface (" + iface + ") reported as added, ignoring"); 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 cb25145a4d..a68bc51be9 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -1008,23 +1008,11 @@ public class TetheringTest { } sendWifiApStateChanged(WIFI_AP_STATE_ENABLED); - // If, and only if, Tethering received an interface status changed then - // it creates a IpServer and sends out a broadcast indicating that the - // interface is "available". - if (emulateInterfaceStatusChanged) { - if (!SdkLevel.isAtLeastB()) { - // There is 1 IpServer state change event: STATE_AVAILABLE - verify(mNotificationUpdater, times(1)).onDownstreamChanged(DOWNSTREAM_NONE); - verifyTetheringBroadcast(TEST_WLAN_IFNAME, EXTRA_AVAILABLE_TETHER); - verify(mWifiManager).updateInterfaceIpState( - TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); - } else { - // Starting in B, ignore the interfaceStatusChanged - verify(mNotificationUpdater, never()).onDownstreamChanged(DOWNSTREAM_NONE); - verify(mWifiManager, never()).updateInterfaceIpState( - TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); - } - } + // Wi-Fi tethering ignores interface up events. The behaviour is the same + // regardless of whether the interface up event was received or not. + verify(mNotificationUpdater, never()).onDownstreamChanged(DOWNSTREAM_NONE); + verify(mWifiManager, never()).updateInterfaceIpState( + TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); verifyNoMoreInteractions(mNetd); verifyNoMoreInteractions(mWifiManager); } @@ -2046,18 +2034,10 @@ public class TetheringTest { sendWifiApStateChanged(WIFI_AP_STATE_ENABLED); mLooper.dispatchAll(); - if (!SdkLevel.isAtLeastB()) { - // There is 1 IpServer state change event: STATE_AVAILABLE from interfaceStatusChanged - verify(mNotificationUpdater, times(1)).onDownstreamChanged(DOWNSTREAM_NONE); - verifyTetheringBroadcast(TEST_WLAN_IFNAME, EXTRA_AVAILABLE_TETHER); - verify(mWifiManager).updateInterfaceIpState( - TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); - } else { - // Starting in B, ignore the interfaceStatusChanged - verify(mNotificationUpdater, never()).onDownstreamChanged(DOWNSTREAM_NONE); - verify(mWifiManager, never()).updateInterfaceIpState( - TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); - } + // Wi-Fi tethering ignores interface up events. + verify(mNotificationUpdater, never()).onDownstreamChanged(DOWNSTREAM_NONE); + verify(mWifiManager, never()).updateInterfaceIpState( + TEST_WLAN_IFNAME, WifiManager.IFACE_IP_MODE_UNSPECIFIED); verifyNoMoreInteractions(mNetd); verifyNoMoreInteractions(mWifiManager); } @@ -3428,6 +3408,10 @@ public class TetheringTest { mTethering.legacyTether(TEST_WIFI_IFNAME, result); mLooper.dispatchAll(); result.assertHasResult(); + + // No IpServer should have been started, so no STATE_AVAILABLE should have been sent, and no + // tethering broadcasts should have been sent. + assertEquals(0, mIntents.size()); } @Test