From d985dde8ccfd38a50377a8ab2378b36c92b0d7c7 Mon Sep 17 00:00:00 2001 From: Christopher Wiley Date: Tue, 31 May 2016 10:44:35 -0700 Subject: [PATCH] Fix race conditions between Tethering and TetherInterfaceStateMachine ( cherry-pick of de4819dc82b734036b22416228327d2712e01884 ) Previously, Tethering would answer requests for current tethering state by calling methods on instances of TetherInterfaceStateMachine to build up that state. This is incorrect, since state queries can come in on an arbitrary thread, independent of updates to the state machines instances. Fix this by: - Altering TetherInterfaceStateMachine to push consistent state snapshots to Tethering via notifyInterfaceStateChange() - Storing the last state snapshot in Tethering.mTetherStates - Removing public methods to query TetherInterfaceStateMachine state. - Consistently synchronizing access to Tethering.mTetherStates Bug: 29009601 Test: WiFi Tethering continues to work, unittests continue to pass Change-Id: Ied334f5e8739bc3aff1b08a1079095b9cc2a7958 --- .../server/connectivity/Tethering.java | 287 ++++++++++-------- .../tethering/IControlsTethering.java | 16 +- .../TetherInterfaceStateMachine.java | 74 ++--- .../TetherInterfaceStateMachineTest.java | 85 ++---- 4 files changed, 230 insertions(+), 232 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Tethering.java b/services/core/java/com/android/server/connectivity/Tethering.java index 4a0e81b42734c..bef48d6734fa0 100644 --- a/services/core/java/com/android/server/connectivity/Tethering.java +++ b/services/core/java/com/android/server/connectivity/Tethering.java @@ -81,8 +81,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.Iterator; -import java.util.Map; -import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -126,7 +124,18 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering private final INetworkStatsService mStatsService; private final Looper mLooper; - private Map mIfaces; // all tethered/tetherable ifaces + private static class TetherState { + public final TetherInterfaceStateMachine mStateMachine; + public int mLastState; + public int mLastError; + public TetherState(TetherInterfaceStateMachine sm) { + mStateMachine = sm; + // Assume all state machines start out available and with no errors. + mLastState = IControlsTethering.STATE_AVAILABLE; + mLastError = ConnectivityManager.TETHER_ERROR_NO_ERROR; + } + } + private final ArrayMap mTetherStates; private final BroadcastReceiver mStateReceiver; @@ -174,7 +183,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering mPublicSync = new Object(); - mIfaces = new ArrayMap(); + mTetherStates = new ArrayMap<>(); // make our own thread so we don't anr the system mLooper = IoThread.get().getLooper(); @@ -255,22 +264,20 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering return; } - TetherInterfaceStateMachine sm = mIfaces.get(iface); + TetherState tetherState = mTetherStates.get(iface); if (up) { - if (sm == null) { - sm = new TetherInterfaceStateMachine(iface, mLooper, interfaceType, - mNMService, mStatsService, this); - mIfaces.put(iface, sm); - sm.start(); + if (tetherState == null) { + trackNewTetherableInterface(iface, interfaceType); } } else { if (interfaceType == ConnectivityManager.TETHERING_USB) { // ignore usb0 down after enabling RNDIS // we will handle disconnect in interfaceRemoved instead if (VDBG) Log.d(TAG, "ignore interface down for " + iface); - } else if (sm != null) { - sm.sendMessage(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); - mIfaces.remove(iface); + } else if (tetherState != null) { + tetherState.mStateMachine.sendMessage( + TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); + mTetherStates.remove(iface); } } } @@ -329,15 +336,12 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering return; } - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm != null) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { + trackNewTetherableInterface(iface, interfaceType); + } else { if (VDBG) Log.d(TAG, "active iface (" + iface + ") reported as added, ignoring"); - return; } - sm = new TetherInterfaceStateMachine(iface, mLooper, interfaceType, - mNMService, mStatsService, this); - mIfaces.put(iface, sm); - sm.start(); } } @@ -345,15 +349,15 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering public void interfaceRemoved(String iface) { if (VDBG) Log.d(TAG, "interfaceRemoved " + iface); synchronized (mPublicSync) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm == null) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { if (VDBG) { Log.e(TAG, "attempting to remove unknown iface (" + iface + "), ignoring"); } return; } - sm.sendMessage(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); - mIfaces.remove(iface); + tetherState.mStateMachine.sendMessage(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); + mTetherStates.remove(iface); } } @@ -582,19 +586,18 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering public int tether(String iface) { if (DBG) Log.d(TAG, "Tethering " + iface); synchronized (mPublicSync) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm == null) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { Log.e(TAG, "Tried to Tether an unknown iface :" + iface + ", ignoring"); return ConnectivityManager.TETHER_ERROR_UNKNOWN_IFACE; } // Ignore the error status of the interface. If the interface is available, // the errors are referring to past tethering attempts anyway. - if (!sm.isAvailable()) { + if (tetherState.mLastState != IControlsTethering.STATE_AVAILABLE) { Log.e(TAG, "Tried to Tether an unavailable iface :" + iface + ", ignoring"); return ConnectivityManager.TETHER_ERROR_UNAVAIL_IFACE; - } - sm.sendMessage(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); + tetherState.mStateMachine.sendMessage(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); return ConnectivityManager.TETHER_ERROR_NO_ERROR; } } @@ -602,43 +605,43 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering public int untether(String iface) { if (DBG) Log.d(TAG, "Untethering " + iface); synchronized (mPublicSync) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm == null) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { Log.e(TAG, "Tried to Untether an unknown iface :" + iface + ", ignoring"); return ConnectivityManager.TETHER_ERROR_UNKNOWN_IFACE; } - if (!sm.isTethered()) { - Log.e(TAG, "Tried to Untethered an errored iface :" + iface + ", ignoring"); + if (tetherState.mLastState != IControlsTethering.STATE_TETHERED) { + Log.e(TAG, "Tried to untether an untethered iface :" + iface + ", ignoring"); return ConnectivityManager.TETHER_ERROR_UNAVAIL_IFACE; } - sm.sendMessage(TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED); + tetherState.mStateMachine.sendMessage( + TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED); return ConnectivityManager.TETHER_ERROR_NO_ERROR; } } public void untetherAll() { - if (DBG) Log.d(TAG, "Untethering " + mIfaces); - for (String iface : mIfaces.keySet()) { - untether(iface); + synchronized (mPublicSync) { + if (DBG) Log.d(TAG, "Untethering " + mTetherStates.keySet()); + for (int i = 0; i < mTetherStates.size(); i++) { + untether(mTetherStates.keyAt(i)); + } } } public int getLastTetherError(String iface) { synchronized (mPublicSync) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm == null) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState == null) { Log.e(TAG, "Tried to getLastTetherError on an unknown iface :" + iface + ", ignoring"); return ConnectivityManager.TETHER_ERROR_UNKNOWN_IFACE; } - return sm.getLastError(); + return tetherState.mLastError; } } - // TODO - move all private methods used only by the state machine into the state machine - // to clarify what needs synchronized protection. - @Override - public void sendTetherStateChangedBroadcast() { + private void sendTetherStateChangedBroadcast() { if (!getConnectivityManager().isTetheringSupported()) return; ArrayList availableList = new ArrayList(); @@ -650,24 +653,22 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering boolean bluetoothTethered = false; synchronized (mPublicSync) { - Set ifaces = mIfaces.keySet(); - for (String iface : ifaces) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm != null) { - if (sm.isErrored()) { - erroredList.add(iface); - } else if (sm.isAvailable()) { - availableList.add(iface); - } else if (sm.isTethered()) { - if (isUsb(iface)) { - usbTethered = true; - } else if (isWifi(iface)) { - wifiTethered = true; - } else if (isBluetooth(iface)) { - bluetoothTethered = true; - } - activeList.add(iface); + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + String iface = mTetherStates.keyAt(i); + if (tetherState.mLastError != ConnectivityManager.TETHER_ERROR_NO_ERROR) { + erroredList.add(iface); + } else if (tetherState.mLastState == IControlsTethering.STATE_AVAILABLE) { + availableList.add(iface); + } else if (tetherState.mLastState == IControlsTethering.STATE_TETHERED) { + if (isUsb(iface)) { + usbTethered = true; + } else if (isWifi(iface)) { + wifiTethered = true; + } else if (isBluetooth(iface)) { + bluetoothTethered = true; } + activeList.add(iface); } } } @@ -961,37 +962,27 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering public String[] getTetheredIfaces() { ArrayList list = new ArrayList(); synchronized (mPublicSync) { - Set keys = mIfaces.keySet(); - for (String key : keys) { - TetherInterfaceStateMachine sm = mIfaces.get(key); - if (sm.isTethered()) { - list.add(key); + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.mLastState == IControlsTethering.STATE_TETHERED) { + list.add(mTetherStates.keyAt(i)); } } } - String[] retVal = new String[list.size()]; - for (int i=0; i < list.size(); i++) { - retVal[i] = list.get(i); - } - return retVal; + return list.toArray(new String[list.size()]); } public String[] getTetherableIfaces() { ArrayList list = new ArrayList(); synchronized (mPublicSync) { - Set keys = mIfaces.keySet(); - for (String key : keys) { - TetherInterfaceStateMachine sm = mIfaces.get(key); - if (sm.isAvailable()) { - list.add(key); + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.mLastState == IControlsTethering.STATE_AVAILABLE) { + list.add(mTetherStates.keyAt(i)); } } } - String[] retVal = new String[list.size()]; - for (int i=0; i < list.size(); i++) { - retVal[i] = list.get(i); - } - return retVal; + return list.toArray(new String[list.size()]); } public String[] getTetheredDhcpRanges() { @@ -1001,19 +992,14 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering public String[] getErroredIfaces() { ArrayList list = new ArrayList(); synchronized (mPublicSync) { - Set keys = mIfaces.keySet(); - for (String key : keys) { - TetherInterfaceStateMachine sm = mIfaces.get(key); - if (sm.isErrored()) { - list.add(key); + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.mLastError != ConnectivityManager.TETHER_ERROR_NO_ERROR) { + list.add(mTetherStates.keyAt(i)); } } } - String[] retVal = new String[list.size()]; - for (int i= 0; i< list.size(); i++) { - retVal[i] = list.get(i); - } - return retVal; + return list.toArray(new String[list.size()]); } private void maybeLogMessage(State state, int what) { @@ -1143,6 +1129,18 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering private State mStopTetheringErrorState; private State mSetDnsForwardersErrorState; + // This list is a little subtle. It contains all the interfaces that currently are + // requesting tethering, regardless of whether these interfaces are still members of + // mTetherStates. This allows us to maintain the following predicates: + // + // 1) mTetherStates contains the set of all currently existing, tetherable, link state up + // interfaces. + // 2) mNotifyList contains all state machines that may have outstanding tethering state + // that needs to be torn down. + // + // Because we excise interfaces immediately from mTetherStates, we must maintain mNotifyList + // so that the garbage collector does not clean up the state machine before it has a chance + // to tear itself down. private ArrayList mNotifyList; private int mMobileApnReserved = ConnectivityManager.TYPE_NONE; @@ -1453,15 +1451,16 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering config_mobile_hotspot_provision_app_no_ui).isEmpty() == false) { ArrayList tethered = new ArrayList(); synchronized (mPublicSync) { - Set ifaces = mIfaces.keySet(); - for (String iface : ifaces) { - TetherInterfaceStateMachine sm = mIfaces.get(iface); - if (sm != null && sm.isTethered()) { - int interfaceType = ifaceNameToType(iface); - if (interfaceType != - ConnectivityManager.TETHERING_INVALID) { - tethered.add(new Integer(interfaceType)); - } + for (int i = 0; i < mTetherStates.size(); i++) { + TetherState tetherState = mTetherStates.valueAt(i); + if (tetherState.mLastState != + IControlsTethering.STATE_TETHERED) { + continue; // Skip interfaces that aren't tethered. + } + String iface = mTetherStates.keyAt(i); + int interfaceType = ifaceNameToType(iface); + if (interfaceType != ConnectivityManager.TETHERING_INVALID) { + tethered.add(new Integer(interfaceType)); } } } @@ -1487,9 +1486,6 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering } class InitialState extends TetherMasterUtilState { - @Override - public void enter() { - } @Override public boolean processMessage(Message message) { maybeLogMessage(this, message.what); @@ -1498,16 +1494,15 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering case CMD_TETHER_MODE_REQUESTED: TetherInterfaceStateMachine who = (TetherInterfaceStateMachine)message.obj; if (VDBG) Log.d(TAG, "Tether Mode requested by " + who); - mNotifyList.add(who); + if (mNotifyList.indexOf(who) < 0) { + mNotifyList.add(who); + } transitionTo(mTetherModeAliveState); break; case CMD_TETHER_MODE_UNREQUESTED: who = (TetherInterfaceStateMachine)message.obj; if (VDBG) Log.d(TAG, "Tether Mode unrequested by " + who); - int index = mNotifyList.indexOf(who); - if (index != -1) { - mNotifyList.remove(who); - } + mNotifyList.remove(who); break; default: retValue = false; @@ -1546,24 +1541,26 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering case CMD_TETHER_MODE_REQUESTED: TetherInterfaceStateMachine who = (TetherInterfaceStateMachine)message.obj; if (VDBG) Log.d(TAG, "Tether Mode requested by " + who); - mNotifyList.add(who); + if (mNotifyList.indexOf(who) < 0) { + mNotifyList.add(who); + } who.sendMessage(TetherInterfaceStateMachine.CMD_TETHER_CONNECTION_CHANGED, mCurrentUpstreamIface); break; case CMD_TETHER_MODE_UNREQUESTED: who = (TetherInterfaceStateMachine)message.obj; if (VDBG) Log.d(TAG, "Tether Mode unrequested by " + who); - int index = mNotifyList.indexOf(who); - if (index != -1) { + if (mNotifyList.remove(who)) { if (DBG) Log.d(TAG, "TetherModeAlive removing notifyee " + who); - mNotifyList.remove(index); if (mNotifyList.isEmpty()) { turnOffMasterTetherSettings(); // transitions appropriately } else { if (DBG) { Log.d(TAG, "TetherModeAlive still has " + mNotifyList.size() + " live requests:"); - for (Object o : mNotifyList) Log.d(TAG, " " + o); + for (TetherInterfaceStateMachine o : mNotifyList) { + Log.d(TAG, " " + o); + } } } } else { @@ -1623,8 +1620,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering } void notify(int msgType) { mErrorNotification = msgType; - for (Object o : mNotifyList) { - TetherInterfaceStateMachine sm = (TetherInterfaceStateMachine)o; + for (TetherInterfaceStateMachine sm : mNotifyList) { sm.sendMessage(msgType); } } @@ -1707,8 +1703,26 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering pw.println("Tether state:"); pw.increaseIndent(); - for (Object o : mIfaces.values()) { - pw.println(o); + for (int i = 0; i < mTetherStates.size(); i++) { + final String iface = mTetherStates.keyAt(i); + final TetherState tetherState = mTetherStates.valueAt(i); + pw.print(iface + " - "); + + switch (tetherState.mLastState) { + case IControlsTethering.STATE_UNAVAILABLE: + pw.print("UnavailableState"); + break; + case IControlsTethering.STATE_AVAILABLE: + pw.print("AvailableState"); + break; + case IControlsTethering.STATE_TETHERED: + pw.print("TetheredState"); + break; + default: + pw.print("UnknownState"); + break; + } + pw.println(" - lastError = " + tetherState.mLastError); } pw.decreaseIndent(); } @@ -1716,9 +1730,40 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering } @Override - public void notifyInterfaceTetheringReadiness(boolean isReady, - TetherInterfaceStateMachine who) { - mTetherMasterSM.sendMessage((isReady) ? TetherMasterSM.CMD_TETHER_MODE_REQUESTED - : TetherMasterSM.CMD_TETHER_MODE_UNREQUESTED, who); + public void notifyInterfaceStateChange(String iface, TetherInterfaceStateMachine who, + int state, int error) { + synchronized (mPublicSync) { + TetherState tetherState = mTetherStates.get(iface); + if (tetherState != null && tetherState.mStateMachine.equals(who)) { + tetherState.mLastState = state; + tetherState.mLastError = error; + } else { + if (DBG) Log.d(TAG, "got notification from stale iface " + iface); + } + } + + if (DBG) { + Log.d(TAG, "iface " + iface + " notified that it was in state " + state + + " with error " + error); + } + + switch (state) { + case IControlsTethering.STATE_UNAVAILABLE: + case IControlsTethering.STATE_AVAILABLE: + mTetherMasterSM.sendMessage(TetherMasterSM.CMD_TETHER_MODE_UNREQUESTED, who); + break; + case IControlsTethering.STATE_TETHERED: + mTetherMasterSM.sendMessage(TetherMasterSM.CMD_TETHER_MODE_REQUESTED, who); + break; + } + sendTetherStateChangedBroadcast(); + } + + private void trackNewTetherableInterface(String iface, int interfaceType) { + TetherState tetherState; + tetherState = new TetherState(new TetherInterfaceStateMachine(iface, mLooper, + interfaceType, mNMService, mStatsService, this)); + mTetherStates.put(iface, tetherState); + tetherState.mStateMachine.start(); } } diff --git a/services/core/java/com/android/server/connectivity/tethering/IControlsTethering.java b/services/core/java/com/android/server/connectivity/tethering/IControlsTethering.java index 7677daf808e80..449b8a8354e7d 100644 --- a/services/core/java/com/android/server/connectivity/tethering/IControlsTethering.java +++ b/services/core/java/com/android/server/connectivity/tethering/IControlsTethering.java @@ -22,6 +22,18 @@ package com.android.server.connectivity.tethering; * Interface with methods necessary to notify that a given interface is ready for tethering. */ public interface IControlsTethering { - void sendTetherStateChangedBroadcast(); - void notifyInterfaceTetheringReadiness(boolean isReady, TetherInterfaceStateMachine who); + public final int STATE_UNAVAILABLE = 0; + public final int STATE_AVAILABLE = 1; + public final int STATE_TETHERED = 2; + + /** + * Notify that |who| has changed its tethering state. This may be called from any thread. + * + * @param iface a network interface (e.g. "wlan0") + * @param who corresponding instance of a TetherInterfaceStateMachine + * @param state one of IControlsTethering.STATE_* + * @param lastError one of ConnectivityManager.TETHER_ERROR_* + */ + void notifyInterfaceStateChange(String iface, TetherInterfaceStateMachine who, + int state, int lastError); } 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 b8bea60a5ccb4..aebeb690d7707 100644 --- a/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java +++ b/services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java @@ -27,7 +27,6 @@ import android.os.Message; import android.util.Log; import android.util.SparseArray; -import com.android.internal.util.IState; import com.android.internal.util.MessageUtils; import com.android.internal.util.Protocol; import com.android.internal.util.State; @@ -98,7 +97,7 @@ public class TetherInterfaceStateMachine extends StateMachine { mTetherController = tetherController; mIfaceName = ifaceName; mInterfaceType = interfaceType; - setLastError(ConnectivityManager.TETHER_ERROR_NO_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_NO_ERROR; mInitialState = new InitialState(); addState(mInitialState); @@ -110,40 +109,6 @@ public class TetherInterfaceStateMachine extends StateMachine { setInitialState(mInitialState); } - @Override - public String toString() { - String res = new String(); - res += mIfaceName + " - "; - IState current = getCurrentState(); - if (current == mInitialState) res += "InitialState"; - if (current == mTetheredState) res += "TetheredState"; - if (current == mUnavailableState) res += "UnavailableState"; - if (isAvailable()) res += " - Available"; - if (isTethered()) res += " - Tethered"; - res += " - lastError =" + getLastError(); - return res; - } - - public int getLastError() { - return mLastError; - } - - private void setLastError(int error) { - mLastError = error; - } - - public boolean isAvailable() { - return getCurrentState() == mInitialState; - } - - public boolean isTethered() { - return getCurrentState() == mTetheredState; - } - - public boolean isErrored() { - return (mLastError != ConnectivityManager.TETHER_ERROR_NO_ERROR); - } - // configured when we start tethering and unconfig'd on error or conclusion private boolean configureIfaceIp(boolean enabled) { if (VDBG) Log.d(TAG, "configureIfaceIp(" + enabled + ")"); @@ -193,7 +158,9 @@ public class TetherInterfaceStateMachine extends StateMachine { class InitialState extends State { @Override public void enter() { - mTetherController.sendTetherStateChangedBroadcast(); + mTetherController.notifyInterfaceStateChange( + mIfaceName, TetherInterfaceStateMachine.this, + IControlsTethering.STATE_AVAILABLE, mLastError); } @Override @@ -202,8 +169,7 @@ public class TetherInterfaceStateMachine extends StateMachine { boolean retValue = true; switch (message.what) { case CMD_TETHER_REQUESTED: - setLastError(ConnectivityManager.TETHER_ERROR_NO_ERROR); - mTetherController.notifyInterfaceTetheringReadiness(true, TetherInterfaceStateMachine.this); + mLastError = ConnectivityManager.TETHER_ERROR_NO_ERROR; transitionTo(mTetheredState); break; case CMD_INTERFACE_DOWN: @@ -221,7 +187,7 @@ public class TetherInterfaceStateMachine extends StateMachine { @Override public void enter() { if (!configureIfaceIp(true)) { - setLastError(ConnectivityManager.TETHER_ERROR_IFACE_CFG_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_IFACE_CFG_ERROR; transitionTo(mInitialState); return; } @@ -230,19 +196,18 @@ public class TetherInterfaceStateMachine extends StateMachine { mNMService.tetherInterface(mIfaceName); } catch (Exception e) { Log.e(TAG, "Error Tethering: " + e.toString()); - setLastError(ConnectivityManager.TETHER_ERROR_TETHER_IFACE_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_TETHER_IFACE_ERROR; transitionTo(mInitialState); return; } if (DBG) Log.d(TAG, "Tethered " + mIfaceName); - mTetherController.sendTetherStateChangedBroadcast(); + mTetherController.notifyInterfaceStateChange( + mIfaceName, TetherInterfaceStateMachine.this, + IControlsTethering.STATE_TETHERED, mLastError); } @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. @@ -251,7 +216,7 @@ public class TetherInterfaceStateMachine extends StateMachine { try { mNMService.untetherInterface(mIfaceName); } catch (Exception ee) { - setLastError(ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR; Log.e(TAG, "Failed to untether interface: " + ee.toString()); } @@ -315,7 +280,7 @@ public class TetherInterfaceStateMachine extends StateMachine { newUpstreamIfaceName); } catch (Exception e) { Log.e(TAG, "Exception enabling Nat: " + e.toString()); - setLastError(ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR; transitionTo(mInitialState); return true; } @@ -327,8 +292,8 @@ public class TetherInterfaceStateMachine extends StateMachine { case CMD_START_TETHERING_ERROR: case CMD_STOP_TETHERING_ERROR: case CMD_SET_DNS_FORWARDERS_ERROR: - setLastErrorAndTransitionToInitialState( - ConnectivityManager.TETHER_ERROR_MASTER_ERROR); + mLastError = ConnectivityManager.TETHER_ERROR_MASTER_ERROR; + transitionTo(mInitialState); break; default: retValue = false; @@ -348,13 +313,10 @@ public class TetherInterfaceStateMachine extends StateMachine { class UnavailableState extends State { @Override public void enter() { - setLastError(ConnectivityManager.TETHER_ERROR_NO_ERROR); - mTetherController.sendTetherStateChangedBroadcast(); + mLastError = ConnectivityManager.TETHER_ERROR_NO_ERROR; + mTetherController.notifyInterfaceStateChange( + mIfaceName, TetherInterfaceStateMachine.this, + IControlsTethering.STATE_UNAVAILABLE, mLastError); } } - - void setLastErrorAndTransitionToInitialState(int error) { - setLastError(error); - transitionTo(mInitialState); - } } 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 30a7dbc80977c..a30b3629d5c43 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 @@ -16,8 +16,6 @@ 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; @@ -26,6 +24,14 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import static android.net.ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR; +import static android.net.ConnectivityManager.TETHER_ERROR_NO_ERROR; +import static android.net.ConnectivityManager.TETHER_ERROR_TETHER_IFACE_ERROR; +import static android.net.ConnectivityManager.TETHER_ERROR_UNTETHER_IFACE_ERROR; +import static com.android.server.connectivity.tethering.IControlsTethering.STATE_AVAILABLE; +import static com.android.server.connectivity.tethering.IControlsTethering.STATE_TETHERED; +import static com.android.server.connectivity.tethering.IControlsTethering.STATE_UNAVAILABLE; + import android.net.ConnectivityManager; import android.net.INetworkStatsService; import android.net.InterfaceConfiguration; @@ -78,8 +84,7 @@ public class TetherInterfaceStateMachineTest { when(mNMService.getInterfaceConfig(IFACE_NAME)).thenReturn(mInterfaceConfiguration); } - @Before - public void setUp() throws Exception { + @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); } @@ -89,10 +94,8 @@ public class TetherInterfaceStateMachineTest { ConnectivityManager.TETHERING_BLUETOOTH, mNMService, mStatsService, mTetherHelper); mTestedSm.start(); mLooper.dispatchAll(); - assertTrue("Should start out available for tethering", mTestedSm.isAvailable()); - assertFalse("Should not be tethered initially", mTestedSm.isTethered()); - assertFalse("Should have no errors initially", mTestedSm.isErrored()); - verify(mTetherHelper).sendTetherStateChangedBroadcast(); + verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mTetherHelper, mNMService, mStatsService); } @@ -119,28 +122,23 @@ public class TetherInterfaceStateMachineTest { @Test public void handlesImmediateInterfaceDown() throws Exception { initStateMachine(ConnectivityManager.TETHERING_BLUETOOTH); + dispatchCommand(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); - verify(mTetherHelper).sendTetherStateChangedBroadcast(); + verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_UNAVAILABLE, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertFalse("Should not be tetherable when the interface is down", mTestedSm.isAvailable()); - assertFalse("Should not be tethered when the interface is down", mTestedSm.isTethered()); - assertFalse("Should have no errors when the interface goes immediately down", - mTestedSm.isErrored()); } @Test public void canBeTethered() throws Exception { initStateMachine(ConnectivityManager.TETHERING_BLUETOOTH); + dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); InOrder inOrder = inOrder(mTetherHelper, mNMService); - inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(true, mTestedSm); inOrder.verify(mNMService).tetherInterface(IFACE_NAME); - inOrder.verify(mTetherHelper).sendTetherStateChangedBroadcast(); - + inOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_TETHERED, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertFalse("Should not be tetherable when tethered", mTestedSm.isAvailable()); - assertTrue("Should be in a tethered state", mTestedSm.isTethered()); - assertFalse("Should have no errors when tethered", mTestedSm.isErrored()); } @Test @@ -149,13 +147,10 @@ public class TetherInterfaceStateMachineTest { dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_UNREQUESTED); InOrder inOrder = inOrder(mNMService, mStatsService, mTetherHelper); - inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); inOrder.verify(mNMService).untetherInterface(IFACE_NAME); - inOrder.verify(mTetherHelper).sendTetherStateChangedBroadcast(); + inOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertTrue("Should be ready for tethering again", mTestedSm.isAvailable()); - assertFalse("Should not be tethered", mTestedSm.isTethered()); - assertFalse("Should have no errors", mTestedSm.isErrored()); } @Test @@ -164,16 +159,12 @@ public class TetherInterfaceStateMachineTest { dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); InOrder inOrder = inOrder(mTetherHelper, mNMService); - inOrder.verify(mTetherHelper).notifyInterfaceTetheringReadiness(true, mTestedSm); inOrder.verify(mNMService).getInterfaceConfig(IFACE_NAME); inOrder.verify(mNMService).setInterfaceConfig(IFACE_NAME, mInterfaceConfiguration); inOrder.verify(mNMService).tetherInterface(IFACE_NAME); - inOrder.verify(mTetherHelper).sendTetherStateChangedBroadcast(); - + inOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_TETHERED, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertFalse("Should not be tetherable when tethered", mTestedSm.isAvailable()); - assertTrue("Should be in a tethered state", mTestedSm.isTethered()); - assertFalse("Should have no errors when tethered", mTestedSm.isErrored()); } @Test @@ -186,9 +177,6 @@ public class TetherInterfaceStateMachineTest { inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE); inOrder.verify(mNMService).startInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertFalse("Should not be tetherable when tethered", mTestedSm.isAvailable()); - assertTrue("Should be in a tethered state", mTestedSm.isTethered()); - assertFalse("Should have no errors when tethered", mTestedSm.isErrored()); } @Test @@ -203,9 +191,6 @@ public class TetherInterfaceStateMachineTest { inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE2); inOrder.verify(mNMService).startInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertFalse("Should not be tetherable when tethered", mTestedSm.isAvailable()); - assertTrue("Should be in a tethered state", mTestedSm.isTethered()); - assertFalse("Should have no errors when tethered", mTestedSm.isErrored()); } @Test @@ -214,16 +199,13 @@ 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).sendTetherStateChangedBroadcast(); + inOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_AVAILABLE, TETHER_ERROR_NO_ERROR); verifyNoMoreInteractions(mNMService, mStatsService, mTetherHelper); - assertTrue("Should be ready for tethering again", mTestedSm.isAvailable()); - assertFalse("Should not be tethered", mTestedSm.isTethered()); - assertFalse("Should have no errors", mTestedSm.isErrored()); } @Test @@ -235,13 +217,12 @@ public class TetherInterfaceStateMachineTest { doThrow(RemoteException.class).when(mNMService).untetherInterface(IFACE_NAME); } dispatchCommand(TetherInterfaceStateMachine.CMD_INTERFACE_DOWN); - InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration, mTetherHelper); 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()); + usbTeardownOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_UNAVAILABLE, TETHER_ERROR_NO_ERROR); } } @@ -251,15 +232,12 @@ public class TetherInterfaceStateMachineTest { doThrow(RemoteException.class).when(mNMService).tetherInterface(IFACE_NAME); dispatchCommand(TetherInterfaceStateMachine.CMD_TETHER_REQUESTED); - InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration, mTetherHelper); 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()); + usbTeardownOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_AVAILABLE, TETHER_ERROR_TETHER_IFACE_ERROR); } @Test @@ -268,10 +246,11 @@ public class TetherInterfaceStateMachineTest { doThrow(RemoteException.class).when(mNMService).enableNat(anyString(), anyString()); dispatchTetherConnectionChanged(UPSTREAM_IFACE); - InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration); + InOrder usbTeardownOrder = inOrder(mNMService, mInterfaceConfiguration, mTetherHelper); usbTeardownOrder.verify(mInterfaceConfiguration).setInterfaceDown(); usbTeardownOrder.verify(mNMService).setInterfaceConfig(IFACE_NAME, mInterfaceConfiguration); - verify(mTetherHelper).notifyInterfaceTetheringReadiness(false, mTestedSm); + usbTeardownOrder.verify(mTetherHelper).notifyInterfaceStateChange( + IFACE_NAME, mTestedSm, STATE_AVAILABLE, TETHER_ERROR_ENABLE_NAT_ERROR); } /**