Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8ea45483 authored by Erik Kline's avatar Erik Kline
Browse files

Cleanup in the face of upstream error 

If either enableNat() or startInterfaceForwarding() fail, be sure
to cleanup any commands that might have succeeded.

Most of this change is a refactoring of cleanupUpstreamIface() into
two methods, one of which (cleanupUpstreamInterface()) is reused
in error handling scenarios.

Test: as follows
    - built (bullhead)
    - flashed
    - booted
    - runtest -x .../tethering/TetherInterfaceStateMachineTest.java passes
Bug: 32031803
Bug: 32163131

Change-Id: Ia4d56e03beeab1908d8b8c2202e94992f1aa58a4
parent fb19d8d7

services/core/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachine.java

+27 −24
Original line number Diff line number Diff line
@@ -250,31 +250,33 @@ public class TetherInterfaceStateMachine extends StateMachine { 
        }



        private void cleanupUpstream() {

            if (mMyUpstreamIfaceName != null) {

                // note that we don't care about errors here.

                // sometimes interfaces are gone before we get

            if (mMyUpstreamIfaceName == null) return;



            cleanupUpstreamInterface(mMyUpstreamIfaceName);

            mMyUpstreamIfaceName = null;

        }



        private void cleanupUpstreamInterface(String upstreamIface) {

            // Note that we don't care about errors here.

            // Sometimes interfaces are gone before we get

            // to remove their rules, which generates errors.

                // just do the best we can.

            // Just do the best we can.

            try {

                    // about to tear down NAT; gather remaining statistics

                // About to tear down NAT; gather remaining statistics.

                mStatsService.forceUpdate();

            } catch (Exception e) {

                if (VDBG) Log.e(TAG, "Exception in forceUpdate: " + e.toString());

            }

            try {

                    mNMService.stopInterfaceForwarding(mIfaceName, mMyUpstreamIfaceName);

                mNMService.stopInterfaceForwarding(mIfaceName, upstreamIface);

            } catch (Exception e) {

                    if (VDBG) Log.e(

                            TAG, "Exception in removeInterfaceForward: " + e.toString());

                if (VDBG) Log.e(TAG, "Exception in removeInterfaceForward: " + e.toString());

            }

            try {

                    mNMService.disableNat(mIfaceName, mMyUpstreamIfaceName);

                mNMService.disableNat(mIfaceName, upstreamIface);

            } catch (Exception e) {

                if (VDBG) Log.e(TAG, "Exception in disableNat: " + e.toString());

            }

                mMyUpstreamIfaceName = null;

            }

            return;

        }



        @Override
@@ -306,6 +308,7 @@ public class TetherInterfaceStateMachine extends StateMachine { 
                                    newUpstreamIfaceName);

                        } catch (Exception e) {

                            Log.e(TAG, "Exception enabling Nat: " + e.toString());

                            cleanupUpstreamInterface(newUpstreamIfaceName);

                            mLastError = ConnectivityManager.TETHER_ERROR_ENABLE_NAT_ERROR;

                            transitionTo(mInitialState);

                            return true;

tests/net/java/com/android/server/connectivity/tethering/TetherInterfaceStateMachineTest.java

+6 −8
Original line number Diff line number Diff line
@@ -210,10 +210,9 @@ public class TetherInterfaceStateMachineTest { 
        inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE);

        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE);

        inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE2);

        // TODO: Verify proper cleanup is performed:

        // inOrder.verify(mStatsService).forceUpdate();

        // inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);

        // inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);

        inOrder.verify(mStatsService).forceUpdate();

        inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);

        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);

    }



    @Test
@@ -230,10 +229,9 @@ public class TetherInterfaceStateMachineTest { 
        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE);

        inOrder.verify(mNMService).enableNat(IFACE_NAME, UPSTREAM_IFACE2);

        inOrder.verify(mNMService).startInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);

        // TODO: Verify proper cleanup is performed:

        // inOrder.verify(mStatsService).forceUpdate();

        // inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);

        // inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);

        inOrder.verify(mStatsService).forceUpdate();

        inOrder.verify(mNMService).stopInterfaceForwarding(IFACE_NAME, UPSTREAM_IFACE2);

        inOrder.verify(mNMService).disableNat(IFACE_NAME, UPSTREAM_IFACE2);

    }



    @Test