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

service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java

@@ -187,22 +187,19 @@ public class EthernetNetworkFactory {
      *                     {@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,
+            @Nullable final NetworkCapabilities capabilities) {
-            final EthernetCallback cb) {
         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 @@ public class EthernetNetworkFactory {
         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 class EthernetNetworkFactory {
             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 @@ public class EthernetNetworkFactory {
         }
 
         void updateInterface(@Nullable final IpConfiguration ipConfig,
-                @Nullable final NetworkCapabilities capabilities,
+                @Nullable final NetworkCapabilities capabilities) {
-                final EthernetCallback cb) {
             if (DBG) {
                 Log.d(TAG, "updateInterface, iface: " + name
                         + ", ipConfig: " + ipConfig + ", old ipConfig: " + mIpConfig
                         + ", capabilities: " + capabilities + ", old capabilities: " + mCapabilities
-                        + ", cb: " + cb
                 );
             }
 
@@ -481,7 +471,7 @@ public class EthernetNetworkFactory {
             // 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 @@ public class EthernetNetworkFactory {
         }
 
         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 @@ public class EthernetNetworkFactory {
                 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 @@ public class EthernetNetworkFactory {
                     });
             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 @@ public class EthernetNetworkFactory {
                 mIpClientCallback.awaitIpClientShutdown();
                 mIpClient = null;
             }
-            // Send an abort callback if an updateInterface request was in progress.
+
-            if (mIpClientCallback != null) {
+            mIpClientCallback = null;
-                mIpClientCallback.mCb.onError("IP provisioning has been aborted.");
-                mIpClientCallback = null;
-            }
 
             if (mNetworkAgent != null) {
                 mNetworkAgent.unregister();
@@ -673,14 +653,9 @@ public class EthernetNetworkFactory {
         }
 
         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

service-t/src/com/android/server/ethernet/EthernetTracker.java

@@ -270,7 +270,7 @@ public class EthernetTracker {
         }
         writeIpConfiguration(iface, ipConfiguration);
         mHandler.post(() -> {
-            mFactory.updateInterface(iface, ipConfiguration, null, null);
+            mFactory.updateInterface(iface, ipConfiguration, null);
             broadcastInterfaceStateChange(iface);
         });
     }
@@ -352,8 +352,16 @@ public class EthernetTracker {
             mNetworkCapabilities.put(iface, capabilities);
         }
         mHandler.post(() -> {
-            mFactory.updateInterface(iface, localIpConfig, capabilities, cb);
+            mFactory.updateInterface(iface, localIpConfig, capabilities);
-            broadcastInterfaceStateChange(iface);
+
+            // 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);
         });
     }
 

tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java

@@ -602,93 +602,16 @@ public class EthernetNetworkFactoryTest {
         }
     }
 
-    @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 @@ public class EthernetNetworkFactoryTest {
         // 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 @@ public class EthernetNetworkFactoryTest {
         createAndVerifyProvisionedInterface(TEST_IFACE);
 
         final IpConfiguration initialIpConfig = createStaticIpConfig();
-        mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/,
+        mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/);
-                new EthernetCallback(null /* cb */));
+
         triggerOnProvisioningSuccess();
         verifyRestart(initialIpConfig);
 
@@ -726,8 +647,7 @@ public class EthernetNetworkFactoryTest {
 
 
         // verify that sending a null ipConfig does not update the current ipConfig.
-        mNetFactory.updateInterface(TEST_IFACE, null /*ipConfig*/, null /*capabilities*/,
+        mNetFactory.updateInterface(TEST_IFACE, null /*ipConfig*/, null /*capabilities*/);
-                new EthernetCallback(null /* cb */));
         triggerOnProvisioningSuccess();
         verifyRestart(initialIpConfig);
     }

tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java

@@ -349,7 +349,7 @@ public class EthernetTrackerTest {
         waitForIdle();
 
         verify(mFactory).updateInterface(
-                eq(TEST_IFACE), eq(ipConfig), eq(capabilities), eq(listener));
+                eq(TEST_IFACE), eq(ipConfig), eq(capabilities));
     }
 
     @Test