Consolidate cleanup logic in TetherInterfaceSM.TetheredState
( cherry-pick of f54c5a932a9ac4a491ce775b21ff8288e40b5bad )
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
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 0dc910d..e33406e6 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 @@
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 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 @@
} 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 @@
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 @@
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 @@
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 @@
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 2c54f3b..4410846 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 @@
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 @@
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 @@
// 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 @@
}
@Test
- public void shouldDoNothingUntilRequested() {
+ public void shouldDoNothingUntilRequested() throws Exception {
initStateMachine(false);
final int [] NOOP_COMMANDS = {
TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED,
@@ -112,7 +116,7 @@
}
@Test
- public void handlesImmediateInterfaceDown() {
+ public void handlesImmediateInterfaceDown() throws Exception {
initStateMachine(false);
dispatchCommand(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN);
verify(mTetherHelper).sendTetherStateChangedBroadcast();
@@ -124,7 +128,7 @@
}
@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 @@
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 @@
}
@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 @@
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 @@
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.