Readability improvements to the Vpn class with no logic changes
Test: runtests frameworks-net
Change-Id: I18de9dc9801b578e5a71adea5c38e49aa40cd2b1
diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index 7715727..1f24121 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -128,7 +128,7 @@
// Length of time (in milliseconds) that an app hosting an always-on VPN is placed on
// the device idle whitelist during service launch and VPN bootstrap.
- private static final long VPN_LAUNCH_IDLE_WHITELIST_DURATION = 60 * 1000;
+ private static final long VPN_LAUNCH_IDLE_WHITELIST_DURATION_MS = 60 * 1000;
// TODO: create separate trackers for each unique VPN to support
// automated reconnection
@@ -183,10 +183,10 @@
@GuardedBy("this")
private Set<UidRange> mBlockedUsers = new ArraySet<>();
- // Handle of user initiating VPN.
+ // Handle of the user initiating VPN.
private final int mUserHandle;
- // Listen to package remove and change event in this user
+ // Listen to package removal and change events (update/uninstall) for this user
private final BroadcastReceiver mPackageIntentReceiver = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
@@ -197,14 +197,14 @@
}
synchronized (Vpn.this) {
- // Avoid race that always-on package has been unset
+ // Avoid race where always-on package has been unset
if (!packageName.equals(getAlwaysOnPackage())) {
return;
}
final String action = intent.getAction();
- Log.i(TAG, "Received broadcast " + action + " for always-on package " + packageName
- + " in user " + mUserHandle);
+ Log.i(TAG, "Received broadcast " + action + " for always-on VPN package "
+ + packageName + " in user " + mUserHandle);
switch(action) {
case Intent.ACTION_PACKAGE_REPLACED:
@@ -248,7 +248,8 @@
Log.wtf(TAG, "Problem registering observer", e);
}
- mNetworkInfo = new NetworkInfo(ConnectivityManager.TYPE_VPN, 0, NETWORKTYPE, "");
+ mNetworkInfo = new NetworkInfo(ConnectivityManager.TYPE_VPN, 0 /* subtype */, NETWORKTYPE,
+ "" /* subtypeName */);
mNetworkCapabilities = new NetworkCapabilities();
mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
@@ -258,7 +259,7 @@
}
/**
- * Set if this object is responsible for watching for {@link NetworkInfo}
+ * Set whether this object is responsible for watching for {@link NetworkInfo}
* teardown. When {@code false}, teardown is handled externally by someone
* else.
*/
@@ -480,7 +481,6 @@
}
private void unregisterPackageChangeReceiverLocked() {
- // register previous intent filter
if (mIsPackageIntentReceiverRegistered) {
mContext.unregisterReceiver(mPackageIntentReceiver);
mIsPackageIntentReceiverRegistered = false;
@@ -581,7 +581,7 @@
DeviceIdleController.LocalService idleController =
LocalServices.getService(DeviceIdleController.LocalService.class);
idleController.addPowerSaveTempWhitelistApp(Process.myUid(), alwaysOnPackage,
- VPN_LAUNCH_IDLE_WHITELIST_DURATION, mUserHandle, false, "vpn");
+ VPN_LAUNCH_IDLE_WHITELIST_DURATION_MS, mUserHandle, false, "vpn");
// Start the VPN service declared in the app's manifest.
Intent serviceIntent = new Intent(VpnConfig.SERVICE_INTERFACE);
@@ -611,9 +611,10 @@
* It uses {@link VpnConfig#LEGACY_VPN} as its package name, and
* it can be revoked by itself.
*
- * Note: when we added VPN pre-consent in http://ag/522961 the names oldPackage
- * and newPackage become misleading, because when an app is pre-consented, we
- * actually prepare oldPackage, not newPackage.
+ * Note: when we added VPN pre-consent in
+ * https://android.googlesource.com/platform/frameworks/base/+/0554260
+ * the names oldPackage and newPackage became misleading, because when
+ * an app is pre-consented, we actually prepare oldPackage, not newPackage.
*
* Their meanings actually are:
*
@@ -629,7 +630,7 @@
* @param oldPackage The package name of the old VPN application
* @param newPackage The package name of the new VPN application
*
- * @return true if the operation is succeeded.
+ * @return true if the operation succeeded.
*/
public synchronized boolean prepare(String oldPackage, String newPackage) {
if (oldPackage != null) {
@@ -638,7 +639,7 @@
return false;
}
- // Package is not same or old package was reinstalled.
+ // Package is not the same or old package was reinstalled.
if (!isCurrentPreparedPackage(oldPackage)) {
// The package doesn't match. We return false (to obtain user consent) unless the
// user has already consented to that VPN package.
@@ -860,8 +861,8 @@
long token = Binder.clearCallingIdentity();
try {
- mNetworkAgent = new NetworkAgent(mLooper, mContext, NETWORKTYPE,
- mNetworkInfo, mNetworkCapabilities, lp, 0, networkMisc) {
+ mNetworkAgent = new NetworkAgent(mLooper, mContext, NETWORKTYPE /* logtag */,
+ mNetworkInfo, mNetworkCapabilities, lp, 0 /* score */, networkMisc) {
@Override
public void unwanted() {
// We are user controlled, not driven by NetworkRequest.
@@ -935,7 +936,7 @@
}
ResolveInfo info = AppGlobals.getPackageManager().resolveService(intent,
- null, 0, mUserHandle);
+ null, 0, mUserHandle);
if (info == null) {
throw new SecurityException("Cannot find " + config.user);
}
@@ -943,7 +944,7 @@
throw new SecurityException(config.user + " does not require " + BIND_VPN_SERVICE);
}
} catch (RemoteException e) {
- throw new SecurityException("Cannot find " + config.user);
+ throw new SecurityException("Cannot find " + config.user);
} finally {
Binder.restoreCallingIdentity(token);
}
@@ -1336,7 +1337,7 @@
}
private void enforceControlPermissionOrInternalCaller() {
- // Require caller to be either an application with CONTROL_VPN permission or a process
+ // Require the caller to be either an application with CONTROL_VPN permission or a process
// in the system server.
mContext.enforceCallingOrSelfPermission(Manifest.permission.CONTROL_VPN,
"Unauthorized Caller");
@@ -1416,7 +1417,7 @@
}
/**
- * This method should only be called by ConnectivityService. Because it doesn't
+ * This method should only be called by ConnectivityService because it doesn't
* have enough data to fill VpnInfo.primaryUnderlyingIface field.
*/
public synchronized VpnInfo getVpnInfo() {
@@ -1767,7 +1768,7 @@
* Bringing up a VPN connection takes time, and that is all this thread
* does. Here we have plenty of time. The only thing we need to take
* care of is responding to interruptions as soon as possible. Otherwise
- * requests will be piled up. This can be done in a Handler as a state
+ * requests will pile up. This could be done in a Handler as a state
* machine, but it is much easier to read in the current form.
*/
private class LegacyVpnRunner extends Thread {
@@ -1780,7 +1781,7 @@
private final AtomicInteger mOuterConnection =
new AtomicInteger(ConnectivityManager.TYPE_NONE);
- private long mTimer = -1;
+ private long mBringupStartTime = -1;
/**
* Watch for the outer connection (passing in the constructor) going away.
@@ -1860,8 +1861,8 @@
synchronized (TAG) {
Log.v(TAG, "Executing");
try {
- execute();
- monitorDaemons();
+ bringup();
+ waitForDaemonsToStop();
interrupted(); // Clear interrupt flag if execute called exit.
} catch (InterruptedException e) {
} finally {
@@ -1882,30 +1883,27 @@
}
}
- private void checkpoint(boolean yield) throws InterruptedException {
+ private void checkInterruptAndDelay(boolean sleepLonger) throws InterruptedException {
long now = SystemClock.elapsedRealtime();
- if (mTimer == -1) {
- mTimer = now;
- Thread.sleep(1);
- } else if (now - mTimer <= 60000) {
- Thread.sleep(yield ? 200 : 1);
+ if (now - mBringupStartTime <= 60000) {
+ Thread.sleep(sleepLonger ? 200 : 1);
} else {
updateState(DetailedState.FAILED, "checkpoint");
- throw new IllegalStateException("Time is up");
+ throw new IllegalStateException("VPN bringup took too long");
}
}
- private void execute() {
- // Catch all exceptions so we can clean up few things.
+ private void bringup() {
+ // Catch all exceptions so we can clean up a few things.
boolean initFinished = false;
try {
// Initialize the timer.
- checkpoint(false);
+ mBringupStartTime = SystemClock.elapsedRealtime();
// Wait for the daemons to stop.
for (String daemon : mDaemons) {
while (!SystemService.isStopped(daemon)) {
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
}
@@ -1942,7 +1940,7 @@
// Wait for the daemon to start.
while (!SystemService.isRunning(daemon)) {
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
// Create the control socket.
@@ -1958,7 +1956,7 @@
} catch (Exception e) {
// ignore
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
mSockets[i].setSoTimeout(500);
@@ -1972,7 +1970,7 @@
out.write(bytes.length >> 8);
out.write(bytes.length);
out.write(bytes);
- checkpoint(false);
+ checkInterruptAndDelay(false);
}
out.write(0xFF);
out.write(0xFF);
@@ -1988,7 +1986,7 @@
} catch (Exception e) {
// ignore
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
}
@@ -2001,7 +1999,7 @@
throw new IllegalStateException(daemon + " is dead");
}
}
- checkpoint(true);
+ checkInterruptAndDelay(true);
}
// Now we are connected. Read and parse the new state.
@@ -2057,8 +2055,8 @@
// Set the start time
mConfig.startTime = SystemClock.elapsedRealtime();
- // Check if the thread is interrupted while we are waiting.
- checkpoint(false);
+ // Check if the thread was interrupted while we were waiting on the lock.
+ checkInterruptAndDelay(false);
// Check if the interface is gone while we are waiting.
if (jniCheck(mConfig.interfaze) == 0) {
@@ -2081,10 +2079,11 @@
}
/**
- * Monitor the daemons we started, moving to disconnected state if the
- * underlying services fail.
+ * Check all daemons every two seconds. Return when one of them is stopped.
+ * The caller will move to the disconnected state when this function returns,
+ * which can happen if a daemon failed or if the VPN was torn down.
*/
- private void monitorDaemons() throws InterruptedException{
+ private void waitForDaemonsToStop() throws InterruptedException {
if (!mNetworkInfo.isConnected()) {
return;
}