Cleanup in the face of upstream error

If either enableNat() or startInterfaceForwarding() fail, be sure
to cleanup any commands that might have succeeded.

Most of this change is a refactoring of cleanupUpstreamIface() into
two methods, one of which (cleanupUpstreamInterface()) is reused
in error handling scenarios.

Test: as follows
    - built (bullhead)
    - flashed
    - booted
    - runtest -x .../tethering/TetherInterfaceStateMachineTest.java passes
Bug: 32031803
Bug: 32163131

Change-Id: Ia4d56e03beeab1908d8b8c2202e94992f1aa58a4

2 files changed, 33 insertions, 32 deletions

diff --git a/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java b/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java
index 37221a971ad1..5e5157913f20 100644
--- a/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java
+++ b/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java
@@ -250,31 +250,33 @@ public class TetherInterfaceStateMachine extends StateMachine {
         }
 
         private void cleanupUpstream() {
-            if (mMyUpstreamIfaceName != null) {
-                // note that we don't care about errors here.
-                // sometimes interfaces are gone before we get
-                // to remove their rules, which generates errors.
-                // just do the best we can.
-                try {
-                    // about to tear down NAT; gather remaining statistics
-                    mStatsService.forceUpdate();
-                } catch (Exception e) {
-                    if (VDBG) Log.e(TAG, "Exception in forceUpdate: " + e.toString());
-                }
-                try {
-                    mNMService.stopInterfaceForwarding(mIfaceName, mMyUpstreamIfaceName);
-                } catch (Exception e) {
-                    if (VDBG) Log.e(
-                            TAG, "Exception in removeInterfaceForward: " + e.toString());
-                }
-                try {
-                    mNMService.disableNat(mIfaceName, mMyUpstreamIfaceName);
-                } catch (Exception e) {
-                    if (VDBG) Log.e(TAG, "Exception in disableNat: " + e.toString());
-                }
-                mMyUpstreamIfaceName = null;
+            if (mMyUpstreamIfaceName == null) return;
+
+            cleanupUpstreamInterface(mMyUpstreamIfaceName);
+            mMyUpstreamIfaceName = null;
+        }
+
+        private void cleanupUpstreamInterface(String upstreamIface) {
+            // Note that we don't care about errors here.
+            // Sometimes interfaces are gone before we get
+            // to remove their rules, which generates errors.
+            // Just do the best we can.
+            try {
+                // About to tear down NAT; gather remaining statistics.
+                mStatsService.forceUpdate();
+            } catch (Exception e) {
+                if (VDBG) Log.e(TAG, "Exception in forceUpdate: " + e.toString());
+            }
+            try {
+                mNMService.stopInterfaceForwarding(mIfaceName, upstreamIface);
+            } catch (Exception e) {
+                if (VDBG) Log.e(TAG, "Exception in removeInterfaceForward: " + e.toString());
+            }
+            try {
+                mNMService.disableNat(mIfaceName, upstreamIface);
+            } catch (Exception e) {
+                if (VDBG) Log.e(TAG, "Exception in disableNat: " + e.toString());
             }
-            return;
         }
 
         @Override
@@ -306,6 +308,7 @@ public class TetherInterfaceStateMachine extends StateMachine {
                                     newUpstreamIfaceName);
                         } catch (Exception e) {
                             Log.e(TAG, "Exception enabling Nat: " + e.toString());
+                            cleanupUpstreamInterface(newUpstreamIfaceName);
                             mLastError = ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR;
                             transitionTo(mInitialState);
                             return true;
diff --git a/tests/net/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java b/tests/net/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java
index 0fa45454fa70..2b31eb6bf825 100644
--- a/tests/net/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java
+++ b/tests/net/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java
@@ -210,10 +210,9 @@ public class TetherInterfaceStateMachineTest {
         inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE);
         inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE);
         inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE2);
-        // TODO: Verify proper cleanup is performed:
-        // inOrder.verify(mStatsService).forceUpdate();
-        // inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);
-        // inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);
+        inOrder.verify(mStatsService).forceUpdate();
+        inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);
+        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);
     }
 
     @Test
@@ -230,10 +229,9 @@ public class TetherInterfaceStateMachineTest {
         inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE);
         inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE2);
         inOrder.verify(mNMService).startInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);
-        // TODO: Verify proper cleanup is performed:
-        // inOrder.verify(mStatsService).forceUpdate();
-        // inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);
-        // inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);
+        inOrder.verify(mStatsService).forceUpdate();
+        inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);
+        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);
     }
 
     @Test