From f972edcae7386d0dfc596aaa01e97d039dd1e0e2 Mon Sep 17 00:00:00 2001 From: Christopher Wiley Date: Mon, 23 May 2016 16:17:30 -0700 Subject: [PATCH] Consolidate cleanup logic in TetherInterfaceSM.TetheredState This pushes all TetheredState cleanup logic into a single place. All new unittests fail without the changes to TetherInterfaceSM. Bug: 28915272 Test: Compiles, unittests pass, WiFi tethering continues to work. Change-Id: Ia7bf210e00e9a54f2797baebc2e5333ce314c947 (cherry picked from commit f54c5a932a9ac4a491ce775b21ff8288e40b5bad) --- .../TetherInterfaceStateMachine.java | 79 +++++++------------ .../TetherInterfaceStateMachineTest.java | 69 +++++++++++++--- 2 files changed, 86 insertions(+), 62 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 0dc910db94a10..e33406e67061f 100644 --- a/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java +++ b/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java @@ -128,14 +128,6 @@ public class TetherInterfaceStateMachine extends StateMachine { private void setLastError(int error) { mLastError = error; - - if (isErrored()) { - if (mUsb) { - // note everything's been unwound by this point so nothing to do on - // further error.. - configureUsbIface(false, mIfaceName); - } - } } public boolean isAvailable() { @@ -215,9 +207,7 @@ public class TetherInterfaceStateMachine extends StateMachine { public void enter() { if (mUsb) { if (!configureUsbIface(true, mIfaceName)) { - mTetherController.notifyInterfaceTetheringReadiness(false, TetherInterfaceStateMachine.this); setLastError(ConnectivityManager.TETHER_ERROR_IFACE_CFG_ERROR); - transitionTo(mInitialState); return; } @@ -228,12 +218,6 @@ public class TetherInterfaceStateMachine extends StateMachine { } catch (Exception e) { Log.e(TAG, "Error Tethering: " + e.toString()); setLastError(ConnectivityManager.TETHER_ERROR_TETHER_IFACE_ERROR); - - try { - mNMService.untetherInterface(mIfaceName); - } catch (Exception ee) { - Log.e(TAG, "Error untethering after failure!" + ee.toString()); - } transitionTo(mInitialState); return; } @@ -241,6 +225,28 @@ public class TetherInterfaceStateMachine extends StateMachine { mTetherController.sendTetherStateChangedBroadcast(); } + @Override + public void exit() { + mTetherController.notifyInterfaceTetheringReadiness(false, + TetherInterfaceStateMachine.this); + + // Note that at this point, we're leaving the tethered state. We can fail any + // of these operations, but it doesn't really change that we have to try them + // all in sequence. + cleanupUpstream(); + + try { + mNMService.untetherInterface(mIfaceName); + } catch (Exception ee) { + setLastError(ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR); + Log.e(TAG, "Failed to untether interface: " + ee.toString()); + } + + if (mUsb) { + configureUsbIface(false, mIfaceName); + } + } + private void cleanupUpstream() { if (mMyUpstreamIfaceName != null) { // note that we don't care about errors here. @@ -275,28 +281,12 @@ public class TetherInterfaceStateMachine extends StateMachine { boolean retValue = true; switch (message.what) { case CMD_TETHER_UNREQUESTED: + transitionTo(mInitialState); + if (DBG) Log.d(TAG, "Untethered (unrequested)" + mIfaceName); + break; case CMD_INTERFACE_DOWN: - cleanupUpstream(); - try { - mNMService.untetherInterface(mIfaceName); - } catch (Exception e) { - setLastErrorAndTransitionToInitialState( - ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR); - break; - } - mTetherController.notifyInterfaceTetheringReadiness(false, TetherInterfaceStateMachine.this); - if (message.what == CMD_TETHER_UNREQUESTED) { - if (mUsb) { - if (!configureUsbIface(false, mIfaceName)) { - setLastError( - ConnectivityManager.TETHER_ERROR_IFACE_CFG_ERROR); - } - } - transitionTo(mInitialState); - } else if (message.what == CMD_INTERFACE_DOWN) { - transitionTo(mUnavailableState); - } - if (DBG) Log.d(TAG, "Untethered " + mIfaceName); + transitionTo(mUnavailableState); + if (DBG) Log.d(TAG, "Untethered (ifdown)" + mIfaceName); break; case CMD_TETHER_CONNECTION_CHANGED: String newUpstreamIfaceName = (String)(message.obj); @@ -314,13 +304,6 @@ public class TetherInterfaceStateMachine extends StateMachine { newUpstreamIfaceName); } catch (Exception e) { Log.e(TAG, "Exception enabling Nat: " + e.toString()); - try { - mNMService.disableNat(mIfaceName, newUpstreamIfaceName); - } catch (Exception ee) {} - try { - mNMService.untetherInterface(mIfaceName); - } catch (Exception ee) {} - setLastError(ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR); transitionTo(mInitialState); return true; @@ -333,14 +316,6 @@ public class TetherInterfaceStateMachine extends StateMachine { case CMD_START_TETHERING_ERROR: case CMD_STOP_TETHERING_ERROR: case CMD_SET_DNS_FORWARDERS_ERROR: - cleanupUpstream(); - try { - mNMService.untetherInterface(mIfaceName); - } catch (Exception e) { - setLastErrorAndTransitionToInitialState( - ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR); - break; - } setLastErrorAndTransitionToInitialState( ConnectivityManager.TETHER_ERROR_MASTER_ERROR); break; diff --git a/services/tests/servicestests/src/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java b/services/tests/servicestests/src/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java index 2c54f3b869f01..441084696b268 100644 --- a/services/tests/servicestests/src/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java +++ b/services/tests/servicestests/src/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java @@ -18,6 +18,8 @@ package com.android.server.connectivity.tethering; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; @@ -54,7 +56,7 @@ public class TetherInterfaceStateMachineTest { private final TestLooper mLooper = new TestLooper(); private TetherInterfaceStateMachine mTestedSm; - private void initStateMachine(boolean isUsb) { + private void initStateMachine(boolean isUsb) throws Exception { mTestedSm = new TetherInterfaceStateMachine(IFACE_NAME, mLooper.getLooper(), isUsb, mNMService, mStatsService, mTetherHelper); mTestedSm.start(); @@ -62,15 +64,17 @@ public class TetherInterfaceStateMachineTest { // the test of the world that we've changed from an unknown to available state. mLooper.dispatchAll(); reset(mNMService, mStatsService, mTetherHelper); + when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration); } - private void initTetheredStateMachine(boolean isUsb, String upstreamIface) { + private void initTetheredStateMachine(boolean isUsb, String upstreamIface) throws Exception { initStateMachine(isUsb); dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); if (upstreamIface != null) { dispatchTetherConnectionChanged(upstreamIface); } reset(mNMService, mStatsService, mTetherHelper); + when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration); } @Before @@ -92,7 +96,7 @@ public class TetherInterfaceStateMachineTest { } @Test - public void shouldDoNothingUntilRequested() { + public void shouldDoNothingUntilRequested() throws Exception { initStateMachine(false); final int [] NOOP_COMMANDS = { TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED, @@ -112,7 +116,7 @@ public class TetherInterfaceStateMachineTest { } @Test - public void handlesImmediateInterfaceDown() { + public void handlesImmediateInterfaceDown() throws Exception { initStateMachine(false); dispatchCommand(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); verify(mTetherHelper).sendTetherStateChangedBroadcast(); @@ -124,7 +128,7 @@ public class TetherInterfaceStateMachineTest { } @Test - public void canBeTethered() throws RemoteException { + public void canBeTethered() throws Exception { initStateMachine(false); dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); InOrder inOrder = inOrder(mTetherHelper, mNMService); @@ -144,8 +148,8 @@ public class TetherInterfaceStateMachineTest { dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED); InOrder inOrder = inOrder(mNMService, mStatsService, mTetherHelper); - inOrder.verify(mNMService).untetherInterface(IFACE_NAME); inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); + inOrder.verify(mNMService).untetherInterface(IFACE_NAME); inOrder.verify(mTetherHelper).sendTetherStateChangedBroadcast(); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); assertTrue("Should be ready for tethering again", mTestedSm.isAvailable()); @@ -154,12 +158,10 @@ public class TetherInterfaceStateMachineTest { } @Test - public void canBeTetheredAsUsb() throws RemoteException { + public void canBeTetheredAsUsb() throws Exception { initStateMachine(true); - when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration); dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); - InOrder inOrder = inOrder(mTetherHelper, mNMService); inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(true, mTestedSm); inOrder.verify(mNMService).getInterfaceConfig(IFACE_NAME); @@ -211,11 +213,11 @@ public class TetherInterfaceStateMachineTest { dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED); InOrder inOrder = inOrder(mNMService, mStatsService, mTetherHelper); + inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); inOrder.verify(mStatsService).forceUpdate(); inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNMService).untetherInterface(IFACE_NAME); - inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); inOrder.verify(mTetherHelper).sendTetherStateChangedBroadcast(); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); assertTrue("Should be ready for tethering again", mTestedSm.isAvailable()); @@ -223,6 +225,53 @@ public class TetherInterfaceStateMachineTest { assertFalse("Should have no errors", mTestedSm.isErrored()); } + @Test + public void interfaceDownLeadsToUnavailable() throws Exception { + for (boolean shouldThrow : new boolean[]{true, false}) { + initTetheredStateMachine(true, null); + + if (shouldThrow) { + doThrow(RemoteException.class).when(mNMService).untetherInterface(IFACE_NAME); + } + dispatchCommand(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + usbTeardownOrder.verify(mInterfaceConfiguration).setInterfaceDown(); + usbTeardownOrder.verify(mNMService).setInterfaceConfig( + IFACE_NAME, mInterfaceConfiguration); + verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); + assertFalse("Should not be available", mTestedSm.isAvailable()); + assertFalse("Should not be tethered", mTestedSm.isTethered()); + } + } + + @Test + public void usbShouldBeTornDownOnTetherError() throws Exception { + initStateMachine(true); + + doThrow(RemoteException.class).when(mNMService).tetherInterface(IFACE_NAME); + dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + usbTeardownOrder.verify(mInterfaceConfiguration).setInterfaceDown(); + usbTeardownOrder.verify(mNMService).setInterfaceConfig( + IFACE_NAME, mInterfaceConfiguration); + // Initial call is when we transition to the tethered state on request. + verify(mTetherHelper).notifyInterfaceTetheringReadiness(true, mTestedSm); + // And this call is to notify that we really aren't requested tethering. + verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); + assertTrue("Expected to see an error reported", mTestedSm.isErrored()); + } + + @Test + public void shouldTearDownUsbOnUpstreamError() throws Exception { + initTetheredStateMachine(true, null); + + doThrow(RemoteException.class).when(mNMService).enableNat(anyString(), anyString()); + dispatchTetherConnectionChanged(UPSTREAM_IFACE); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + usbTeardownOrder.verify(mInterfaceConfiguration).setInterfaceDown(); + usbTeardownOrder.verify(mNMService).setInterfaceConfig(IFACE_NAME, mInterfaceConfiguration); + verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); + } /** * Send a command to the state machine under test, and run the event loop to idle.