LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge "Nat464Xlat: rely on netd events being called on handler thread." into main

Browse Source
This commit is contained in:
Remi NGUYEN VAN
2023-09-27 02:01:58 +00:00
committed by Gerrit Code Review
parent f0afc53836 3b817cbad3
commit d196fab898
4 changed files with 36 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
service/src/com/android/server/ConnectivityService.java
View File

@@ -11418,7 +11418,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) { public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) {
mHandler.post(() -> { mHandler.post(() -> {
for (NetworkAgentInfo nai : mNetworkAgentInfos) { for (NetworkAgentInfo nai : mNetworkAgentInfos) {
nai.clatd.interfaceLinkStateChanged(iface, up); nai.clatd.handleInterfaceLinkStateChanged(iface, up);
} }
}); });
} }
@@ -11427,7 +11427,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
public void onInterfaceRemoved(@NonNull String iface) { public void onInterfaceRemoved(@NonNull String iface) {
mHandler.post(() -> { mHandler.post(() -> {
for (NetworkAgentInfo nai : mNetworkAgentInfos) { for (NetworkAgentInfo nai : mNetworkAgentInfos) {
nai.clatd.interfaceRemoved(iface); nai.clatd.handleInterfaceRemoved(iface);
} }
}); });
} }

16
service/src/com/android/server/connectivity/Nat464Xlat.java
View File

@@ -483,8 +483,9 @@ public class Nat464Xlat {
/** /**
* Adds stacked link on base link and transitions to RUNNING state. * Adds stacked link on base link and transitions to RUNNING state.
* Must be called on the handler thread.
*/ */
private void handleInterfaceLinkStateChanged(String iface, boolean up) { public void handleInterfaceLinkStateChanged(String iface, boolean up) {
// TODO: if we call start(), then stop(), then start() again, and the // TODO: if we call start(), then stop(), then start() again, and the
// interfaceLinkStateChanged notification for the first start is delayed past the first // interfaceLinkStateChanged notification for the first start is delayed past the first
// stop, then the code becomes out of sync with system state and will behave incorrectly. // stop, then the code becomes out of sync with system state and will behave incorrectly.
@@ -499,6 +500,7 @@ public class Nat464Xlat {
// Once this code is converted to StateMachine, it will be possible to use deferMessage to // Once this code is converted to StateMachine, it will be possible to use deferMessage to
// ensure it stays in STARTING state until the interfaceLinkStateChanged notification fires, // ensure it stays in STARTING state until the interfaceLinkStateChanged notification fires,
// and possibly use a timeout (or provide some guarantees at the lower layer) to address #1. // and possibly use a timeout (or provide some guarantees at the lower layer) to address #1.
ensureRunningOnHandlerThread();
if (!isStarting() || !up || !Objects.equals(mIface, iface)) { if (!isStarting() || !up || !Objects.equals(mIface, iface)) {
return; return;
} }
@@ -519,8 +521,10 @@ public class Nat464Xlat {
/** /**
* Removes stacked link on base link and transitions to IDLE state. * Removes stacked link on base link and transitions to IDLE state.
* Must be called on the handler thread.
*/ */
private void handleInterfaceRemoved(String iface) { public void handleInterfaceRemoved(String iface) {
ensureRunningOnHandlerThread();
if (!Objects.equals(mIface, iface)) { if (!Objects.equals(mIface, iface)) {
return; return;
} }
@@ -536,14 +540,6 @@ public class Nat464Xlat {
stop(); stop();
} }
public void interfaceLinkStateChanged(String iface, boolean up) {
mNetwork.handler().post(() -> { handleInterfaceLinkStateChanged(iface, up); });
}
public void interfaceRemoved(String iface) {
mNetwork.handler().post(() -> handleInterfaceRemoved(iface));
}
/** /**
* Translate the input v4 address to v6 clat address. * Translate the input v4 address to v6 clat address.
*/ */

15
tests/unit/java/com/android/server/ConnectivityServiceTest.java
View File

@@ -10772,6 +10772,8 @@ public class ConnectivityServiceTest {
final RouteInfo ipv4Subnet = new RouteInfo(myIpv4, null, MOBILE_IFNAME); final RouteInfo ipv4Subnet = new RouteInfo(myIpv4, null, MOBILE_IFNAME);
final RouteInfo stackedDefault = final RouteInfo stackedDefault =
new RouteInfo((IpPrefix) null, myIpv4.getAddress(), CLAT_MOBILE_IFNAME); new RouteInfo((IpPrefix) null, myIpv4.getAddress(), CLAT_MOBILE_IFNAME);
final BaseNetdUnsolicitedEventListener netdUnsolicitedListener =
getRegisteredNetdUnsolicitedEventListener();
final NetworkRequest networkRequest = new NetworkRequest.Builder() final NetworkRequest networkRequest = new NetworkRequest.Builder()
.addTransportType(TRANSPORT_CELLULAR) .addTransportType(TRANSPORT_CELLULAR)
@@ -10839,7 +10841,6 @@ public class ConnectivityServiceTest {
assertRoutesRemoved(cellNetId, ipv4Subnet); assertRoutesRemoved(cellNetId, ipv4Subnet);
// When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started. // When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started.
Nat464Xlat clat = getNat464Xlat(mCellAgent);
assertNull(mCm.getLinkProperties(mCellAgent.getNetwork()).getNat64Prefix()); assertNull(mCm.getLinkProperties(mCellAgent.getNetwork()).getNat64Prefix());
mService.mResolverUnsolEventCallback.onNat64PrefixEvent( mService.mResolverUnsolEventCallback.onNat64PrefixEvent(
makeNat64PrefixEvent(cellNetId, PREFIX_OPERATION_ADDED, kNat64PrefixString, 96)); makeNat64PrefixEvent(cellNetId, PREFIX_OPERATION_ADDED, kNat64PrefixString, 96));
@@ -10850,7 +10851,8 @@ public class ConnectivityServiceTest {
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
// Clat iface comes up. Expect stacked link to be added. // Clat iface comes up. Expect stacked link to be added.
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); netdUnsolicitedListener.onInterfaceLinkStateChanged(
CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent);
List<LinkProperties> stackedLps = mCm.getLinkProperties(mCellAgent.getNetwork()) List<LinkProperties> stackedLps = mCm.getLinkProperties(mCellAgent.getNetwork())
.getStackedLinks(); .getStackedLinks();
@@ -10896,7 +10898,7 @@ public class ConnectivityServiceTest {
kOtherNat64Prefix.toString()); kOtherNat64Prefix.toString());
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getNat64Prefix().equals(kOtherNat64Prefix)); cb -> cb.getLp().getNat64Prefix().equals(kOtherNat64Prefix));
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1); cb -> cb.getLp().getStackedLinks().size() == 1);
assertRoutesAdded(cellNetId, stackedDefault); assertRoutesAdded(cellNetId, stackedDefault);
@@ -10924,7 +10926,7 @@ public class ConnectivityServiceTest {
assertRoutesRemoved(cellNetId, stackedDefault); assertRoutesRemoved(cellNetId, stackedDefault);
// The interface removed callback happens but has no effect after stop is called. // The interface removed callback happens but has no effect after stop is called.
clat.interfaceRemoved(CLAT_MOBILE_IFNAME); netdUnsolicitedListener.onInterfaceRemoved(CLAT_MOBILE_IFNAME);
networkCallback.assertNoCallback(); networkCallback.assertNoCallback();
verify(mMockNetd, times(1)).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME); verify(mMockNetd, times(1)).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME);
@@ -10961,7 +10963,7 @@ public class ConnectivityServiceTest {
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
// Clat iface comes up. Expect stacked link to be added. // Clat iface comes up. Expect stacked link to be added.
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1 cb -> cb.getLp().getStackedLinks().size() == 1
&& cb.getLp().getNat64Prefix() != null); && cb.getLp().getNat64Prefix() != null);
@@ -11029,8 +11031,7 @@ public class ConnectivityServiceTest {
// Clatd is started and clat iface comes up. Expect stacked link to be added. // Clatd is started and clat iface comes up. Expect stacked link to be added.
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
clat = getNat464Xlat(mCellAgent); netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1 cb -> cb.getLp().getStackedLinks().size() == 1
&& cb.getLp().getNat64Prefix().equals(kNat64Prefix)); && cb.getLp().getNat64Prefix().equals(kNat64Prefix));

49
tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
View File

@@ -86,7 +86,6 @@ public class Nat464XlatTest {
@Mock ClatCoordinator mClatCoordinator; @Mock ClatCoordinator mClatCoordinator;
TestLooper mLooper; TestLooper mLooper;
Handler mHandler;
NetworkAgentConfig mAgentConfig = new NetworkAgentConfig(); NetworkAgentConfig mAgentConfig = new NetworkAgentConfig();
Nat464Xlat makeNat464Xlat(boolean isCellular464XlatEnabled) { Nat464Xlat makeNat464Xlat(boolean isCellular464XlatEnabled) {
@@ -96,6 +95,14 @@ public class Nat464XlatTest {
} }
}; };
// The test looper needs to be created here on the test case thread and not in setUp,
// because setUp and test cases are run in different threads. Creating the test looper in
// setUp would make Looper.getThread() return the setUp thread, which does not match the
// test case thread that is actually used to process the messages.
mLooper = new TestLooper();
final Handler handler = new Handler(mLooper.getLooper());
doReturn(handler).when(mNai).handler();
return new Nat464Xlat(mNai, mNetd, mDnsResolver, deps) { return new Nat464Xlat(mNai, mNetd, mDnsResolver, deps) {
@Override protected int getNetId() { @Override protected int getNetId() {
return NETID; return NETID;
@@ -117,9 +124,6 @@ public class Nat464XlatTest {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
mLooper = new TestLooper();
mHandler = new Handler(mLooper.getLooper());
MockitoAnnotations.initMocks(this); MockitoAnnotations.initMocks(this);
mNai.linkProperties = new LinkProperties(); mNai.linkProperties = new LinkProperties();
@@ -130,7 +134,6 @@ public class Nat464XlatTest {
markNetworkConnected(); markNetworkConnected();
when(mNai.connService()).thenReturn(mConnectivity); when(mNai.connService()).thenReturn(mConnectivity);
when(mNai.netAgentConfig()).thenReturn(mAgentConfig); when(mNai.netAgentConfig()).thenReturn(mAgentConfig);
when(mNai.handler()).thenReturn(mHandler);
final InterfaceConfigurationParcel mConfig = new InterfaceConfigurationParcel(); final InterfaceConfigurationParcel mConfig = new InterfaceConfigurationParcel();
when(mNetd.interfaceGetCfg(eq(STACKED_IFACE))).thenReturn(mConfig); when(mNetd.interfaceGetCfg(eq(STACKED_IFACE))).thenReturn(mConfig);
mConfig.ipv4Addr = ADDR.getAddress().getHostAddress(); mConfig.ipv4Addr = ADDR.getAddress().getHostAddress();
@@ -272,8 +275,7 @@ public class Nat464XlatTest {
verifyClatdStart(null /* inOrder */); verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives. // Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE)); verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -294,8 +296,7 @@ public class Nat464XlatTest {
// Verify the generated v6 is reset when clat is stopped. // Verify the generated v6 is reset when clat is stopped.
assertNull(nat.mIPv6Address); assertNull(nat.mIPv6Address);
// Stacked interface removed notification arrives and is ignored. // Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE); nat.handleInterfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
verifyNoMoreInteractions(mNetd, mConnectivity); verifyNoMoreInteractions(mNetd, mConnectivity);
} }
@@ -324,8 +325,7 @@ public class Nat464XlatTest {
verifyClatdStart(inOrder); verifyClatdStart(inOrder);
// Stacked interface up notification arrives. // Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getStackedLinks().isEmpty());
@@ -344,10 +344,8 @@ public class Nat464XlatTest {
if (interfaceRemovedFirst) { if (interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored. // Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE); nat.handleInterfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext(); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
} }
assertTrue(c.getValue().getStackedLinks().isEmpty()); assertTrue(c.getValue().getStackedLinks().isEmpty());
@@ -361,15 +359,12 @@ public class Nat464XlatTest {
if (!interfaceRemovedFirst) { if (!interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored. // Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE); nat.handleInterfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext(); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
} }
// Stacked interface up notification arrives. // Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty()); assertFalse(c.getValue().getStackedLinks().isEmpty());
@@ -411,8 +406,7 @@ public class Nat464XlatTest {
verifyClatdStart(null /* inOrder */); verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives. // Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE)); verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture()); verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -421,8 +415,7 @@ public class Nat464XlatTest {
assertRunning(nat); assertRunning(nat);
// Stacked interface removed notification arrives (clatd crashed, ...). // Stacked interface removed notification arrives (clatd crashed, ...).
nat.interfaceRemoved(STACKED_IFACE); nat.handleInterfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
verifyClatdStop(null /* inOrder */); verifyClatdStop(null /* inOrder */);
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture()); verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -457,12 +450,10 @@ public class Nat464XlatTest {
assertIdle(nat); assertIdle(nat);
// In-flight interface up notification arrives: no-op // In-flight interface up notification arrives: no-op
nat.interfaceLinkStateChanged(STACKED_IFACE, true); nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
// Interface removed notification arrives after stopClatd() takes effect: no-op. // Interface removed notification arrives after stopClatd() takes effect: no-op.
nat.interfaceRemoved(STACKED_IFACE); nat.handleInterfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
assertIdle(nat); assertIdle(nat);