From 24398db2b6ea2cba4b3f1010466c586d604c5e79 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Mon, 6 Apr 2020 11:24:19 +0000 Subject: [PATCH] Refactor the Nat464Xlat function for simplicity. This makes the code easier to understand by making state transitions more explicit. It also makes it easier to address a TODO to turn the class into a StateMachine. This should be an exact no-op refactoring. The current cases covered by the code (all mutually exclusive) are: 1. requiresClat && !isPrefixDiscoveryStarted Action: startPrefixDiscovery() Equivalent to IDLE && requiresClat, because isPrefixDiscoveryStarted returns true for every state except IDLE. 2. requiresClat && isPrefixDiscoveryStarted && shouldStartClat Action: start() Equivalent to DISCOVERING && shouldStartClat, because isPrefixDiscoveryStarted is true in DISCOVERING, STARTING, and RUNNING, but start() does nothing if mState is STARTING or RUNNING. 3. requiresClat && isPrefixDiscoveryStarted && !shouldStartClat Action: stop() Equivalent to (STARTING or RUNNING) && !shouldStartClat, because isPrefixDiscoveryStarted is true in DISCOVERING, STARTING, and RUNNING, but stop() does nothing if mState is not STARTING or RUNNING. 4. !requiresClat && isStarted Action: stop() Equivalent to (STARTING or RUNNING) && !requiresClat, because isStarted() is only true in STARTING and RUNNING. 5. !requiresClat && !isStarted && isPrefixDiscoveryStarted Action: leaveStartedState() Equivalent to DISCOVERING && !requiresClat, because the only state with isPrefixDiscoveryStarted and !isStarted is DISCOVERING. Also, simplify case #5. In this case, calling leaveStartedState is superfluous, because in the DISCOVERING state: - There is no need to call unregisterObserver, since the observer is only registered when entering STARTING and is unregistered when going back to DISCOVERING or IDLE. - mIface and mBaseIface don't need to be set to null because they are only set to non-null when entering STARTING and nulled out when going back to DISCOVERING or IDLE. Bug: 126113090 Bug: 150648313 Test: covered by existing ConnectivityServiceTest and Nat464XlatTest Merged-In: Ice536bcb269cc8b040c6e7a72c15d0bc8b5bd235 Change-Id: Ice536bcb269cc8b040c6e7a72c15d0bc8b5bd235 --- .../server/connectivity/Nat464Xlat.java | 61 ++++++++++++------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index 82465f8a09..af2db4abde 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -213,12 +213,10 @@ public class Nat464Xlat extends BaseNetworkObserver { } mIface = null; mBaseIface = null; - mState = State.IDLE; if (requiresClat(mNetwork)) { mState = State.DISCOVERING; } else { stopPrefixDiscovery(); - mState = State.IDLE; } } @@ -285,6 +283,7 @@ public class Nat464Xlat extends BaseNetworkObserver { private void stopPrefixDiscovery() { try { mDnsResolver.stopPrefix64Discovery(getNetId()); + mState = State.IDLE; } catch (RemoteException | ServiceSpecificException e) { Slog.e(TAG, "Error stopping prefix discovery on netId " + getNetId() + ": " + e); } @@ -294,27 +293,43 @@ public class Nat464Xlat extends BaseNetworkObserver { * Starts/stops NAT64 prefix discovery and clatd as necessary. */ public void update() { - // TODO: turn this class into a proper StateMachine. // http://b/126113090 - if (requiresClat(mNetwork)) { - if (!isPrefixDiscoveryStarted()) { - startPrefixDiscovery(); - } else if (shouldStartClat(mNetwork)) { - // NAT64 prefix detected. Start clatd. - // TODO: support the NAT64 prefix changing after it's been discovered. There is no - // need to support this at the moment because it cannot happen without changes to - // the Dns64Configuration code in netd. - start(); - } else { - // NAT64 prefix removed. Stop clatd and go back into DISCOVERING state. - stop(); - } - } else { - // Network no longer requires clat. Stop clat and prefix discovery. - if (isStarted()) { - stop(); - } else if (isPrefixDiscoveryStarted()) { - leaveStartedState(); - } + // TODO: turn this class into a proper StateMachine. http://b/126113090 + switch (mState) { + case IDLE: + if (requiresClat(mNetwork)) { + // Network is detected to be IPv6-only. + // TODO: consider going to STARTING directly if the NAT64 prefix is already + // known. This would however result in clatd running without prefix discovery + // running, which might be a surprising combination. + startPrefixDiscovery(); // Enters DISCOVERING state. + return; + } + break; + + case DISCOVERING: + if (shouldStartClat(mNetwork)) { + // NAT64 prefix detected. Start clatd. + start(); // Enters STARTING state. + return; + } + if (!requiresClat(mNetwork)) { + // IPv4 address added. Go back to IDLE state. + stopPrefixDiscovery(); + return; + } + break; + + case STARTING: + case RUNNING: + // NAT64 prefix removed, or IPv4 address added. + // Stop clatd and go back into DISCOVERING or idle. + if (!shouldStartClat(mNetwork)) { + stop(); + } + break; + // TODO: support the NAT64 prefix changing after it's been discovered. There is + // no need to support this at the moment because it cannot happen without + // changes to the Dns64Configuration code in netd. } }