Merge "Don't completely stop offload if setting data limit fails." into oc-mr1-dev

am: 9b601f2066

Change-Id: I7fc47e60b2da5f39c26fb22c1325618f9948dd38

2 files changed, 28 insertions, 6 deletions

diff --git a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
index bcc74c0d49e4..ef18e4e0ba75 100644
--- a/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
+++ b/services/core/java/com/android/server/connectivity/tethering/OffloadController.java
@@ -131,21 +131,25 @@ public class OffloadController {
                 new OffloadHardwareInterface.ControlCallback() {
                     @Override
                     public void onStarted() {
+                        if (!started()) return;
                         mLog.log("onStarted");
                     }
 
                     @Override
                     public void onStoppedError() {
+                        if (!started()) return;
                         mLog.log("onStoppedError");
                     }
 
                     @Override
                     public void onStoppedUnsupported() {
+                        if (!started()) return;
                         mLog.log("onStoppedUnsupported");
                     }
 
                     @Override
                     public void onSupportAvailable() {
+                        if (!started()) return;
                         mLog.log("onSupportAvailable");
 
                         // [1] Poll for statistics and notify NetworkStats
@@ -153,11 +157,12 @@ public class OffloadController {
                         //     [a] push local prefixes
                         //     [b] push downstreams
                         //     [c] push upstream parameters
-                        pushUpstreamParameters();
+                        pushUpstreamParameters(null);
                     }
 
                     @Override
                     public void onStoppedLimitReached() {
+                        if (!started()) return;
                         mLog.log("onStoppedLimitReached");
 
                         // We cannot reliably determine on which interface the limit was reached,
@@ -185,6 +190,7 @@ public class OffloadController {
                     public void onNatTimeoutUpdate(int proto,
                                                    String srcAddr, int srcPort,
                                                    String dstAddr, int dstPort) {
+                        if (!started()) return;
                         mLog.log(String.format("NAT timeout update: %s (%s,%s) -> (%s,%s)",
                                 proto, srcAddr, srcPort, dstAddr, dstPort));
                     }
@@ -197,6 +203,9 @@ public class OffloadController {
     }
 
     public void stop() {
+        // Completely stops tethering offload. After this method is called, it is no longer safe to
+        // call any HAL method, no callbacks from the hardware will be delivered, and any in-flight
+        // callbacks must be ignored. Offload may be started again by calling start().
         final boolean wasStarted = started();
         updateStatsForCurrentUpstream();
         mUpstreamLinkProperties = null;
@@ -305,10 +314,7 @@ public class OffloadController {
         // onOffloadEvent() callback to tell us offload is available again and
         // then reapply all state).
         computeAndPushLocalPrefixes();
-        pushUpstreamParameters();
-
-        // Update stats after we've told the hardware to change routing so we don't miss packets.
-        maybeUpdateStats(prevUpstream);
+        pushUpstreamParameters(prevUpstream);
     }
 
     public void setLocalPrefixes(Set<IpPrefix> localPrefixes) {
@@ -342,8 +348,9 @@ public class OffloadController {
         return mConfigInitialized && mControlInitialized;
     }
 
-    private boolean pushUpstreamParameters() {
+    private boolean pushUpstreamParameters(String prevUpstream) {
         if (mUpstreamLinkProperties == null) {
+            maybeUpdateStats(prevUpstream);
             return mHwInterface.setUpstreamParameters(null, null, null, null);
         }
 
@@ -382,9 +389,14 @@ public class OffloadController {
            return success;
         }
 
+        // Update stats after we've told the hardware to change routing so we don't miss packets.
+        maybeUpdateStats(prevUpstream);
+
         // Data limits can only be set once offload is running on the upstream.
         success = maybeUpdateDataLimit(iface);
         if (!success) {
+            // If we failed to set a data limit, don't use this upstream, because we don't want to
+            // blow through the data limit that we were told to apply.
             mLog.log("Setting data limit for " + iface + " failed, disabling offload.");
             stop();
         }
diff --git a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
index c7290e77ba2d..d5bbed7e2aa0 100644
--- a/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
+++ b/tests/net/java/com/android/server/connectivity/tethering/OffloadControllerTest.java
@@ -110,7 +110,9 @@ public class OffloadControllerTest {
         when(mHardware.initOffloadConfig()).thenReturn(true);
         when(mHardware.initOffloadControl(mControlCallbackCaptor.capture()))
                 .thenReturn(true);
+        when(mHardware.setUpstreamParameters(anyString(), any(), any(), any())).thenReturn(true);
         when(mHardware.getForwardedStats(any())).thenReturn(new ForwardedStats());
+        when(mHardware.setDataLimit(anyString(), anyLong())).thenReturn(true);
     }
 
     private void enableOffload() {
@@ -256,6 +258,7 @@ public class OffloadControllerTest {
         inOrder.verify(mHardware, never()).setLocalPrefixes(mStringArrayCaptor.capture());
         inOrder.verify(mHardware, times(1)).setUpstreamParameters(
                 eq(testIfName), eq(null), eq(null), eq(null));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         final String ipv4Addr = "192.0.2.5";
@@ -273,6 +276,7 @@ public class OffloadControllerTest {
         inOrder.verify(mHardware, times(1)).setUpstreamParameters(
                 eq(testIfName), eq(ipv4Addr), eq(null), eq(null));
         inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         final String ipv4Gateway = "192.0.2.1";
@@ -283,6 +287,7 @@ public class OffloadControllerTest {
         inOrder.verify(mHardware, times(1)).setUpstreamParameters(
                 eq(testIfName), eq(ipv4Addr), eq(ipv4Gateway), eq(null));
         inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         final String ipv6Gw1 = "fe80::cafe";
@@ -296,6 +301,7 @@ public class OffloadControllerTest {
         ArrayList<String> v6gws = mStringArrayCaptor.getValue();
         assertEquals(1, v6gws.size());
         assertTrue(v6gws.contains(ipv6Gw1));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         final String ipv6Gw2 = "fe80::d00d";
@@ -310,6 +316,7 @@ public class OffloadControllerTest {
         assertEquals(2, v6gws.size());
         assertTrue(v6gws.contains(ipv6Gw1));
         assertTrue(v6gws.contains(ipv6Gw2));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         final LinkProperties stacked = new LinkProperties();
@@ -328,6 +335,7 @@ public class OffloadControllerTest {
         assertEquals(2, v6gws.size());
         assertTrue(v6gws.contains(ipv6Gw1));
         assertTrue(v6gws.contains(ipv6Gw2));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         // Add in some IPv6 upstream info. When there is a tethered downstream
@@ -359,6 +367,7 @@ public class OffloadControllerTest {
         assertTrue(v6gws.contains(ipv6Gw1));
         assertTrue(v6gws.contains(ipv6Gw2));
         inOrder.verify(mHardware, times(1)).getForwardedStats(eq(testIfName));
+        inOrder.verify(mHardware, times(1)).setDataLimit(eq(testIfName), eq(Long.MAX_VALUE));
         inOrder.verifyNoMoreInteractions();
 
         // Completely identical LinkProperties updates are de-duped.
@@ -520,6 +529,7 @@ public class OffloadControllerTest {
         offload.setUpstreamLinkProperties(lp);
         provider.setInterfaceQuota(mobileIface, mobileLimit);
         waitForIdle();
+        inOrder.verify(mHardware).getForwardedStats(ethernetIface);
         inOrder.verify(mHardware).stopOffloadControl();
     }