From dd59eced79d9a3af037ec7ab7c0f8dc3fb62549d Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 20 Dec 2017 13:23:32 +0900 Subject: [PATCH] Readability improvements to the Vpn class with no logic changes Test: runtests frameworks-net Change-Id: I18de9dc9801b578e5a71adea5c38e49aa40cd2b1 --- .../com/android/server/connectivity/Vpn.java | 91 +++++++++---------- 1 file changed, 45 insertions(+), 46 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 7715727f6d73e..1f241213e8110 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 @@ public class Vpn { // 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 @@ public class Vpn { @GuardedBy("this") private Set 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 @@ public class Vpn { } 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 @@ public class Vpn { 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 @@ public class Vpn { } /** - * 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 @@ public class Vpn { } private void unregisterPackageChangeReceiverLocked() { - // register previous intent filter if (mIsPackageIntentReceiverRegistered) { mContext.unregisterReceiver(mPackageIntentReceiver); mIsPackageIntentReceiverRegistered = false; @@ -581,7 +581,7 @@ public class Vpn { 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 @@ public class Vpn { * 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 @@ public class Vpn { * @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 @@ public class Vpn { 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 @@ public class Vpn { 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 @@ public class Vpn { } 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 @@ public class Vpn { 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 @@ public class Vpn { } 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 @@ public class Vpn { } /** - * 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 @@ public class Vpn { * 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 @@ public class Vpn { 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 @@ public class Vpn { 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 @@ public class Vpn { } } - 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 @@ public class Vpn { // Wait for the daemon to start. while (!SystemService.isRunning(daemon)) { - checkpoint(true); + checkInterruptAndDelay(true); } // Create the control socket. @@ -1958,7 +1956,7 @@ public class Vpn { } catch (Exception e) { // ignore } - checkpoint(true); + checkInterruptAndDelay(true); } mSockets[i].setSoTimeout(500); @@ -1972,7 +1970,7 @@ public class Vpn { 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 @@ public class Vpn { } catch (Exception e) { // ignore } - checkpoint(true); + checkInterruptAndDelay(true); } } @@ -2001,7 +1999,7 @@ public class Vpn { throw new IllegalStateException(daemon + " is dead"); } } - checkpoint(true); + checkInterruptAndDelay(true); } // Now we are connected. Read and parse the new state. @@ -2057,8 +2055,8 @@ public class Vpn { // 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 @@ public class Vpn { } /** - * 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; }