From b152cd0aa4f333b721615bb17773b35a989245fb Mon Sep 17 00:00:00 2001 From: Erik Kline Date: Fri, 2 Mar 2018 16:51:13 +0900 Subject: [PATCH] Fail if the interface is not available when starting Addresses a long-standing TODO. Now, when calling IpClient's startProvisioning(), the interface has to be available (i.e. InterfaceParams#getByName() must return non-null). Also: - add a test - refactor for testability - delete some constructors no longer used - properly handle passed-in null IpClient.Callback - some more IpManager -> IpClient renaming - permit recording metrics before starting a provisioning attempt (logging immediate errors) without Log.wtf(). Test: as follows - built - flashed - booted - runtest frameworks/opt/net/wifi/tests/wifitests/runtests.sh passes - runtest frameworks-net passes - basic WiFi IpClient connections works fine Bug: 62476366 Bug: 73487570 Change-Id: Ic83ad2a65637277dcb273feb27b2d1bb7a11eb2b --- core/java/android/net/metrics/IpManagerEvent.java | 3 +- services/net/java/android/net/ip/IpClient.java | 53 ++++++--- services/net/java/android/net/ip/IpManager.java | 15 +-- .../ip/{IpManagerTest.java => IpClientTest.java} | 123 +++++++++++++++------ 4 files changed, 131 insertions(+), 63 deletions(-) rename tests/net/java/android/net/ip/{IpManagerTest.java => IpClientTest.java} (80%) diff --git a/core/java/android/net/metrics/IpManagerEvent.java b/core/java/android/net/metrics/IpManagerEvent.java index a94b9284df3b..f8a63ce5f1a3 100644 --- a/core/java/android/net/metrics/IpManagerEvent.java +++ b/core/java/android/net/metrics/IpManagerEvent.java @@ -40,11 +40,12 @@ public final class IpManagerEvent implements Parcelable { public static final int ERROR_STARTING_IPV6 = 5; public static final int ERROR_STARTING_IPREACHABILITYMONITOR = 6; public static final int ERROR_INVALID_PROVISIONING = 7; + public static final int ERROR_INTERFACE_NOT_FOUND = 8; @IntDef(value = { PROVISIONING_OK, PROVISIONING_FAIL, COMPLETE_LIFECYCLE, ERROR_STARTING_IPV4, ERROR_STARTING_IPV6, ERROR_STARTING_IPREACHABILITYMONITOR, - ERROR_INVALID_PROVISIONING, + ERROR_INVALID_PROVISIONING, ERROR_INTERFACE_NOT_FOUND, }) @Retention(RetentionPolicy.SOURCE) public @interface EventType {} diff --git a/services/net/java/android/net/ip/IpClient.java b/services/net/java/android/net/ip/IpClient.java index 1f370a574231..9863370e3840 100644 --- a/services/net/java/android/net/ip/IpClient.java +++ b/services/net/java/android/net/ip/IpClient.java @@ -540,6 +540,8 @@ public class IpClient extends StateMachine { // TODO: Revert this hack once IpClient and Nat464Xlat work in concert. private static final String CLAT_PREFIX = "v4-"; + private static final int IMMEDIATE_FAILURE_DURATION = 0; + private final State mStoppedState = new StoppedState(); private final State mStoppingState = new StoppingState(); private final State mStartedState = new StartedState(); @@ -551,6 +553,7 @@ public class IpClient extends StateMachine { private final String mClatInterfaceName; @VisibleForTesting protected final Callback mCallback; + private final Dependencies mDependencies; private final CountDownLatch mShutdownLatch; private final INetworkManagementService mNwService; private final NetlinkTracker mNetlinkTracker; @@ -579,10 +582,23 @@ public class IpClient extends StateMachine { private boolean mMulticastFiltering; private long mStartTimeMillis; + public static class Dependencies { + public INetworkManagementService getNMS() { + return INetworkManagementService.Stub.asInterface( + ServiceManager.getService(Context.NETWORKMANAGEMENT_SERVICE)); + } + + public INetd getNetd() { + return NetdService.getInstance(); + } + + public InterfaceParams getInterfaceParams(String ifname) { + return InterfaceParams.getByName(ifname); + } + } + public IpClient(Context context, String ifName, Callback callback) { - this(context, ifName, callback, INetworkManagementService.Stub.asInterface( - ServiceManager.getService(Context.NETWORKMANAGEMENT_SERVICE)), - NetdService.getInstance()); + this(context, ifName, callback, new Dependencies()); } /** @@ -591,27 +607,35 @@ public class IpClient extends StateMachine { */ public IpClient(Context context, String ifName, Callback callback, INetworkManagementService nwService) { - this(context, ifName, callback, nwService, NetdService.getInstance()); + this(context, ifName, callback, new Dependencies() { + @Override + public INetworkManagementService getNMS() { return nwService; } + }); } @VisibleForTesting - IpClient(Context context, String ifName, Callback callback, - INetworkManagementService nwService, INetd netd) { + IpClient(Context context, String ifName, Callback callback, Dependencies deps) { super(IpClient.class.getSimpleName() + "." + ifName); + Preconditions.checkNotNull(ifName); + Preconditions.checkNotNull(callback); + mTag = getName(); mContext = context; mInterfaceName = ifName; mClatInterfaceName = CLAT_PREFIX + ifName; mCallback = new LoggingCallbackWrapper(callback); + mDependencies = deps; mShutdownLatch = new CountDownLatch(1); - mNwService = nwService; + mNwService = deps.getNMS(); mLog = new SharedLog(MAX_LOG_RECORDS, mTag); mConnectivityPacketLog = new LocalLog(MAX_PACKET_RECORDS); mMsgStateLogger = new MessageHandlingLogger(); - mInterfaceCtrl = new InterfaceController(mInterfaceName, mNwService, netd, mLog); + // TODO: Consider creating, constructing, and passing in some kind of + // InterfaceController.Dependencies class. + mInterfaceCtrl = new InterfaceController(mInterfaceName, mNwService, deps.getNetd(), mLog); mNetlinkTracker = new NetlinkTracker( mInterfaceName, @@ -742,11 +766,11 @@ public class IpClient extends StateMachine { return; } - mInterfaceParams = InterfaceParams.getByName(mInterfaceName); + mInterfaceParams = mDependencies.getInterfaceParams(mInterfaceName); if (mInterfaceParams == null) { logError("Failed to find InterfaceParams for " + mInterfaceName); - // TODO: call doImmediateProvisioningFailure() with an error code - // indicating something like "interface not ready". + doImmediateProvisioningFailure(IpManagerEvent.ERROR_INTERFACE_NOT_FOUND); + return; } mCallback.setNeighborDiscoveryOffload(true); @@ -930,8 +954,11 @@ public class IpClient extends StateMachine { } private void recordMetric(final int type) { - if (mStartTimeMillis <= 0) { Log.wtf(mTag, "Start time undefined!"); } - final long duration = SystemClock.elapsedRealtime() - mStartTimeMillis; + // We may record error metrics prior to starting. + // Map this to IMMEDIATE_FAILURE_DURATION. + final long duration = (mStartTimeMillis > 0) + ? (SystemClock.elapsedRealtime() - mStartTimeMillis) + : IMMEDIATE_FAILURE_DURATION; mMetricsLog.log(mInterfaceName, new IpManagerEvent(type, duration)); } diff --git a/services/net/java/android/net/ip/IpManager.java b/services/net/java/android/net/ip/IpManager.java index 38981453e2e1..508a43d022f4 100644 --- a/services/net/java/android/net/ip/IpManager.java +++ b/services/net/java/android/net/ip/IpManager.java @@ -144,20 +144,7 @@ public class IpManager extends IpClient { } public IpManager(Context context, String ifName, Callback callback) { - this(context, ifName, callback, INetworkManagementService.Stub.asInterface( - ServiceManager.getService(Context.NETWORKMANAGEMENT_SERVICE)), - NetdService.getInstance()); - } - - public IpManager(Context context, String ifName, Callback callback, - INetworkManagementService nwService) { - this(context, ifName, callback, nwService, NetdService.getInstance()); - } - - @VisibleForTesting - public IpManager(Context context, String ifName, Callback callback, - INetworkManagementService nwService, INetd netd) { - super(context, ifName, callback, nwService, netd); + super(context, ifName, callback); } public void startProvisioning(ProvisioningConfiguration req) { diff --git a/tests/net/java/android/net/ip/IpManagerTest.java b/tests/net/java/android/net/ip/IpClientTest.java similarity index 80% rename from tests/net/java/android/net/ip/IpManagerTest.java rename to tests/net/java/android/net/ip/IpClientTest.java index 22d88fb70697..e9e880d1e7c1 100644 --- a/tests/net/java/android/net/ip/IpManagerTest.java +++ b/tests/net/java/android/net/ip/IpClientTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -38,10 +39,12 @@ import android.net.INetd; import android.net.IpPrefix; import android.net.LinkAddress; import android.net.LinkProperties; +import android.net.MacAddress; import android.net.RouteInfo; -import android.net.ip.IpManager.Callback; -import android.net.ip.IpManager.InitialConfiguration; -import android.net.ip.IpManager.ProvisioningConfiguration; +import android.net.ip.IpClient.Callback; +import android.net.ip.IpClient.InitialConfiguration; +import android.net.ip.IpClient.ProvisioningConfiguration; +import android.net.util.InterfaceParams; import android.os.INetworkManagementService; import android.provider.Settings; import android.support.test.filters.SmallTest; @@ -68,15 +71,19 @@ import java.util.HashSet; import java.util.Set; /** - * Tests for IpManager. + * Tests for IpClient. */ @RunWith(AndroidJUnit4.class) @SmallTest -public class IpManagerTest { +public class IpClientTest { private static final int DEFAULT_AVOIDBADWIFI_CONFIG_VALUE = 1; private static final String VALID = "VALID"; private static final String INVALID = "INVALID"; + private static final String TEST_IFNAME = "test_wlan0"; + private static final int TEST_IFINDEX = 1001; + // See RFC 7042#section-2.1.2 for EUI-48 documentation values. + private static final MacAddress TEST_MAC = MacAddress.fromString("00:00:5E:00:53:01"); @Mock private Context mContext; @Mock private INetworkManagementService mNMService; @@ -84,9 +91,11 @@ public class IpManagerTest { @Mock private Resources mResources; @Mock private Callback mCb; @Mock private AlarmManager mAlarm; + @Mock private IpClient.Dependencies mDependecies; private MockContentResolver mContentResolver; - BaseNetworkObserver mObserver; + private BaseNetworkObserver mObserver; + private InterfaceParams mIfParams; @Before public void setUp() throws Exception { @@ -100,10 +109,23 @@ public class IpManagerTest { mContentResolver = new MockContentResolver(); mContentResolver.addProvider(Settings.AUTHORITY, new FakeSettingsProvider()); when(mContext.getContentResolver()).thenReturn(mContentResolver); + + mIfParams = null; + + when(mDependecies.getNMS()).thenReturn(mNMService); + when(mDependecies.getNetd()).thenReturn(mNetd); + } + + private void setTestInterfaceParams(String ifname) { + mIfParams = (ifname != null) + ? new InterfaceParams(ifname, TEST_IFINDEX, TEST_MAC) + : null; + when(mDependecies.getInterfaceParams(anyString())).thenReturn(mIfParams); } - private IpManager makeIpManager(String ifname) throws Exception { - final IpManager ipm = new IpManager(mContext, ifname, mCb, mNMService, mNetd); + private IpClient makeIpClient(String ifname) throws Exception { + setTestInterfaceParams(ifname); + final IpClient ipc = new IpClient(mContext, ifname, mCb, mDependecies); verify(mNMService, timeout(100).times(1)).disableIpv6(ifname); verify(mNMService, timeout(100).times(1)).clearInterfaceAddresses(ifname); ArgumentCaptor arg = @@ -111,23 +133,54 @@ public class IpManagerTest { verify(mNMService, times(1)).registerObserver(arg.capture()); mObserver = arg.getValue(); reset(mNMService); - return ipm; + return ipc; + } + + @Test + public void testNullInterfaceNameMostDefinitelyThrows() throws Exception { + setTestInterfaceParams(null); + try { + final IpClient ipc = new IpClient(mContext, null, mCb, mDependecies); + ipc.shutdown(); + fail(); + } catch (NullPointerException npe) { + // Phew; null interface names not allowed. + } } @Test - public void testNullCallbackDoesNotThrow() throws Exception { - final IpManager ipm = new IpManager(mContext, "lo", null, mNMService); + public void testNullCallbackMostDefinitelyThrows() throws Exception { + final String ifname = "lo"; + setTestInterfaceParams(ifname); + try { + final IpClient ipc = new IpClient(mContext, ifname, null, mDependecies); + ipc.shutdown(); + fail(); + } catch (NullPointerException npe) { + // Phew; null callbacks not allowed. + } } @Test public void testInvalidInterfaceDoesNotThrow() throws Exception { - final IpManager ipm = new IpManager(mContext, "test_wlan0", mCb, mNMService); + setTestInterfaceParams(TEST_IFNAME); + final IpClient ipc = new IpClient(mContext, TEST_IFNAME, mCb, mDependecies); + ipc.shutdown(); + } + + @Test + public void testInterfaceNotFoundFailsImmediately() throws Exception { + setTestInterfaceParams(null); + final IpClient ipc = new IpClient(mContext, TEST_IFNAME, mCb, mDependecies); + ipc.startProvisioning(new IpClient.ProvisioningConfiguration()); + verify(mCb, times(1)).onProvisioningFailure(any()); + ipc.shutdown(); } @Test public void testDefaultProvisioningConfiguration() throws Exception { - final String iface = "test_wlan0"; - final IpManager ipm = makeIpManager(iface); + final String iface = TEST_IFNAME; + final IpClient ipc = makeIpClient(iface); ProvisioningConfiguration config = new ProvisioningConfiguration.Builder() .withoutIPv4() @@ -136,20 +189,20 @@ public class IpManagerTest { .withoutIpReachabilityMonitor() .build(); - ipm.startProvisioning(config); + ipc.startProvisioning(config); verify(mCb, times(1)).setNeighborDiscoveryOffload(true); verify(mCb, timeout(100).times(1)).setFallbackMulticastFilter(false); verify(mCb, never()).onProvisioningFailure(any()); - ipm.stop(); + ipc.shutdown(); verify(mNMService, timeout(100).times(1)).disableIpv6(iface); verify(mNMService, timeout(100).times(1)).clearInterfaceAddresses(iface); } @Test public void testProvisioningWithInitialConfiguration() throws Exception { - final String iface = "test_wlan0"; - final IpManager ipm = makeIpManager(iface); + final String iface = TEST_IFNAME; + final IpClient ipc = makeIpClient(iface); String[] addresses = { "fe80::a4be:f92:e1f7:22d1/64", @@ -164,7 +217,7 @@ public class IpManagerTest { .withInitialConfiguration(conf(links(addresses), prefixes(prefixes), ips())) .build(); - ipm.startProvisioning(config); + ipc.startProvisioning(config); verify(mCb, times(1)).setNeighborDiscoveryOffload(true); verify(mCb, timeout(100).times(1)).setFallbackMulticastFilter(false); verify(mCb, never()).onProvisioningFailure(any()); @@ -190,7 +243,7 @@ public class IpManagerTest { want.setInterfaceName(iface); verify(mCb, timeout(100).times(1)).onProvisioningSuccess(eq(want)); - ipm.stop(); + ipc.shutdown(); verify(mNMService, timeout(100).times(1)).disableIpv6(iface); verify(mNMService, timeout(100).times(1)).clearInterfaceAddresses(iface); } @@ -228,7 +281,7 @@ public class IpManagerTest { }; for (IsProvisionedTestCase testcase : testcases) { - if (IpManager.isProvisioned(testcase.lp, testcase.config) != testcase.isProvisioned) { + if (IpClient.isProvisioned(testcase.lp, testcase.config) != testcase.isProvisioned) { fail(testcase.errorMessage()); } } @@ -424,11 +477,11 @@ public class IpManagerTest { List list3 = Arrays.asList("bar", "baz"); List list4 = Arrays.asList("foo", "bar", "baz"); - assertTrue(IpManager.all(list1, (x) -> false)); - assertFalse(IpManager.all(list2, (x) -> false)); - assertTrue(IpManager.all(list3, (x) -> true)); - assertTrue(IpManager.all(list2, (x) -> x.charAt(0) == 'f')); - assertFalse(IpManager.all(list4, (x) -> x.charAt(0) == 'f')); + assertTrue(IpClient.all(list1, (x) -> false)); + assertFalse(IpClient.all(list2, (x) -> false)); + assertTrue(IpClient.all(list3, (x) -> true)); + assertTrue(IpClient.all(list2, (x) -> x.charAt(0) == 'f')); + assertFalse(IpClient.all(list4, (x) -> x.charAt(0) == 'f')); } @Test @@ -438,11 +491,11 @@ public class IpManagerTest { List list3 = Arrays.asList("bar", "baz"); List list4 = Arrays.asList("foo", "bar", "baz"); - assertFalse(IpManager.any(list1, (x) -> true)); - assertTrue(IpManager.any(list2, (x) -> true)); - assertTrue(IpManager.any(list2, (x) -> x.charAt(0) == 'f')); - assertFalse(IpManager.any(list3, (x) -> x.charAt(0) == 'f')); - assertTrue(IpManager.any(list4, (x) -> x.charAt(0) == 'f')); + assertFalse(IpClient.any(list1, (x) -> true)); + assertTrue(IpClient.any(list2, (x) -> true)); + assertTrue(IpClient.any(list2, (x) -> x.charAt(0) == 'f')); + assertFalse(IpClient.any(list3, (x) -> x.charAt(0) == 'f')); + assertTrue(IpClient.any(list4, (x) -> x.charAt(0) == 'f')); } @Test @@ -451,9 +504,9 @@ public class IpManagerTest { List list2 = Arrays.asList("foo"); List list3 = Arrays.asList("foo", "bar", "baz"); - assertEquals(list1, IpManager.findAll(list1, (x) -> true)); - assertEquals(list1, IpManager.findAll(list3, (x) -> false)); - assertEquals(list3, IpManager.findAll(list3, (x) -> true)); - assertEquals(list2, IpManager.findAll(list3, (x) -> x.charAt(0) == 'f')); + assertEquals(list1, IpClient.findAll(list1, (x) -> true)); + assertEquals(list1, IpClient.findAll(list3, (x) -> false)); + assertEquals(list3, IpClient.findAll(list3, (x) -> true)); + assertEquals(list2, IpClient.findAll(list3, (x) -> x.charAt(0) == 'f')); } } -- 2.11.0