Skip to content
Snippets Groups Projects
Commit d196fab8 authored by Remi NGUYEN VAN's avatar Remi NGUYEN VAN Committed by Gerrit Code Review
Browse files

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

parents f0afc538 3b817cba
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
service/src/com/android/server/ConnectivityService.java
+ 2
− 2
View file @ d196fab8
......@@ -11418,7 +11418,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) {
mHandler.post(() -> {
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) {
mHandler.post(() -> {
for (NetworkAgentInfo nai : mNetworkAgentInfos) {
nai.clatd.interfaceRemoved(iface);
nai.clatd.handleInterfaceRemoved(iface);
}
});
}
......
service/src/com/android/server/connectivity/Nat464Xlat.java
+ 6
− 10
View file @ d196fab8
......@@ -483,8 +483,9 @@ public class Nat464Xlat {
/**
* 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
// 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.
......@@ -499,6 +500,7 @@ public class Nat464Xlat {
// 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,
// and possibly use a timeout (or provide some guarantees at the lower layer) to address #1.
ensureRunningOnHandlerThread();
if (!isStarting() || !up || !Objects.equals(mIface, iface)) {
return;
}
......@@ -519,8 +521,10 @@ public class Nat464Xlat {
/**
* 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)) {
return;
}
......@@ -536,14 +540,6 @@ public class Nat464Xlat {
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.
*/
......
tests/unit/java/com/android/server/ConnectivityServiceTest.java
+ 8
− 7
View file @ d196fab8
......@@ -10772,6 +10772,8 @@ public class ConnectivityServiceTest {
final RouteInfo ipv4Subnet = new RouteInfo(myIpv4, null, MOBILE_IFNAME);
final RouteInfo stackedDefault =
new RouteInfo((IpPrefix) null, myIpv4.getAddress(), CLAT_MOBILE_IFNAME);
final BaseNetdUnsolicitedEventListener netdUnsolicitedListener =
getRegisteredNetdUnsolicitedEventListener();
 
final NetworkRequest networkRequest = new NetworkRequest.Builder()
.addTransportType(TRANSPORT_CELLULAR)
......@@ -10839,7 +10841,6 @@ public class ConnectivityServiceTest {
assertRoutesRemoved(cellNetId, ipv4Subnet);
 
// When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started.
Nat464Xlat clat = getNat464Xlat(mCellAgent);
assertNull(mCm.getLinkProperties(mCellAgent.getNetwork()).getNat64Prefix());
mService.mResolverUnsolEventCallback.onNat64PrefixEvent(
makeNat64PrefixEvent(cellNetId, PREFIX_OPERATION_ADDED, kNat64PrefixString, 96));
......@@ -10850,7 +10851,8 @@ public class ConnectivityServiceTest {
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
 
// 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);
List<LinkProperties> stackedLps = mCm.getLinkProperties(mCellAgent.getNetwork())
.getStackedLinks();
......@@ -10896,7 +10898,7 @@ public class ConnectivityServiceTest {
kOtherNat64Prefix.toString());
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getNat64Prefix().equals(kOtherNat64Prefix));
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1);
assertRoutesAdded(cellNetId, stackedDefault);
......@@ -10924,7 +10926,7 @@ public class ConnectivityServiceTest {
assertRoutesRemoved(cellNetId, stackedDefault);
 
// 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();
verify(mMockNetd, times(1)).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME);
 
......@@ -10961,7 +10963,7 @@ public class ConnectivityServiceTest {
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
 
// 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,
cb -> cb.getLp().getStackedLinks().size() == 1
&& 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.
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
clat = getNat464Xlat(mCellAgent);
clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1
&& cb.getLp().getNat64Prefix().equals(kNat64Prefix));
......
tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
+ 20
− 29
View file @ d196fab8
......@@ -86,7 +86,6 @@ public class Nat464XlatTest {
@Mock ClatCoordinator mClatCoordinator;
TestLooper mLooper;
Handler mHandler;
NetworkAgentConfig mAgentConfig = new NetworkAgentConfig();
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) {
@Override protected int getNetId() {
return NETID;
......@@ -117,9 +124,6 @@ public class Nat464XlatTest {
@Before
public void setUp() throws Exception {
mLooper = new TestLooper();
mHandler = new Handler(mLooper.getLooper());
MockitoAnnotations.initMocks(this);
mNai.linkProperties = new LinkProperties();
......@@ -130,7 +134,6 @@ public class Nat464XlatTest {
markNetworkConnected();
when(mNai.connService()).thenReturn(mConnectivity);
when(mNai.netAgentConfig()).thenReturn(mAgentConfig);
when(mNai.handler()).thenReturn(mHandler);
final InterfaceConfigurationParcel mConfig = new InterfaceConfigurationParcel();
when(mNetd.interfaceGetCfg(eq(STACKED_IFACE))).thenReturn(mConfig);
mConfig.ipv4Addr = ADDR.getAddress().getHostAddress();
......@@ -272,8 +275,7 @@ public class Nat464XlatTest {
verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
......@@ -294,8 +296,7 @@ public class Nat464XlatTest {
// Verify the generated v6 is reset when clat is stopped.
assertNull(nat.mIPv6Address);
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.handleInterfaceRemoved(STACKED_IFACE);
verifyNoMoreInteractions(mNetd, mConnectivity);
}
......@@ -324,8 +325,7 @@ public class Nat464XlatTest {
verifyClatdStart(inOrder);
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
......@@ -344,10 +344,8 @@ public class Nat464XlatTest {
if (interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
nat.handleInterfaceRemoved(STACKED_IFACE);
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
}
assertTrue(c.getValue().getStackedLinks().isEmpty());
......@@ -361,15 +359,12 @@ public class Nat464XlatTest {
if (!interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.interfaceLinkStateChanged(STACKED_IFACE, false);
mLooper.dispatchNext();
nat.handleInterfaceRemoved(STACKED_IFACE);
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
}
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
......@@ -411,8 +406,7 @@ public class Nat464XlatTest {
verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives.
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
......@@ -421,8 +415,7 @@ public class Nat464XlatTest {
assertRunning(nat);
// Stacked interface removed notification arrives (clatd crashed, ...).
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.handleInterfaceRemoved(STACKED_IFACE);
verifyClatdStop(null /* inOrder */);
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
......@@ -457,12 +450,10 @@ public class Nat464XlatTest {
assertIdle(nat);
// In-flight interface up notification arrives: no-op
nat.interfaceLinkStateChanged(STACKED_IFACE, true);
mLooper.dispatchNext();
nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
// Interface removed notification arrives after stopClatd() takes effect: no-op.
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
nat.handleInterfaceRemoved(STACKED_IFACE);
assertIdle(nat);
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment