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
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 37221a9..5e51579 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 @@
}
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());
}
- return;
+ 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());
+ }
}
@Override
@@ -306,6 +308,7 @@
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 0fa4545..2b31eb6 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 @@
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 @@
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