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
This commit is contained in:
@@ -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.
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user