commit 543d0d32e428342cc3022e7f8453c52c701be5ac
author Maciej Żenczykowski <maze@google.com> Sat Oct 28 02:10:52 2023 +0000
committer Gerrit Code Review <noreply-gerritcodereview@google.com> Sat Oct 28 02:10:52 2023 +0000
tree d33d0dcaa19371a65c76e556a1d56ee2ef5da97e
parent 55ccfe19e24e0fdeab1c8c736a94da2478e65890

Revert "Add forwarding methods to RoutingCoordinator"

This reverts commit 55ccfe19e24e0fdeab1c8c736a94da2478e65890.

Reason for revert: this must be introducing some sort of race
it appears to cause
  atest EthernetTetheringTest 'NetdBinderTest#TetherForwardAddRemove'
to no longer reliably pass.

Change-Id: I5281ab3f42c5ce268d97a12db24a6768db3f4354

Tethering/src/android/net/ip/IpServer.java

diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java
index 2a25a86..246e5bc 100644
--- a/Tethering/src/android/net/ip/IpServer.java
+++ b/Tethering/src/android/net/ip/IpServer.java
@@ -827,38 +827,7 @@
             }
         } catch (ServiceSpecificException | RemoteException e) {
             mLog.e("Failed to add " + mIfaceName + " to local table: ", e);
-        }
-    }
-
-    private void addInterfaceForward(@NonNull final String fromIface,
-            @NonNull final String toIface) throws ServiceSpecificException, RemoteException {
-        if (null != mRoutingCoordinator.value) {
-            mRoutingCoordinator.value.addInterfaceForward(fromIface, toIface);
-        } else {
-            mNetd.tetherAddForward(fromIface, toIface);
-            mNetd.ipfwdAddInterfaceForward(fromIface, toIface);
-        }
-    }
-
-    private void removeInterfaceForward(@NonNull final String fromIface,
-            @NonNull final String toIface) {
-        if (null != mRoutingCoordinator.value) {
-            try {
-                mRoutingCoordinator.value.removeInterfaceForward(fromIface, toIface);
-            } catch (ServiceSpecificException e) {
-                mLog.e("Exception in removeInterfaceForward", e);
-            }
-        } else {
-            try {
-                mNetd.ipfwdRemoveInterfaceForward(fromIface, toIface);
-            } catch (RemoteException | ServiceSpecificException e) {
-                mLog.e("Exception in ipfwdRemoveInterfaceForward", e);
-            }
-            try {
-                mNetd.tetherRemoveForward(fromIface, toIface);
-            } catch (RemoteException | ServiceSpecificException e) {
-                mLog.e("Exception in disableNat", e);
-            }
+            return;
         }
     }
 
@@ -1368,7 +1337,16 @@
             // to remove their rules, which generates errors.
             // Just do the best we can.
             mBpfCoordinator.maybeDetachProgram(mIfaceName, upstreamIface);
-            removeInterfaceForward(mIfaceName, upstreamIface);
+            try {
+                mNetd.ipfwdRemoveInterfaceForward(mIfaceName, upstreamIface);
+            } catch (RemoteException | ServiceSpecificException e) {
+                mLog.e("Exception in ipfwdRemoveInterfaceForward: " + e.toString());
+            }
+            try {
+                mNetd.tetherRemoveForward(mIfaceName, upstreamIface);
+            } catch (RemoteException | ServiceSpecificException e) {
+                mLog.e("Exception in disableNat: " + e.toString());
+            }
         }
 
         @Override
@@ -1424,9 +1402,10 @@
 
                         mBpfCoordinator.maybeAttachProgram(mIfaceName, ifname);
                         try {
-                            addInterfaceForward(mIfaceName, ifname);
+                            mNetd.tetherAddForward(mIfaceName, ifname);
+                            mNetd.ipfwdAddInterfaceForward(mIfaceName, ifname);
                         } catch (RemoteException | ServiceSpecificException e) {
-                            mLog.e("Exception enabling iface forward", e);
+                            mLog.e("Exception enabling NAT: " + e.toString());
                             cleanupUpstream();
                             mLastError = TETHER_ERROR_ENABLE_FORWARDING_ERROR;
                             transitionTo(mInitialState);

Tethering/tests/unit/src/android/net/ip/IpServerTest.java

diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
index 15df3ba..fc9928d 100644
--- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
+++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
@@ -321,23 +321,6 @@
         when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(DEFAULT_USING_BPF_OFFLOAD);
         when(mTetherConfig.useLegacyDhcpServer()).thenReturn(false /* default value */);
 
-        // Simulate the behavior of RoutingCoordinator
-        if (null != mRoutingCoordinatorManager.value) {
-            doAnswer(it -> {
-                final String fromIface = (String) it.getArguments()[0];
-                final String toIface = (String) it.getArguments()[1];
-                mNetd.tetherAddForward(fromIface, toIface);
-                mNetd.ipfwdAddInterfaceForward(fromIface, toIface);
-                return null;
-            }).when(mRoutingCoordinatorManager.value).addInterfaceForward(any(), any());
-            doAnswer(it -> {
-                final String fromIface = (String) it.getArguments()[0];
-                final String toIface = (String) it.getArguments()[1];
-                mNetd.ipfwdRemoveInterfaceForward(fromIface, toIface);
-                mNetd.tetherRemoveForward(fromIface, toIface);
-                return null;
-            }).when(mRoutingCoordinatorManager.value).removeInterfaceForward(any(), any());
-        }
         mBpfDeps = new BpfCoordinator.Dependencies() {
                     @NonNull
                     public Handler getHandler() {