Address comments from previous change (320592)

Test: as follows
    - built (bullhead)
    - flashed
    - booted
    - runtest frameworks-net passes
    - vanilla wifi-to-mobile tethering works
Bug: 32163131

Change-Id: I8788cb0d93606a2893c3dbc4f22e72f450f319b8

diff --git a/services/core/java/com/android/server/connectivity/Tethering.java b/services/core/java/com/android/server/connectivity/Tethering.java
index 26ddc1a..de17740 100644 (file)
--- a/services/core/java/com/android/server/connectivity/Tethering.java
+++ b/services/core/java/com/android/server/connectivity/Tethering.java
@@ -911,10 +911,6 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
     public int setUsbTethering(boolean enable) {
         if (VDBG) Log.d(TAG, "setUsbTethering(" + enable + ")");
         UsbManager usbManager = mContext.getSystemService(UsbManager.class);
-        if (usbManager == null) {
-            return enable ? ConnectivityManager.TETHER_ERROR_MASTER_ERROR
-                          : ConnectivityManager.TETHER_ERROR_NO_ERROR;
-        }
 
         synchronized (mPublicSync) {
             if (enable) {
@@ -1103,7 +1099,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
         private final ArrayList<TetherInterfaceStateMachine> mNotifyList;
         private final IPv6TetheringCoordinator mIPv6TetheringCoordinator;
 
-        private int mPreviousMobileApn = ConnectivityManager.TYPE_NONE;
+        private int mPreviousMobileType = ConnectivityManager.TYPE_NONE;
 
         private static final int UPSTREAM_SETTLE_TIME_MS     = 10000;
 
@@ -1138,13 +1134,13 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
                 return false;
             }
 
-            protected boolean turnOnUpstreamMobileConnection(int apnType) {
+            protected boolean requestUpstreamMobileConnection(int apnType) {
                 if (apnType == ConnectivityManager.TYPE_NONE) { return false; }
 
-                if (apnType != mPreviousMobileApn) {
+                if (apnType != mPreviousMobileType) {
                     // Unregister any previous mobile upstream callback because
                     // this request, if any, will be different.
-                    turnOffUpstreamMobileConnection();
+                    unrequestUpstreamMobileConnection();
                 }
 
                 if (mUpstreamNetworkMonitor.mobileNetworkRequested()) {
@@ -1156,25 +1152,25 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
                     case ConnectivityManager.TYPE_MOBILE_DUN:
                     case ConnectivityManager.TYPE_MOBILE:
                     case ConnectivityManager.TYPE_MOBILE_HIPRI:
-                        mPreviousMobileApn = apnType;
+                        mPreviousMobileType = apnType;
                         break;
                     default:
                         return false;
                 }
 
-                // TODO: This should be called by the code that observes
-                // configuration changes, once the above code in this function
-                // is simplified (i.e. eradicated).
-                mUpstreamNetworkMonitor.mobileUpstreamRequiresDun(
+                // TODO: Replace this with a call to pass the current tethering
+                // configuration to mUpstreamNetworkMonitor and let it handle
+                // choosing APN type accordingly.
+                mUpstreamNetworkMonitor.updateMobileRequiresDun(
                         apnType == ConnectivityManager.TYPE_MOBILE_DUN);
 
                 mUpstreamNetworkMonitor.registerMobileNetworkRequest();
                 return true;
             }
 
-            protected void turnOffUpstreamMobileConnection() {
+            protected void unrequestUpstreamMobileConnection() {
                 mUpstreamNetworkMonitor.releaseMobileNetworkRequest();
-                mPreviousMobileApn = ConnectivityManager.TYPE_NONE;
+                mPreviousMobileType = ConnectivityManager.TYPE_NONE;
             }
 
             protected boolean turnOnMasterTetherSettings() {
@@ -1253,11 +1249,11 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
                     case ConnectivityManager.TYPE_MOBILE_DUN:
                     case ConnectivityManager.TYPE_MOBILE_HIPRI:
                         // If we're on DUN, put our own grab on it.
-                        turnOnUpstreamMobileConnection(upType);
+                        requestUpstreamMobileConnection(upType);
                         break;
                     case ConnectivityManager.TYPE_NONE:
                         if (tryCell &&
-                                turnOnUpstreamMobileConnection(mPreferredUpstreamMobileApn)) {
+                                requestUpstreamMobileConnection(mPreferredUpstreamMobileApn)) {
                             // We think mobile should be coming up; don't set a retry.
                         } else {
                             sendMessageDelayed(CMD_RETRY_UPSTREAM, UPSTREAM_SETTLE_TIME_MS);
@@ -1270,7 +1266,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
                          * If we found NONE we don't want to do this as we want any previous
                          * requests to keep trying to bring up something we can use.
                          */
-                        turnOffUpstreamMobileConnection();
+                        unrequestUpstreamMobileConnection();
                         break;
                 }
 
@@ -1491,7 +1487,7 @@ public class Tethering extends BaseNetworkObserver implements IControlsTethering
 
             @Override
             public void exit() {
-                turnOffUpstreamMobileConnection();
+                unrequestUpstreamMobileConnection();
                 mUpstreamNetworkMonitor.stop();
                 stopListeningForSimChanges();
                 notifyTetheredOfNewUpstreamIface(null);

diff --git a/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java b/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
index 4c950de..23481dc 100644 (file)
--- a/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
+++ b/services/core/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitor.java
@@ -109,7 +109,7 @@ public class UpstreamNetworkMonitor {
         mNetworkMap.clear();
     }
 
-    public void mobileUpstreamRequiresDun(boolean dunRequired) {
+    public void updateMobileRequiresDun(boolean dunRequired) {
         final boolean valueChanged = (mDunRequired != dunRequired);
         mDunRequired = dunRequired;
         if (valueChanged && mobileNetworkRequested()) {
@@ -123,7 +123,10 @@ public class UpstreamNetworkMonitor {
     }
 
     public void registerMobileNetworkRequest() {
-        if (mMobileNetworkCallback != null) return;
+        if (mMobileNetworkCallback != null) {
+            Log.e(TAG, "registerMobileNetworkRequest() already registered");
+            return;
+        }
 
         final NetworkRequest.Builder builder = new NetworkRequest.Builder()
                 .addTransportType(NetworkCapabilities.TRANSPORT_CELLULAR);
@@ -139,11 +142,10 @@ public class UpstreamNetworkMonitor {
         // Therefore, to avoid duplicate notifications, we only register a no-op.
         mMobileNetworkCallback = new NetworkCallback();
 
-        // TODO: Change the timeout from 0 (no onUnavailable callback) to use some
-        // moderate callback time (once timeout callbacks are implemented). This might
-        // be useful for updating some UI. Additionally, we should definitely log a
-        // message to aid in any subsequent debugging
-        if (DBG) Log.d(TAG, "requesting mobile upstream network: " + mobileUpstreamRequest);
+        // TODO: Change the timeout from 0 (no onUnavailable callback) to some
+        // moderate callback timeout. This might be useful for updating some UI.
+        // Additionally, we log a message to aid in any subsequent debugging.
+        Log.d(TAG, "requesting mobile upstream network: " + mobileUpstreamRequest);
 
         // The following use of the legacy type system cannot be removed until
         // after upstream selection no longer finds networks by legacy type.

diff --git a/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java b/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
index 00420e9..b4d4871 100644 (file)
--- a/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
+++ b/tests/net/java/com/android/server/connectivity/tethering/UpstreamNetworkMonitorTest.java
@@ -73,8 +73,8 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(unm.mobileNetworkRequested());
         // Given a null Context, and therefore a null ConnectivityManager,
         // these would cause an exception, if they actually attempted anything.
-        unm.mobileUpstreamRequiresDun(true);
-        unm.mobileUpstreamRequiresDun(false);
+        unm.updateMobileRequiresDun(true);
+        unm.updateMobileRequiresDun(false);
     }
 
     @Test
@@ -109,7 +109,7 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
-        mUNM.mobileUpstreamRequiresDun(false);
+        mUNM.updateMobileRequiresDun(false);
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
@@ -135,7 +135,7 @@ public class UpstreamNetworkMonitorTest {
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());
 
-        mUNM.mobileUpstreamRequiresDun(true);
+        mUNM.updateMobileRequiresDun(true);
         assertFalse(mUNM.mobileNetworkRequested());
         assertEquals(0, mCM.requested.size());