Change Nat464Xlat lifecycle.
Currently, NetworkAgentInfo keeps a Nat464Xlat object only on
networks where we're starting clatd (i.e., IPv6-only networks).
Simplify this code by making the Nat464Xlat object final and
always non-null. This allows us to use Nat464Xlat to store
information, such as the NAT64 prefix, even if the clat daemon
has not been started yet.
Also, remove the STOPPING state which waits for the interface to
be removed. Instead, when stop() is called, immediately enter the
IDLE state.
- This is necessary in order to be able to call start() again
before the interface removal notification has arrived.
- It's also arguably more correct than the current code,
because when stop() returns clatd has already terminated
(ClatdController::stopClatd calls waitpid), and thus the tun
fd is already closed and the tun interface is gone.
Also, now that Nat464Xlat objects are reused after stop(), add
test coverage for calling start() after stop, in both cases:
- The notification that the previous interface was removed
arrives before the second start().
- The notification that the previous interface was removed
arrives after the second start() but before the notification
that the second interface was added.
Also fix a couple of lint warnings.
Test: builds, boots
Test: atest FrameworksNetTests
Test: clatd stops when IPv4 address added
Test: clatd restarts after "adb shell killall clatd"
Change-Id: I3dc66d155aa27606681f3473daf2170434d8c6d0
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index b5fcde4..113d92c 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -2877,7 +2877,7 @@
e.rethrowFromSystemServer();
}
mNetworkAgentInfos.remove(nai.messenger);
- nai.maybeStopClat();
+ nai.updateClat();
synchronized (mNetworkForNetId) {
// Remove the NetworkAgent, but don't mark the netId as
// available until we've told netd to delete it below.
@@ -5185,11 +5185,10 @@
LinkProperties oldLp) {
int netId = networkAgent.network.netId;
- // The NetworkAgentInfo does not know whether clatd is running on its network or not. Before
- // we do anything else, make sure its LinkProperties are accurate.
- if (networkAgent.clatd != null) {
- networkAgent.clatd.fixupLinkProperties(oldLp, newLp);
- }
+ // The NetworkAgentInfo does not know whether clatd is running on its network or not, or
+ // whether there is a NAT64 prefix. Before we do anything else, make sure its LinkProperties
+ // are accurate.
+ networkAgent.clatd.fixupLinkProperties(oldLp, newLp);
updateInterfaces(newLp, oldLp, netId, networkAgent.networkCapabilities);
updateMtu(newLp, oldLp);
@@ -5220,7 +5219,7 @@
networkAgent.linkProperties = newLp;
}
// Start or stop clat accordingly to network state.
- networkAgent.updateClat(mNMS);
+ networkAgent.updateClat();
notifyIfacesChangedForNetworkStats();
if (networkAgent.everConnected) {
try {
diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java
index 9d9b1cf..c834eaa 100644
--- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java
+++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java
@@ -70,8 +70,6 @@
IDLE, // start() not called. Base iface and stacked iface names are null.
STARTING, // start() called. Base iface and stacked iface names are known.
RUNNING, // start() called, and the stacked iface is known to be up.
- STOPPING; // stop() called, this Nat464Xlat is still registered as a network observer for
- // the stacked interface.
}
private String mBaseIface;
@@ -124,27 +122,19 @@
}
/**
- * @return true if clatd has been stopped.
- */
- public boolean isStopping() {
- return mState == State.STOPPING;
- }
-
- /**
* Start clatd, register this Nat464Xlat as a network observer for the stacked interface,
* and set internal state.
*/
private void enterStartingState(String baseIface) {
try {
mNMService.registerObserver(this);
- } catch(RemoteException e) {
- Slog.e(TAG,
- "startClat: Can't register interface observer for clat on " + mNetwork.name());
+ } catch (RemoteException e) {
+ Slog.e(TAG, "Can't register interface observer for clat on " + mNetwork.name());
return;
}
try {
mNetd.clatdStart(baseIface);
- } catch(RemoteException|IllegalStateException e) {
+ } catch (RemoteException | IllegalStateException e) {
Slog.e(TAG, "Error starting clatd on " + baseIface, e);
}
mIface = CLAT_PREFIX + baseIface;
@@ -161,28 +151,14 @@
}
/**
- * Stop clatd, and turn ND offload on if it had been turned off.
- */
- private void enterStoppingState() {
- try {
- mNetd.clatdStop(mBaseIface);
- } catch(RemoteException|IllegalStateException e) {
- Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
- }
-
- mState = State.STOPPING;
- }
-
- /**
* Unregister as a base observer for the stacked interface, and clear internal state.
*/
private void enterIdleState() {
try {
mNMService.unregisterObserver(this);
- } catch(RemoteException|IllegalStateException e) {
+ } catch (RemoteException | IllegalStateException e) {
Slog.e(TAG, "Error unregistering clatd observer on " + mBaseIface, e);
}
-
mIface = null;
mBaseIface = null;
mState = State.IDLE;
@@ -217,14 +193,30 @@
*/
public void stop() {
if (!isStarted()) {
+ Slog.e(TAG, "stopClat: already stopped");
return;
}
- Slog.i(TAG, "Stopping clatd on " + mBaseIface);
- boolean wasStarting = isStarting();
- enterStoppingState();
- if (wasStarting) {
- enterIdleState();
+ Slog.i(TAG, "Stopping clatd on " + mBaseIface);
+ try {
+ mNetd.clatdStop(mBaseIface);
+ } catch (RemoteException | IllegalStateException e) {
+ Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
+ }
+
+ String iface = mIface;
+ boolean wasRunning = isRunning();
+
+ // Enter IDLE state before updating LinkProperties. handleUpdateLinkProperties ends up
+ // calling fixupLinkProperties, and if at that time the state is still RUNNING,
+ // fixupLinkProperties would wrongly inform ConnectivityService that there is still a
+ // stacked interface.
+ enterIdleState();
+
+ if (wasRunning) {
+ LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
+ lp.removeStackedLink(iface);
+ mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
}
}
@@ -272,7 +264,7 @@
try {
InterfaceConfiguration config = mNMService.getInterfaceConfig(iface);
return config.getLinkAddress();
- } catch(RemoteException|IllegalStateException e) {
+ } catch (RemoteException | IllegalStateException e) {
Slog.e(TAG, "Error getting link properties: " + e);
return null;
}
@@ -307,20 +299,16 @@
if (!Objects.equals(mIface, iface)) {
return;
}
- if (!isRunning() && !isStopping()) {
+ if (!isRunning()) {
return;
}
Slog.i(TAG, "interface " + iface + " removed");
- if (!isStopping()) {
- // Ensure clatd is stopped if stop() has not been called: this likely means that clatd
- // has crashed.
- enterStoppingState();
- }
- enterIdleState();
- LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
- lp.removeStackedLink(iface);
- mNetwork.connService().handleUpdateLinkProperties(mNetwork, lp);
+ // If we're running, and the interface was removed, then we didn't call stop(), and it's
+ // likely that clatd crashed. Ensure we call stop() so we can start clatd again. Calling
+ // stop() will also update LinkProperties, and if clatd crashed, the LinkProperties update
+ // will cause ConnectivityService to call start() again.
+ stop();
}
@Override
diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java
index d0cff25..96f7283 100644
--- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java
+++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java
@@ -236,7 +236,7 @@
public final AsyncChannel asyncChannel;
// Used by ConnectivityService to keep track of 464xlat.
- public Nat464Xlat clatd;
+ public final Nat464Xlat clatd;
// Set after asynchronous creation of the NetworkMonitor.
private volatile INetworkMonitor mNetworkMonitor;
@@ -244,8 +244,6 @@
private static final String TAG = ConnectivityService.class.getSimpleName();
private static final boolean VDBG = false;
private final ConnectivityService mConnService;
- private final INetd mNetd;
- private final INetworkManagementService mNMS;
private final Context mContext;
private final Handler mHandler;
@@ -260,9 +258,8 @@
linkProperties = lp;
networkCapabilities = nc;
currentScore = score;
+ clatd = new Nat464Xlat(this, netd, nms);
mConnService = connService;
- mNetd = netd;
- mNMS = nms;
mContext = context;
mHandler = handler;
networkMisc = misc;
@@ -595,32 +592,15 @@
for (LingerTimer timer : mLingerTimers) { pw.println(timer); }
}
- public void updateClat(INetworkManagementService netd) {
- if (Nat464Xlat.requiresClat(this)) {
- maybeStartClat();
- } else {
- maybeStopClat();
+ /** Starts or stops clatd as required. */
+ public void updateClat() {
+ if (!clatd.isStarted() && Nat464Xlat.requiresClat(this)) {
+ clatd.start();
+ } else if (clatd.isStarted() && !Nat464Xlat.requiresClat(this)) {
+ clatd.stop();
}
}
- /** Ensure clat has started for this network. */
- public void maybeStartClat() {
- if (clatd != null && clatd.isStarted()) {
- return;
- }
- clatd = new Nat464Xlat(this, mNetd, mNMS);
- clatd.start();
- }
-
- /** Ensure clat has stopped for this network. */
- public void maybeStopClat() {
- if (clatd == null) {
- return;
- }
- clatd.stop();
- clatd = null;
- }
-
public String toString() {
return "NetworkAgentInfo{ ni{" + networkInfo + "} "
+ "network{" + network + "} nethandle{" + network.getNetworkHandle() + "} "
diff --git a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java
index 07b1d05..b9450b3 100644
--- a/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java
+++ b/tests/net/java/com/android/server/connectivity/Nat464XlatTest.java
@@ -19,6 +19,7 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -43,6 +44,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
@@ -145,20 +147,106 @@
nat.stop();
verify(mNetd).clatdStop(eq(BASE_IFACE));
-
- // Stacked interface removed notification arrives.
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
-
- verify(mNms).unregisterObserver(eq(nat));
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
+ verify(mNms).unregisterObserver(eq(nat));
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
+ // Stacked interface removed notification arrives and is ignored.
+ nat.interfaceRemoved(STACKED_IFACE);
+ mLooper.dispatchNext();
+
verifyNoMoreInteractions(mNetd, mNms, mConnectivity);
}
+ private void checkStartStopStart(boolean interfaceRemovedFirst) throws Exception {
+ Nat464Xlat nat = makeNat464Xlat();
+ ArgumentCaptor<LinkProperties> c = ArgumentCaptor.forClass(LinkProperties.class);
+ InOrder inOrder = inOrder(mNetd, mConnectivity);
+
+ // ConnectivityService starts clat.
+ nat.start();
+
+ inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE));
+
+ // Stacked interface up notification arrives.
+ nat.interfaceLinkStateChanged(STACKED_IFACE, true);
+ mLooper.dispatchNext();
+
+ inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
+ assertFalse(c.getValue().getStackedLinks().isEmpty());
+ assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+ assertRunning(nat);
+
+ // ConnectivityService stops clat (Network disconnects, IPv4 addr appears, ...).
+ nat.stop();
+
+ inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE));
+
+ inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
+ assertTrue(c.getValue().getStackedLinks().isEmpty());
+ assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+ assertIdle(nat);
+
+ if (interfaceRemovedFirst) {
+ // Stacked interface removed notification arrives and is ignored.
+ nat.interfaceRemoved(STACKED_IFACE);
+ mLooper.dispatchNext();
+ nat.interfaceLinkStateChanged(STACKED_IFACE, false);
+ mLooper.dispatchNext();
+ }
+
+ assertTrue(c.getValue().getStackedLinks().isEmpty());
+ assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+ assertIdle(nat);
+ inOrder.verifyNoMoreInteractions();
+
+ // ConnectivityService starts clatd again.
+ nat.start();
+
+ inOrder.verify(mNetd).clatdStart(eq(BASE_IFACE));
+
+ if (!interfaceRemovedFirst) {
+ // Stacked interface removed notification arrives and is ignored.
+ nat.interfaceRemoved(STACKED_IFACE);
+ mLooper.dispatchNext();
+ nat.interfaceLinkStateChanged(STACKED_IFACE, false);
+ mLooper.dispatchNext();
+ }
+
+ // Stacked interface up notification arrives.
+ nat.interfaceLinkStateChanged(STACKED_IFACE, true);
+ mLooper.dispatchNext();
+
+ inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
+ assertFalse(c.getValue().getStackedLinks().isEmpty());
+ assertTrue(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+ assertRunning(nat);
+
+ // ConnectivityService stops clat again.
+ nat.stop();
+
+ inOrder.verify(mNetd).clatdStop(eq(BASE_IFACE));
+
+ inOrder.verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
+ assertTrue(c.getValue().getStackedLinks().isEmpty());
+ assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
+ assertIdle(nat);
+
+ inOrder.verifyNoMoreInteractions();
+ }
+
+ @Test
+ public void testStartStopStart() throws Exception {
+ checkStartStopStart(true);
+ }
+
+ @Test
+ public void testStartStopStartBeforeInterfaceRemoved() throws Exception {
+ checkStartStopStart(false);
+ }
+
@Test
public void testClatdCrashWhileRunning() throws Exception {
Nat464Xlat nat = makeNat464Xlat();
@@ -184,9 +272,9 @@
nat.interfaceRemoved(STACKED_IFACE);
mLooper.dispatchNext();
- verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
+ verify(mNms).unregisterObserver(eq(nat));
assertTrue(c.getValue().getStackedLinks().isEmpty());
assertFalse(c.getValue().getAllInterfaceNames().contains(STACKED_IFACE));
assertIdle(nat);
@@ -210,8 +298,8 @@
// ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
nat.stop();
- verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
+ verify(mNms).unregisterObserver(eq(nat));
assertIdle(nat);
// In-flight interface up notification arrives: no-op
@@ -241,8 +329,8 @@
// ConnectivityService immediately stops clat (Network disconnects, IPv4 addr appears, ...)
nat.stop();
- verify(mNms).unregisterObserver(eq(nat));
verify(mNetd).clatdStop(eq(BASE_IFACE));
+ verify(mNms).unregisterObserver(eq(nat));
assertIdle(nat);
verifyNoMoreInteractions(mNetd, mNms, mConnectivity);
