From 2b8a42d666cb7477e06fb215e79e6ca813b8b0ab Mon Sep 17 00:00:00 2001 From: Quang Anh Luong Date: Tue, 28 Jan 2025 18:57:15 +0900 Subject: [PATCH] no-op: Make TetheringRequest @NonNull in enableIpServing Currently the enableIpServing->tetherInternal call stack may have a tethering request alongside a redundant tethering type and requested state. The redundant arguments were necessary since there may be cases where the active request is null (e.g. back-to-back start/stop, which removes the active request before the link layer event enables ip serving). This change gets rid of the redundant arguments and makes the call stack take a non-null TetheringRequest. In case the active request is not found, we will create a placeholder request with just the type and connectivity scope (i.e. requested state). This should be equivalent to the old code of sending a null request (since none of the other fields in TetheringRequest are used), making this a no-op change. Bug: 216524590 Test: covered by existing tests Change-Id: I5ac83c74f5bc4aec2fe85daa9477740749d95c63 --- Tethering/src/android/net/ip/IpServer.java | 6 +- .../networkstack/tethering/Tethering.java | 120 ++++++++---------- .../unit/src/android/net/ip/IpServerTest.java | 37 ++++-- 3 files changed, 80 insertions(+), 83 deletions(-) diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java index 6229f6dc4d..8dd7ef2ce4 100644 --- a/Tethering/src/android/net/ip/IpServer.java +++ b/Tethering/src/android/net/ip/IpServer.java @@ -430,7 +430,7 @@ public class IpServer extends StateMachineShim { } /** Enable this IpServer. IpServer state machine will be tethered or localHotspot state. */ - public void enable(final int requestedState, final TetheringRequest request) { + public void enable(final int requestedState, @NonNull final TetheringRequest request) { sendMessage(CMD_TETHER_REQUESTED, requestedState, 0, request); } @@ -1026,11 +1026,11 @@ public class IpServer extends StateMachineShim { mLinkProperties.setInterfaceName(mIfaceName); } - private void maybeConfigureStaticIp(final TetheringRequest request) { + private void maybeConfigureStaticIp(@NonNull final TetheringRequest request) { // Ignore static address configuration if they are invalid or null. In theory, static // addresses should not be invalid here because TetheringManager do not allow caller to // specify invalid static address configuration. - if (request == null || request.getLocalIpv4Address() == null + if (request.getLocalIpv4Address() == null || request.getClientStaticIpv4Address() == null || !checkStaticAddressConfiguration( request.getLocalIpv4Address(), request.getClientStaticIpv4Address())) { return; diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 1c8ea3a1b0..b53a763fc7 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -28,6 +28,7 @@ import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.EXTRA_NETWORK_INFO; import static android.net.NetworkStack.PERMISSION_MAINLINE_NETWORK_STACK; import static android.net.TetheringManager.ACTION_TETHER_STATE_CHANGED; +import static android.net.TetheringManager.CONNECTIVITY_SCOPE_GLOBAL; import static android.net.TetheringManager.CONNECTIVITY_SCOPE_LOCAL; import static android.net.TetheringManager.EXTRA_ACTIVE_LOCAL_ONLY; import static android.net.TetheringManager.EXTRA_ACTIVE_TETHER; @@ -944,9 +945,9 @@ public class Tethering { public void onAvailable(String iface) { if (this != mBluetoothCallback) return; - final TetheringRequest request = getPendingTetheringRequest(TETHERING_BLUETOOTH); - enableIpServing(request, TETHERING_BLUETOOTH, iface, - getRequestedState(TETHERING_BLUETOOTH)); + final TetheringRequest request = + getOrCreatePendingTetheringRequest(TETHERING_BLUETOOTH); + enableIpServing(request, iface); mConfiguredBluetoothIface = iface; } @@ -1002,9 +1003,8 @@ public class Tethering { return; } - final TetheringRequest request = getPendingTetheringRequest(TETHERING_ETHERNET); - enableIpServing(request, TETHERING_ETHERNET, iface, - getRequestedState(TETHERING_ETHERNET)); + final TetheringRequest request = getOrCreatePendingTetheringRequest(TETHERING_ETHERNET); + enableIpServing(request, iface); mConfiguredEthernetIface = iface; } @@ -1025,12 +1025,8 @@ public class Tethering { } else { mConfiguredVirtualIface = iface; } - final TetheringRequest request = getPendingTetheringRequest(TETHERING_VIRTUAL); - enableIpServing( - request, - TETHERING_VIRTUAL, - mConfiguredVirtualIface, - getRequestedState(TETHERING_VIRTUAL)); + final TetheringRequest request = getOrCreatePendingTetheringRequest(TETHERING_VIRTUAL); + enableIpServing(request, mConfiguredVirtualIface); } else if (mConfiguredVirtualIface != null) { ensureIpServerStopped(mConfiguredVirtualIface); mConfiguredVirtualIface = null; @@ -1040,12 +1036,12 @@ public class Tethering { /** * Create a legacy tethering request for calls to the legacy tether() API, which doesn't take an - * explicit request. + * explicit request. These are always CONNECTIVITY_SCOPE_GLOBAL, per historical behavior. */ - private TetheringRequest createLegacyTetheringRequest(int type, int connectivityScope) { + private TetheringRequest createLegacyGlobalScopeTetheringRequest(int type) { final TetheringRequest request = new TetheringRequest.Builder(type).build(); request.getParcel().requestType = TetheringRequest.REQUEST_TYPE_LEGACY; - request.getParcel().connectivityScope = connectivityScope; + request.getParcel().connectivityScope = CONNECTIVITY_SCOPE_GLOBAL; return request; } @@ -1067,7 +1063,8 @@ public class Tethering { * Note: There are edge cases where the pending request is absent and we must temporarily * synthesize a placeholder request, such as if stopTethering was called before link layer * went up, or if the link layer goes up without us poking it (e.g. adb shell cmd wifi - * start-softap). + * start-softap). These placeholder requests only specify the tethering type and the + * default connectivity scope. */ @NonNull private TetheringRequest getOrCreatePendingTetheringRequest(int type) { @@ -1098,7 +1095,8 @@ public class Tethering { } catch (RemoteException e) { } } - int result = tetherInternal(null, iface, IpServer.STATE_TETHERED); + final TetheringRequest request = createLegacyGlobalScopeTetheringRequest(type); + int result = tetherInternal(request, iface); switch (type) { case TETHERING_WIFI: TerribleErrorLog.logTerribleError(TetheringStatsLog::write, @@ -1150,11 +1148,7 @@ public class Tethering { mHandler.post(() -> handleLegacyTether(iface, listener)); } - // TODO: is it possible to make the request @NonNull here? - // Because the IpServer doesn't actually look at (almost anything in) the request, we can just - // make callers synthesize a reasonable-looking request and make this @NonNull. - private int tetherInternal(@Nullable TetheringRequest request, String iface, - int requestedState) { + private int tetherInternal(@NonNull TetheringRequest request, String iface) { if (DBG) Log.d(TAG, "Tethering " + iface); TetherState tetherState = mTetherStates.get(iface); if (tetherState == null) { @@ -1171,15 +1165,9 @@ public class Tethering { // 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. - // - // TODO: once the request is @NonNull, it will be correct to fetch the type from it, because - // the type used to fetch (or synthesize) the request is the same as the type passed in at - // IpServer creation time. - // BUG: the connectivity scope in the request is ignored. This matches current behaviour. - // TODO: see if the connectivity scope can be made (or is already) correct, and fetch the - // requested state from there. - final int type = tetherState.ipServer.interfaceType(); - mPendingTetheringRequests.remove(type); + mPendingTetheringRequests.remove(request.getTetheringType()); + final int requestedState = request.getConnectivityScope() == CONNECTIVITY_SCOPE_GLOBAL + ? IpServer.STATE_TETHERED : IpServer.STATE_LOCAL_ONLY; tetherState.ipServer.enable(requestedState, request); return TETHER_ERROR_NO_ERROR; } @@ -1239,23 +1227,6 @@ public class Tethering { return mEntitlementMgr.isTetherProvisioningRequired(cfg); } - // TODO: make this take a TetheringRequest instead. - private int getRequestedState(int type) { - final TetheringRequest request = mPendingTetheringRequests.get(type); - - // The request could have been deleted before we had a chance to complete it. - // If so, assume that the scope is the default scope for this tethering type. - // This likely doesn't matter - if the request has been deleted, then tethering is - // likely going to be stopped soon anyway. - final int connectivityScope = (request != null) - ? request.getConnectivityScope() - : TetheringRequest.getDefaultConnectivityScope(type); - - return connectivityScope == CONNECTIVITY_SCOPE_LOCAL - ? IpServer.STATE_LOCAL_ONLY - : IpServer.STATE_TETHERED; - } - private int getServedUsbType(boolean forNcmFunction) { // TETHERING_NCM is only used if the device does not use NCM for regular USB tethering. if (forNcmFunction && !mConfig.isUsingNcm()) return TETHERING_NCM; @@ -1621,17 +1592,13 @@ public class Tethering { return mPendingTetheringRequests.get(type, null); } - // TODO: make the request @NonNull and move the tetheringType and ipServingMode into it. - private void enableIpServing(@Nullable TetheringRequest request, int tetheringType, - String ifname, int ipServingMode) { - enableIpServing(request, tetheringType, ifname, ipServingMode, false /* isNcm */); + private void enableIpServing(@NonNull TetheringRequest request, String ifname) { + enableIpServing(request, ifname, false /* isNcm */); } - // TODO: make the request @NonNull and move the tetheringType and ipServingMode into it. - private void enableIpServing(@Nullable TetheringRequest request, int tetheringType, - String ifname, int ipServingMode, boolean isNcm) { - ensureIpServerStartedForType(ifname, tetheringType, isNcm); - if (tetherInternal(request, ifname, ipServingMode) != TETHER_ERROR_NO_ERROR) { + private void enableIpServing(@NonNull TetheringRequest request, String ifname, boolean isNcm) { + ensureIpServerStartedForType(ifname, request.getTetheringType(), isNcm); + if (tetherInternal(request, ifname) != TETHER_ERROR_NO_ERROR) { Log.e(TAG, "unable start tethering on iface " + ifname); } } @@ -1683,9 +1650,10 @@ public class Tethering { mLog.e(ifname + " is not a tetherable iface, ignoring"); return; } - // No need to look for active requests. There can never be explicit requests for + // No need to call getOrCreatePendingRequest. There can never be explicit requests for // TETHERING_WIFI_P2P because enableTetheringInternal ignores that type. - enableIpServing(null, type, ifname, IpServer.STATE_LOCAL_ONLY); + final TetheringRequest request = createImplicitLocalOnlyTetheringRequest(type); + enableIpServing(request, ifname); } private void disableWifiP2pIpServingIfNeeded(String ifname) { @@ -1707,16 +1675,20 @@ public class Tethering { // Map wifiIpMode values to IpServer.Callback serving states. TetheringRequest request; - final int ipServingMode; final int type; switch (wifiIpMode) { case IFACE_IP_MODE_TETHERED: - ipServingMode = IpServer.STATE_TETHERED; type = maybeInferWifiTetheringType(ifname); - request = getPendingTetheringRequest(type); + request = getOrCreatePendingTetheringRequest(type); + // Wifi requests will always have CONNECTIVITY_SCOPE_GLOBAL, because + // TetheringRequest.Builder will not allow callers to set CONNECTIVITY_SCOPE_LOCAL + // for TETHERING_WIFI. However, if maybeInferWifiTetheringType returns a non-Wifi + // type (which could happen on a pre-T implementation of Wi-Fi if the regexps are + // misconfigured), then force the connectivity scope to global in order to match the + // historical behavior. + request.getParcel().connectivityScope = CONNECTIVITY_SCOPE_GLOBAL; break; case IFACE_IP_MODE_LOCAL_ONLY: - ipServingMode = IpServer.STATE_LOCAL_ONLY; type = maybeInferWifiTetheringType(ifname); // BUG: this request is incorrect - instead of LOHS, it will reflect whatever // request (if any) is being processed for TETHERING_WIFI. However, this is the @@ -1724,6 +1696,21 @@ public class Tethering { // such request b) tetherinternal doesn't look at the connectivity scope of the // request, it takes the scope from requestedState. request = getPendingTetheringRequest(type); + if (request == null) { + request = createImplicitLocalOnlyTetheringRequest(TETHERING_WIFI); + } else { + // If we've taken this request from the pending requests, then force the + // connectivity scope to local so we start IpServer in local-only mode (this + // matches historical behavior). This should be OK since the connectivity scope + // is only used to start IpServer in the correct mode. + // TODO: This will break fuzzy-matching logic for start/stop tethering in the + // future. Figure out how to reconcile that with this forced scope. + // Possibly ignore the connectivity scope for wifi if both requests are + // explicit, since explicit Wifi requests may only have + // CONNECTIVITY_SCOPE_GLOBAL. Or possibly, don't add any edge case and + // treat it as a different request entirely. + request.getParcel().connectivityScope = CONNECTIVITY_SCOPE_LOCAL; + } break; default: mLog.e("Cannot enable IP serving in unknown WiFi mode: " + wifiIpMode); @@ -1738,7 +1725,7 @@ public class Tethering { } if (!TextUtils.isEmpty(ifname)) { - enableIpServing(request, type, ifname, ipServingMode); + enableIpServing(request, ifname); } else { mLog.e("Cannot enable IP serving on missing interface name"); } @@ -1759,7 +1746,6 @@ public class Tethering { // for both TETHERING_USB and TETHERING_NCM, so the local-only NCM interface will be // stopped immediately. final int tetheringType = getServedUsbType(forNcmFunction); - final int requestedState = getRequestedState(tetheringType); String[] ifaces = null; try { ifaces = mNetd.interfaceGetList(); @@ -1768,11 +1754,11 @@ public class Tethering { return; } - final TetheringRequest request = getPendingTetheringRequest(tetheringType); + final TetheringRequest request = getOrCreatePendingTetheringRequest(tetheringType); if (ifaces != null) { for (String iface : ifaces) { if (ifaceNameToType(iface) == tetheringType) { - enableIpServing(request, tetheringType, iface, requestedState, forNcmFunction); + enableIpServing(request, iface, forNcmFunction); return; } } diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index 680e81db05..c5c3ad13d9 100644 --- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -48,7 +48,6 @@ import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.clearInvocations; @@ -74,6 +73,7 @@ import android.net.LinkAddress; import android.net.LinkProperties; import android.net.MacAddress; import android.net.RouteInfo; +import android.net.TetheringManager.TetheringRequest; import android.net.dhcp.DhcpServerCallbacks; import android.net.dhcp.DhcpServingParamsParcel; import android.net.dhcp.IDhcpEventCallbacks; @@ -240,7 +240,8 @@ public class IpServerTest { Set upstreamAddresses, boolean usingLegacyDhcp, boolean usingBpfOffload) throws Exception { initStateMachine(interfaceType, usingLegacyDhcp, usingBpfOffload); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED, 0, + mock(TetheringRequest.class)); verify(mBpfCoordinator).addIpServer(mIpServer); if (upstreamIface != null) { InterfaceParams interfaceParams = mDependencies.getInterfaceParams(upstreamIface); @@ -345,7 +346,8 @@ public class IpServerTest { public void canBeTetheredAsBluetooth() throws Exception { initStateMachine(TETHERING_BLUETOOTH); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED, 0, + mock(TetheringRequest.class)); InOrder inOrder = inOrder(mCallback, mNetd, mRoutingCoordinatorManager); if (isAtLeastT()) { inOrder.verify(mRoutingCoordinatorManager) @@ -400,7 +402,8 @@ public class IpServerTest { public void canBeTetheredAsUsb() throws Exception { initStateMachine(TETHERING_USB); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED, 0, + mock(TetheringRequest.class)); InOrder inOrder = inOrder(mCallback, mNetd, mRoutingCoordinatorManager); inOrder.verify(mRoutingCoordinatorManager).requestStickyDownstreamAddress(anyInt(), eq(CONNECTIVITY_SCOPE_GLOBAL), any()); @@ -423,7 +426,8 @@ public class IpServerTest { public void canBeTetheredAsWifiP2p_NotUsingDedicatedIp() throws Exception { initStateMachine(TETHERING_WIFI_P2P); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY, 0, + mock(TetheringRequest.class)); InOrder inOrder = inOrder(mCallback, mNetd, mRoutingCoordinatorManager); inOrder.verify(mRoutingCoordinatorManager).requestStickyDownstreamAddress(anyInt(), eq(CONNECTIVITY_SCOPE_LOCAL), any()); @@ -447,7 +451,8 @@ public class IpServerTest { initStateMachine(TETHERING_WIFI_P2P, false /* usingLegacyDhcp */, DEFAULT_USING_BPF_OFFLOAD, true /* shouldEnableWifiP2pDedicatedIp */); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY, 0, + mock(TetheringRequest.class)); InOrder inOrder = inOrder(mCallback, mNetd, mRoutingCoordinatorManager); // When using WiFi P2p dedicated IP, the IpServer just picks the IP address without // requesting for it at RoutingCoordinatorManager. @@ -627,7 +632,8 @@ public class IpServerTest { initStateMachine(TETHERING_USB); doThrow(RemoteException.class).when(mNetd).tetherInterfaceAdd(IFACE_NAME); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED, 0, + mock(TetheringRequest.class)); InOrder usbTeardownOrder = inOrder(mNetd, mCallback); usbTeardownOrder.verify(mNetd).interfaceSetCfg( argThat(cfg -> IFACE_NAME.equals(cfg.ifName))); @@ -713,7 +719,8 @@ public class IpServerTest { @Test public void startsDhcpServerOnNcm() throws Exception { initStateMachine(TETHERING_NCM); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY, 0, + mock(TetheringRequest.class)); dispatchTetherConnectionChanged(UPSTREAM_IFACE); assertDhcpStarted(new IpPrefix("192.168.42.0/24")); @@ -722,7 +729,8 @@ public class IpServerTest { @Test public void testOnNewPrefixRequest() throws Exception { initStateMachine(TETHERING_NCM); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_LOCAL_ONLY, 0, + mock(TetheringRequest.class)); final IDhcpEventCallbacks eventCallbacks; final ArgumentCaptor dhcpEventCbsCaptor = @@ -911,7 +919,8 @@ public class IpServerTest { doNothing().when(mDependencies).makeDhcpServer(any(), mDhcpParamsCaptor.capture(), cbCaptor.capture()); initStateMachine(TETHERING_WIFI); - dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED); + dispatchCommand(IpServer.CMD_TETHER_REQUESTED, STATE_TETHERED, 0, + mock(TetheringRequest.class)); verify(mDhcpServer, never()).startWithCallbacks(any(), any()); // No stop dhcp server because dhcp server is not created yet. @@ -961,10 +970,12 @@ public class IpServerTest { * Send a command to the state machine under test, and run the event loop to idle. * * @param command One of the IpServer.CMD_* constants. - * @param arg1 An additional argument to pass. + * @param arg1 An additional argument to pass. + * @param arg2 An additional argument to pass. + * @param obj An additional object to pass. */ - private void dispatchCommand(int command, int arg1) { - mIpServer.sendMessage(command, arg1); + private void dispatchCommand(int command, int arg1, int arg2, Object obj) { + mIpServer.sendMessage(command, arg1, arg2, obj); mLooper.dispatchAll(); }