ethernet: updateConfiguration result should not rely on ip provisioning
In addition, the result should always be success if the new
configuration is stored. This allows updating interfaces that are not
currently registered with ethernet service.
Bug: 236312641
Test: atest EthernetManagerTest
Change-Id: I70474ef5f046c65a6f74161137e25debd8dfe612
diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
index 64cb735..51c750c 100644
--- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
+++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
@@ -187,22 +187,19 @@
* {@code null} is passed, then the network's current
* {@link NetworkCapabilities} will be used in support of existing APIs as
* the public API does not allow this.
- * @param cb a {@link EthernetCallback} to notify callers of
- * completion.
*/
@VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE)
protected void updateInterface(@NonNull final String ifaceName,
@Nullable final IpConfiguration ipConfig,
- @Nullable final NetworkCapabilities capabilities,
- final EthernetCallback cb) {
+ @Nullable final NetworkCapabilities capabilities) {
if (!hasInterface(ifaceName)) {
- cb.onError(ifaceName + " is not tracked by the ethernet service");
return;
}
final NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName);
- iface.updateInterface(ipConfig, capabilities, cb);
+ iface.updateInterface(ipConfig, capabilities);
mTrackingInterfaces.put(ifaceName, iface);
+ return;
}
private static NetworkCapabilities mixInCapabilities(NetworkCapabilities nc,
@@ -303,11 +300,6 @@
private class EthernetIpClientCallback extends IpClientCallbacks {
private final ConditionVariable mIpClientStartCv = new ConditionVariable(false);
private final ConditionVariable mIpClientShutdownCv = new ConditionVariable(false);
- EthernetCallback mCb;
-
- EthernetIpClientCallback(EthernetCallback cb) {
- mCb = cb;
- }
@Override
public void onIpClientCreated(IIpClient ipClient) {
@@ -350,7 +342,7 @@
public void onProvisioningFailure(LinkProperties newLp) {
// This cannot happen due to provisioning timeout, because our timeout is 0. It can
// happen due to errors while provisioning or on provisioning loss.
- handleIpEvent(() -> onIpLayerStopped(mCb));
+ handleIpEvent(() -> onIpLayerStopped());
}
@Override
@@ -462,13 +454,11 @@
}
void updateInterface(@Nullable final IpConfiguration ipConfig,
- @Nullable final NetworkCapabilities capabilities,
- final EthernetCallback cb) {
+ @Nullable final NetworkCapabilities capabilities) {
if (DBG) {
Log.d(TAG, "updateInterface, iface: " + name
+ ", ipConfig: " + ipConfig + ", old ipConfig: " + mIpConfig
+ ", capabilities: " + capabilities + ", old capabilities: " + mCapabilities
- + ", cb: " + cb
);
}
@@ -481,7 +471,7 @@
// TODO: Update this logic to only do a restart if required. Although a restart may
// be required due to the capabilities or ipConfiguration values, not all
// capabilities changes require a restart.
- restart(cb);
+ restart();
}
boolean isRestricted() {
@@ -489,11 +479,6 @@
}
private void start() {
- // TODO: remove EthernetCallback from this class entirely.
- start(new EthernetCallback(null));
- }
-
- private void start(EthernetCallback cb) {
if (mIpClient != null) {
if (DBG) Log.d(TAG, "IpClient already started");
return;
@@ -502,7 +487,7 @@
Log.d(TAG, String.format("Starting Ethernet IpClient(%s)", name));
}
- mIpClientCallback = new EthernetIpClientCallback(cb);
+ mIpClientCallback = new EthernetIpClientCallback();
mDeps.makeIpClient(mContext, name, mIpClientCallback);
mIpClientCallback.awaitIpClientStart();
@@ -544,20 +529,18 @@
});
mNetworkAgent.register();
mNetworkAgent.markConnected();
- mIpClientCallback.mCb.onResult(name);
}
- void onIpLayerStopped(EthernetCallback cb) {
+ void onIpLayerStopped() {
// There is no point in continuing if the interface is gone as stop() will be triggered
// by removeInterface() when processed on the handler thread and start() won't
// work for a non-existent interface.
if (null == mDeps.getNetworkInterfaceByName(name)) {
if (DBG) Log.d(TAG, name + " is no longer available.");
// Send a callback in case a provisioning request was in progress.
- mIpClientCallback.mCb.onError("IP provisioning has been aborted.");
return;
}
- restart(cb);
+ restart();
}
private void ensureRunningOnEthernetHandlerThread() {
@@ -613,11 +596,8 @@
mIpClientCallback.awaitIpClientShutdown();
mIpClient = null;
}
- // Send an abort callback if an updateInterface request was in progress.
- if (mIpClientCallback != null) {
- mIpClientCallback.mCb.onError("IP provisioning has been aborted.");
- mIpClientCallback = null;
- }
+
+ mIpClientCallback = null;
if (mNetworkAgent != null) {
mNetworkAgent.unregister();
@@ -673,14 +653,9 @@
}
void restart() {
- // TODO: remove EthernetCallback from this class entirely
- restart(new EthernetCallback(null));
- }
-
- void restart(EthernetCallback cb) {
if (DBG) Log.d(TAG, "reconnecting Ethernet");
stop();
- start(cb);
+ start();
}
@Override
diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java
index a24a119..8b956fa 100644
--- a/service-t/src/com/android/server/ethernet/EthernetTracker.java
+++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java
@@ -270,7 +270,7 @@
}
writeIpConfiguration(iface, ipConfiguration);
mHandler.post(() -> {
- mFactory.updateInterface(iface, ipConfiguration, null, null);
+ mFactory.updateInterface(iface, ipConfiguration, null);
broadcastInterfaceStateChange(iface);
});
}
@@ -352,8 +352,16 @@
mNetworkCapabilities.put(iface, capabilities);
}
mHandler.post(() -> {
- mFactory.updateInterface(iface, localIpConfig, capabilities, cb);
- broadcastInterfaceStateChange(iface);
+ mFactory.updateInterface(iface, localIpConfig, capabilities);
+
+ // only broadcast state change when the ip configuration is updated.
+ if (ipConfig != null) {
+ broadcastInterfaceStateChange(iface);
+ }
+ // Always return success. Even if the interface does not currently exist, the
+ // IpConfiguration and NetworkCapabilities were saved and will be applied if an
+ // interface with the given name is ever added.
+ cb.onResult(iface);
});
}
diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
index 3b5a9b2..dcacf23 100644
--- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
+++ b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
@@ -603,92 +603,15 @@
}
@Test
- public void testUpdateInterfaceCallsListenerCorrectlyOnSuccess() throws Exception {
- initEthernetNetworkFactory();
- createAndVerifyProvisionedInterface(TEST_IFACE);
- final NetworkCapabilities capabilities = createDefaultFilterCaps();
- final IpConfiguration ipConfiguration = createStaticIpConfig();
- final TestNetworkManagementListener listener = new TestNetworkManagementListener();
-
- mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener);
- triggerOnProvisioningSuccess();
-
- assertEquals(TEST_IFACE, listener.expectOnResult());
- }
-
- @Test
- public void testUpdateInterfaceAbortsOnConcurrentRemoveInterface() throws Exception {
- initEthernetNetworkFactory();
- verifyNetworkManagementCallIsAbortedWhenInterrupted(
- TEST_IFACE,
- () -> mNetFactory.removeInterface(TEST_IFACE));
- }
-
- @Test
- public void testUpdateInterfaceAbortsOnConcurrentUpdateInterfaceLinkState() throws Exception {
- initEthernetNetworkFactory();
- verifyNetworkManagementCallIsAbortedWhenInterrupted(
- TEST_IFACE,
- () -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_CB));
- }
-
- @Test
- public void testUpdateInterfaceAbortsOnNetworkUneededRemovesAllRequests() throws Exception {
- initEthernetNetworkFactory();
- verifyNetworkManagementCallIsAbortedWhenInterrupted(
- TEST_IFACE,
- () -> mNetworkOfferCallback.onNetworkUnneeded(mRequestToKeepNetworkUp));
- }
-
- @Test
- public void testUpdateInterfaceCallsListenerCorrectlyOnConcurrentRequests() throws Exception {
- initEthernetNetworkFactory();
- final NetworkCapabilities capabilities = createDefaultFilterCaps();
- final IpConfiguration ipConfiguration = createStaticIpConfig();
- final TestNetworkManagementListener successfulListener =
- new TestNetworkManagementListener();
-
- // If two calls come in before the first one completes, the first listener will be aborted
- // and the second one will be successful.
- verifyNetworkManagementCallIsAbortedWhenInterrupted(
- TEST_IFACE,
- () -> {
- mNetFactory.updateInterface(
- TEST_IFACE, ipConfiguration, capabilities, successfulListener);
- triggerOnProvisioningSuccess();
- });
-
- assertEquals(successfulListener.expectOnResult(), TEST_IFACE);
- assertEquals(TEST_IFACE, successfulListener.expectOnResult());
- }
-
- private void verifyNetworkManagementCallIsAbortedWhenInterrupted(
- @NonNull final String iface,
- @NonNull final Runnable interruptingRunnable) throws Exception {
- createAndVerifyProvisionedInterface(iface);
- final NetworkCapabilities capabilities = createDefaultFilterCaps();
- final IpConfiguration ipConfiguration = createStaticIpConfig();
- final TestNetworkManagementListener failedListener = new TestNetworkManagementListener();
-
- // An active update request will be aborted on interrupt prior to provisioning completion.
- mNetFactory.updateInterface(iface, ipConfiguration, capabilities, failedListener);
- interruptingRunnable.run();
-
- failedListener.expectOnError();
- }
-
- @Test
public void testUpdateInterfaceRestartsAgentCorrectly() throws Exception {
initEthernetNetworkFactory();
createAndVerifyProvisionedInterface(TEST_IFACE);
final NetworkCapabilities capabilities = createDefaultFilterCaps();
final IpConfiguration ipConfiguration = createStaticIpConfig();
- final TestNetworkManagementListener listener = new TestNetworkManagementListener();
- mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener);
+ mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities);
triggerOnProvisioningSuccess();
- assertEquals(TEST_IFACE, listener.expectOnResult());
verify(mDeps).makeEthernetNetworkAgent(any(), any(),
eq(capabilities), any(), any(), any(), any());
verifyRestart(ipConfiguration);
@@ -700,12 +623,10 @@
// No interface exists due to not calling createAndVerifyProvisionedInterface(...).
final NetworkCapabilities capabilities = createDefaultFilterCaps();
final IpConfiguration ipConfiguration = createStaticIpConfig();
- final TestNetworkManagementListener listener = new TestNetworkManagementListener();
- mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener);
+ mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities);
verifyNoStopOrStart();
- listener.expectOnError();
}
@Test
@@ -714,8 +635,8 @@
createAndVerifyProvisionedInterface(TEST_IFACE);
final IpConfiguration initialIpConfig = createStaticIpConfig();
- mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/,
- new EthernetCallback(null /* cb */));
+ mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/);
+
triggerOnProvisioningSuccess();
verifyRestart(initialIpConfig);
@@ -726,8 +647,7 @@
// verify that sending a null ipConfig does not update the current ipConfig.
- mNetFactory.updateInterface(TEST_IFACE, null /*ipConfig*/, null /*capabilities*/,
- new EthernetCallback(null /* cb */));
+ mNetFactory.updateInterface(TEST_IFACE, null /*ipConfig*/, null /*capabilities*/);
triggerOnProvisioningSuccess();
verifyRestart(initialIpConfig);
}
diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
index 3e1e4a5..1df56c7 100644
--- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
+++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java
@@ -349,7 +349,7 @@
waitForIdle();
verify(mFactory).updateInterface(
- eq(TEST_IFACE), eq(ipConfig), eq(capabilities), eq(listener));
+ eq(TEST_IFACE), eq(ipConfig), eq(capabilities));
}
@Test