Refactor the Nat464Xlat function for simplicity.
This makes the code easier to understand by making state
transitions more explicit. It also makes it easier to address a
TODO to turn the class into a StateMachine.
This should be an exact no-op refactoring. The current cases
covered by the code (all mutually exclusive) are:
1. requiresClat && !isPrefixDiscoveryStarted
Action: startPrefixDiscovery()
Equivalent to IDLE && requiresClat, because
isPrefixDiscoveryStarted returns true for every state except
IDLE.
2. requiresClat && isPrefixDiscoveryStarted && shouldStartClat
Action: start()
Equivalent to DISCOVERING && shouldStartClat,
because isPrefixDiscoveryStarted is true in DISCOVERING,
STARTING, and RUNNING, but start() does nothing if mState is
STARTING or RUNNING.
3. requiresClat && isPrefixDiscoveryStarted && !shouldStartClat
Action: stop()
Equivalent to (STARTING or RUNNING) && !shouldStartClat,
because isPrefixDiscoveryStarted is true in DISCOVERING,
STARTING, and RUNNING, but stop() does nothing if mState is
not STARTING or RUNNING.
4. !requiresClat && isStarted
Action: stop()
Equivalent to (STARTING or RUNNING) && !requiresClat,
because isStarted() is only true in STARTING and RUNNING.
5. !requiresClat && !isStarted && isPrefixDiscoveryStarted
Action: leaveStartedState()
Equivalent to DISCOVERING && !requiresClat, because
the only state with isPrefixDiscoveryStarted and !isStarted
is DISCOVERING.
Also, simplify case #5. In this case, calling leaveStartedState
is superfluous, because in the DISCOVERING state:
- There is no need to call unregisterObserver, since the observer
is only registered when entering STARTING and is unregistered
when going back to DISCOVERING or IDLE.
- mIface and mBaseIface don't need to be set to null because they
are only set to non-null when entering STARTING and nulled out
when going back to DISCOVERING or IDLE.
Bug: 126113090
Bug: 150648313
Test: covered by existing ConnectivityServiceTest and Nat464XlatTest
Change-Id: Ice536bcb269cc8b040c6e7a72c15d0bc8b5bd235
diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java
index 3b3c520..741cb5b 100644
--- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java
+++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java
@@ -213,12 +213,10 @@
}
mIface = null;
mBaseIface = null;
- mState = State.IDLE;
if (requiresClat(mNetwork)) {
mState = State.DISCOVERING;
} else {
stopPrefixDiscovery();
- mState = State.IDLE;
}
}
@@ -285,6 +283,7 @@
private void stopPrefixDiscovery() {
try {
mDnsResolver.stopPrefix64Discovery(getNetId());
+ mState = State.IDLE;
} catch (RemoteException | ServiceSpecificException e) {
Slog.e(TAG, "Error stopping prefix discovery on netId " + getNetId() + ": " + e);
}
@@ -294,27 +293,43 @@
* Starts/stops NAT64 prefix discovery and clatd as necessary.
*/
public void update() {
- // TODO: turn this class into a proper StateMachine. // http://b/126113090
- if (requiresClat(mNetwork)) {
- if (!isPrefixDiscoveryStarted()) {
- startPrefixDiscovery();
- } else if (shouldStartClat(mNetwork)) {
- // NAT64 prefix detected. Start clatd.
- // TODO: support the NAT64 prefix changing after it's been discovered. There is no
- // need to support this at the moment because it cannot happen without changes to
- // the Dns64Configuration code in netd.
- start();
- } else {
- // NAT64 prefix removed. Stop clatd and go back into DISCOVERING state.
- stop();
- }
- } else {
- // Network no longer requires clat. Stop clat and prefix discovery.
- if (isStarted()) {
- stop();
- } else if (isPrefixDiscoveryStarted()) {
- leaveStartedState();
- }
+ // TODO: turn this class into a proper StateMachine. http://b/126113090
+ switch (mState) {
+ case IDLE:
+ if (requiresClat(mNetwork)) {
+ // Network is detected to be IPv6-only.
+ // TODO: consider going to STARTING directly if the NAT64 prefix is already
+ // known. This would however result in clatd running without prefix discovery
+ // running, which might be a surprising combination.
+ startPrefixDiscovery(); // Enters DISCOVERING state.
+ return;
+ }
+ break;
+
+ case DISCOVERING:
+ if (shouldStartClat(mNetwork)) {
+ // NAT64 prefix detected. Start clatd.
+ start(); // Enters STARTING state.
+ return;
+ }
+ if (!requiresClat(mNetwork)) {
+ // IPv4 address added. Go back to IDLE state.
+ stopPrefixDiscovery();
+ return;
+ }
+ break;
+
+ case STARTING:
+ case RUNNING:
+ // NAT64 prefix removed, or IPv4 address added.
+ // Stop clatd and go back into DISCOVERING or idle.
+ if (!shouldStartClat(mNetwork)) {
+ stop();
+ }
+ break;
+ // TODO: support the NAT64 prefix changing after it's been discovered. There is
+ // no need to support this at the moment because it cannot happen without
+ // changes to the Dns64Configuration code in netd.
}
}