joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

no-op: Make TetheringRequest @NonNull in enableIpServing

Browse source 
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
This commit is contained in:
Quang Anh Luong 2025-01-28 18:57:15 +09:00
commit 2b8a42d666
3 changed files with 80 additions and 83 deletions
Download patch file Download diff file Expand all files Collapse all files

6
Tethering/src/android/net/ip/IpServer.java
View file

@ -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;

120
Tethering/src/com/android/networkstack/tethering/Tethering.java
View file

@ -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;
}
}

37
Tethering/tests/unit/src/android/net/ip/IpServerTest.java
View file

@ -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<LinkAddress> 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<IDhcpEventCallbacks> 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();
}