From 3b817cbad34300490c089269875bf06dd98b4319 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 26 Sep 2023 13:40:13 +0900 Subject: [PATCH] Nat464Xlat: rely on netd events being called on handler thread. aosp/2724918 moved processing netd events for 464xlat to the handler thread. This CL makes 464xlat run the code that processes those events inline. This simplifies the code and makes the ordering of events more similar to what it was before. Bug: 293965195 Fix: 302071735 Test: existing unit tests Change-Id: I18b0d491aff94646b878a3d3488b5519fd42783c --- .../android/server/ConnectivityService.java | 4 +- .../server/connectivity/Nat464Xlat.java | 16 +++--- .../server/ConnectivityServiceTest.java | 15 +++--- .../server/connectivity/Nat464XlatTest.java | 49 ++++++++----------- 4 files changed, 36 insertions(+), 48 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 85507f6d16..c2da84d324 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -11353,7 +11353,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); } }); } @@ -11362,7 +11362,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); } }); } diff --git a/service/src/com/android/server/connectivity/Nat464Xlat.java b/service/src/com/android/server/connectivity/Nat464Xlat.java index f9e07fdbd3..065922dcd8 100644 --- a/service/src/com/android/server/connectivity/Nat464Xlat.java +++ b/service/src/com/android/server/connectivity/Nat464Xlat.java @@ -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. */ diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 2fccdcb9a4..c8cbce1ef7 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -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 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)); diff --git a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java index 58c0114761..2fe871389a 100644 --- a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java +++ b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java @@ -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);