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
Merged-In: Ice536bcb269cc8b040c6e7a72c15d0bc8b5bd235
Change-Id: Ice536bcb269cc8b040c6e7a72c15d0bc8b5bd235

1 files changed, 38 insertions, 23 deletions

diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java
index 82465f8a093e..af2db4abde7b 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 @@ public class Nat464Xlat extends BaseNetworkObserver {
         }
         mIface = null;
         mBaseIface = null;
-        mState = State.IDLE;
         if (requiresClat(mNetwork)) {
             mState = State.DISCOVERING;
         } else {
             stopPrefixDiscovery();
-            mState = State.IDLE;
         }
     }
 
@@ -285,6 +283,7 @@ public class Nat464Xlat extends BaseNetworkObserver {
     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 @@ public class Nat464Xlat extends BaseNetworkObserver {
      * 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.
         }
     }