Merge "Nat464Xlat: internal state guards cleanup + state enum"

am: aee703e7e4

Change-Id: I188ac4c367db11cb33b67fe92df3a120e3c6fbce

services/core/java/com/android/server/connectivity/Nat464Xlat.java

@@ -16,8 +16,6 @@
 
 package com.android.server.connectivity;
 
-import java.net.Inet4Address;
-
 import android.net.InterfaceConfiguration;
 import android.net.ConnectivityManager;
 import android.net.LinkAddress;
@@ -33,6 +31,9 @@ import android.util.Slog;
 import com.android.server.net.BaseNetworkObserver;
 import com.android.internal.util.ArrayUtils;
 
+import java.net.Inet4Address;
+import java.util.Objects;
+
 /**
  * Class to manage a 464xlat CLAT daemon.
  *
@@ -60,21 +61,18 @@ public class Nat464Xlat extends BaseNetworkObserver {
     // The network we're running on, and its type.
     private final NetworkAgentInfo mNetwork;
 
-    // Internal state variables.
+    private enum State {
-    //
+        IDLE,       // start() not called. Base iface and stacked iface names are null.
-    // The possible states are:
+        STARTING,   // start() called. Base iface and stacked iface names are known.
-    //  - Idle: start() not called. Everything is null.
+        RUNNING;    // start() called, and the stacked iface is known to be up.
-    //  - Starting: start() called. Interfaces are non-null. isStarted() returns true.
+    }
-    //    mIsRunning is false.
+
-    //  - Running: start() called, and interfaceLinkStateChanged() told us that mIface is up.
-    //    mIsRunning is true.
-    //
     // Once mIface is non-null and isStarted() is true, methods called by ConnectivityService on
     // its handler thread must not modify any internal state variables; they are only updated by the
     // interface observers, called on the notification threads.
     private String mBaseIface;
     private String mIface;
-    private boolean mIsRunning;
+    private volatile State mState = State.IDLE;
 
     public Nat464Xlat(INetworkManagementService nmService, Handler handler, NetworkAgentInfo nai) {
         mNMService = nmService;
@@ -99,20 +97,36 @@ public class Nat464Xlat extends BaseNetworkObserver {
     }
 
     /**
-     * Determines whether clatd is started. Always true, except a) if start has not yet been called,
+     * @return true if clatd has been started and has not yet stopped.
-     * or b) if our interface was removed.
+     * A true result corresponds to internal states STARTING and RUNNING.
      */
     public boolean isStarted() {
-        return mIface != null;
+        return mState != State.IDLE;
+    }
+
+    /**
+     * @return true if clatd has been started and the stacked interface is up.
+     */
+    public boolean isRunning() {
+        return mState == State.RUNNING;
+    }
+
+    /**
+     * Sets internal state.
+     */
+    private void enterStartingState(String baseIface) {
+        mIface = CLAT_PREFIX + baseIface;
+        mBaseIface = baseIface;
+        mState = State.STARTING;
     }
 
     /**
      * Clears internal state. Must not be called by ConnectivityService.
      */
-    private void clear() {
+    private void enterIdleState() {
         mIface = null;
         mBaseIface = null;
-        mIsRunning = false;
+        mState = State.IDLE;
     }
 
     /**
@@ -136,19 +150,19 @@ public class Nat464Xlat extends BaseNetworkObserver {
             return;
         }
 
-        mBaseIface = mNetwork.linkProperties.getInterfaceName();
+        String baseIface = mNetwork.linkProperties.getInterfaceName();
-        if (mBaseIface == null) {
+        if (baseIface == null) {
             Slog.e(TAG, "startClat: Can't start clat on null interface");
             return;
         }
-        mIface = CLAT_PREFIX + mBaseIface;
+        // TODO: should we only do this if mNMService.startClatd() succeeds?
-        // From now on, isStarted() will return true.
+        enterStartingState(baseIface);
 
         Slog.i(TAG, "Starting clatd on " + mBaseIface);
         try {
             mNMService.startClatd(mBaseIface);
         } catch(RemoteException|IllegalStateException e) {
-            Slog.e(TAG, "Error starting clatd: " + e);
+            Slog.e(TAG, "Error starting clatd on " + mBaseIface, e);
         }
     }
 
@@ -156,18 +170,19 @@ public class Nat464Xlat extends BaseNetworkObserver {
      * Stops the clat daemon. Called by ConnectivityService on the handler thread.
      */
     public void stop() {
-        if (isStarted()) {
+        if (!isStarted()) {
-            Slog.i(TAG, "Stopping clatd");
+            Slog.e(TAG, "stopClat: already stopped or not started");
+            return;
+        }
+
+        Slog.i(TAG, "Stopping clatd on " + mBaseIface);
         try {
             mNMService.stopClatd(mBaseIface);
         } catch(RemoteException|IllegalStateException e) {
-                Slog.e(TAG, "Error stopping clatd: " + e);
+            Slog.e(TAG, "Error stopping clatd on " + mBaseIface, e);
         }
         // When clatd stops and its interface is deleted, interfaceRemoved() will notify
-            // ConnectivityService and call clear().
+        // ConnectivityService and call enterIdleState().
-        } else {
-            Slog.e(TAG, "clatd: already stopped");
-        }
     }
 
     private void updateConnectivityService(LinkProperties lp) {
@@ -183,16 +198,19 @@ public class Nat464Xlat extends BaseNetworkObserver {
      * has no idea that 464xlat is running on top of it.
      */
     public void fixupLinkProperties(LinkProperties oldLp) {
-        if (mNetwork.clatd != null &&
+        if (!isRunning()) {
-                mIsRunning &&
+            return;
-                mNetwork.linkProperties != null &&
+        }
-                !mNetwork.linkProperties.getAllInterfaceNames().contains(mIface)) {
+        LinkProperties lp = mNetwork.linkProperties;
+        if (lp == null || lp.getAllInterfaceNames().contains(mIface)) {
+            return;
+        }
+
         Slog.d(TAG, "clatd running, updating NAI for " + mIface);
         for (LinkProperties stacked: oldLp.getStackedLinks()) {
-                if (mIface.equals(stacked.getInterfaceName())) {
+            if (Objects.equals(mIface, stacked.getInterfaceName())) {
-                    mNetwork.linkProperties.addStackedLink(stacked);
+                lp.addStackedLink(stacked);
-                    break;
+                return;
-                }
             }
         }
     }
@@ -238,33 +256,42 @@ public class Nat464Xlat extends BaseNetworkObserver {
         }
     }
 
+    /**
+     * Adds stacked link on base link and transitions to Running state
+     * This is called by the InterfaceObserver on its own thread, so can race with stop().
+     */
     @Override
     public void interfaceLinkStateChanged(String iface, boolean up) {
-        // Called by the InterfaceObserver on its own thread, so can race with stop().
+        if (!isStarted() || !up || !Objects.equals(mIface, iface)) {
-        if (isStarted() && up && mIface.equals(iface)) {
+            return;
-            Slog.i(TAG, "interface " + iface + " is up, mIsRunning " + mIsRunning + "->true");
+        }
-
+        if (isRunning()) {
-            if (!mIsRunning) {
+            return;
+        }
         LinkAddress clatAddress = getLinkAddress(iface);
         if (clatAddress == null) {
             return;
         }
-                mIsRunning = true;
+        mState = State.RUNNING;
+        Slog.i(TAG, String.format("interface %s is up, adding stacked link %s on top of %s",
+                mIface, mIface, mBaseIface));
+
         maybeSetIpv6NdOffload(mBaseIface, false);
         LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
         lp.addStackedLink(makeLinkProperties(clatAddress));
-                Slog.i(TAG, "Adding stacked link " + mIface + " on top of " + mBaseIface);
         updateConnectivityService(lp);
     }
-        }
-    }
 
     @Override
     public void interfaceRemoved(String iface) {
-        if (isStarted() && mIface.equals(iface)) {
+        if (!isStarted() || !Objects.equals(mIface, iface)) {
-            Slog.i(TAG, "interface " + iface + " removed, mIsRunning " + mIsRunning + "->false");
+            return;
+        }
+        if (!isRunning()) {
+            return;
+        }
 
-            if (mIsRunning) {
+        Slog.i(TAG, "interface " + iface + " removed");
         // The interface going away likely means clatd has crashed. Ask netd to stop it,
         // because otherwise when we try to start it again on the same base interface netd
         // will complain that it's already started.
@@ -281,14 +308,12 @@ public class Nat464Xlat extends BaseNetworkObserver {
         maybeSetIpv6NdOffload(mBaseIface, true);
         LinkProperties lp = new LinkProperties(mNetwork.linkProperties);
         lp.removeStackedLink(mIface);
-                clear();
+        enterIdleState();
         updateConnectivityService(lp);
     }
-        }
-    }
 
     @Override
     public String toString() {
-        return "mBaseIface: " + mBaseIface + ", mIface: " + mIface + ", mIsRunning: " + mIsRunning;
+        return "mBaseIface: " + mBaseIface + ", mIface: " + mIface + ", mState: " + mState;
     }
 }