From 251f2fcf133dd099550d2ed9d70f769c775cae2d Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 19 Aug 2022 14:35:52 -0700 Subject: [PATCH 1/4] ethernet: add EthernetCallback class to wrap OutcomeReceiver The OutcomeReceiver that gets passed into the EthernetServiceImpl is hard to use (it is nullable and both onError and onResult can throw RemoteExceptions). Note: This is an intermediary state. The next step will be to completely tear EthernetCallback out of EthernetNetworkFactory, which will remove a lot of boilerplate code. Bug: 225317892 Test: atest EthernetManagerTest Change-Id: Ifc0e1ba29ded933c418e4b335cb731c3496d7e44 --- .../server/ethernet/EthernetCallback.java | 57 ++++++++++ .../ethernet/EthernetNetworkFactory.java | 104 ++++++++---------- .../server/ethernet/EthernetServiceImpl.java | 16 +-- .../server/ethernet/EthernetTracker.java | 20 ++-- .../ethernet/EthernetNetworkFactoryTest.java | 31 +++--- .../ethernet/EthernetServiceImplTest.java | 27 +++-- .../server/ethernet/EthernetTrackerTest.java | 13 +-- 7 files changed, 157 insertions(+), 111 deletions(-) create mode 100644 service-t/src/com/android/server/ethernet/EthernetCallback.java diff --git a/service-t/src/com/android/server/ethernet/EthernetCallback.java b/service-t/src/com/android/server/ethernet/EthernetCallback.java new file mode 100644 index 0000000000..5461156485 --- /dev/null +++ b/service-t/src/com/android/server/ethernet/EthernetCallback.java @@ -0,0 +1,57 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.ethernet; + +import android.net.EthernetNetworkManagementException; +import android.net.INetworkInterfaceOutcomeReceiver; +import android.os.RemoteException; +import android.util.Log; + +import com.android.internal.annotations.VisibleForTesting; + +/** Convenience wrapper for INetworkInterfaceOutcomeReceiver */ +@VisibleForTesting +public class EthernetCallback { + private static final String TAG = EthernetCallback.class.getSimpleName(); + private final INetworkInterfaceOutcomeReceiver mReceiver; + + public EthernetCallback(INetworkInterfaceOutcomeReceiver receiver) { + mReceiver = receiver; + } + + /** Calls INetworkInterfaceOutcomeReceiver#onResult */ + public void onResult(String ifname) { + try { + if (mReceiver != null) { + mReceiver.onResult(ifname); + } + } catch (RemoteException e) { + Log.e(TAG, "Failed to report error to OutcomeReceiver", e); + } + } + + /** Calls INetworkInterfaceOutcomeReceiver#onError */ + public void onError(String msg) { + try { + if (mReceiver != null) { + mReceiver.onError(new EthernetNetworkManagementException(msg)); + } + } catch (RemoteException e) { + Log.e(TAG, "Failed to report error to OutcomeReceiver", e); + } + } +} diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index e5bddf64fe..c4127b6cc0 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -22,9 +22,7 @@ import android.content.Context; import android.net.ConnectivityManager; import android.net.ConnectivityResources; import android.net.EthernetManager; -import android.net.EthernetNetworkManagementException; import android.net.EthernetNetworkSpecifier; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.IpConfiguration; import android.net.IpConfiguration.IpAssignment; import android.net.IpConfiguration.ProxySettings; @@ -42,7 +40,6 @@ import android.net.shared.ProvisioningConfiguration; import android.os.ConditionVariable; import android.os.Handler; import android.os.Looper; -import android.os.RemoteException; import android.text.TextUtils; import android.util.AndroidRuntimeException; import android.util.ArraySet; @@ -190,21 +187,21 @@ 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 listener an optional {@link INetworkInterfaceOutcomeReceiver} to notify callers of + * @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 INetworkInterfaceOutcomeReceiver listener) { + final EthernetCallback cb) { if (!hasInterface(ifaceName)) { - maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); + maybeSendNetworkManagementCallbackForUntracked(ifaceName, cb); return; } final NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName); - iface.updateInterface(ipConfig, capabilities, listener); + iface.updateInterface(ipConfig, capabilities, cb); mTrackingInterfaces.put(ifaceName, iface); } @@ -239,9 +236,9 @@ public class EthernetNetworkFactory { /** Returns true if state has been modified */ @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + final EthernetCallback cb) { if (!hasInterface(ifaceName)) { - maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); + maybeSendNetworkManagementCallbackForUntracked(ifaceName, cb); return false; } @@ -250,14 +247,13 @@ public class EthernetNetworkFactory { } NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName); - return iface.updateLinkState(up, listener); + return iface.updateLinkState(up, cb); } private void maybeSendNetworkManagementCallbackForUntracked( - String ifaceName, INetworkInterfaceOutcomeReceiver listener) { - maybeSendNetworkManagementCallback(listener, null, - new EthernetNetworkManagementException( - ifaceName + " can't be updated as it is not available.")); + String ifaceName, EthernetCallback cb) { + maybeSendNetworkManagementCallback(cb, null, + ifaceName + " can't be updated as it is not available."); } @VisibleForTesting @@ -266,21 +262,14 @@ public class EthernetNetworkFactory { } private static void maybeSendNetworkManagementCallback( - @Nullable final INetworkInterfaceOutcomeReceiver listener, + final EthernetCallback cb, @Nullable final String iface, - @Nullable final EthernetNetworkManagementException e) { - if (null == listener) { - return; - } - - try { - if (iface != null) { - listener.onResult(iface); - } else { - listener.onError(e); - } - } catch (RemoteException re) { - Log.e(TAG, "Can't send onComplete for network management callback", re); + @Nullable final String msg) { + Objects.requireNonNull(cb, "EthernetCallback may never be null"); + if (iface != null) { + cb.onResult(iface); + } else { + cb.onError(msg); } } @@ -332,10 +321,10 @@ public class EthernetNetworkFactory { private class EthernetIpClientCallback extends IpClientCallbacks { private final ConditionVariable mIpClientStartCv = new ConditionVariable(false); private final ConditionVariable mIpClientShutdownCv = new ConditionVariable(false); - @Nullable INetworkInterfaceOutcomeReceiver mNetworkManagementListener; + EthernetCallback mCb; - EthernetIpClientCallback(@Nullable final INetworkInterfaceOutcomeReceiver listener) { - mNetworkManagementListener = listener; + EthernetIpClientCallback(EthernetCallback cb) { + mCb = cb; } @Override @@ -372,14 +361,14 @@ public class EthernetNetworkFactory { @Override public void onProvisioningSuccess(LinkProperties newLp) { - handleIpEvent(() -> onIpLayerStarted(newLp, mNetworkManagementListener)); + handleIpEvent(() -> onIpLayerStarted(newLp)); } @Override 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(mNetworkManagementListener)); + handleIpEvent(() -> onIpLayerStopped(mCb)); } @Override @@ -492,12 +481,12 @@ public class EthernetNetworkFactory { void updateInterface(@Nullable final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + final EthernetCallback cb) { if (DBG) { Log.d(TAG, "updateInterface, iface: " + name + ", ipConfig: " + ipConfig + ", old ipConfig: " + mIpConfig + ", capabilities: " + capabilities + ", old capabilities: " + mCapabilities - + ", listener: " + listener + + ", cb: " + cb ); } @@ -510,7 +499,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(listener); + restart(cb); } boolean isRestricted() { @@ -518,10 +507,11 @@ public class EthernetNetworkFactory { } private void start() { - start(null); + // TODO: remove EthernetCallback from this class entirely. + start(new EthernetCallback(null)); } - private void start(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + private void start(EthernetCallback cb) { if (mIpClient != null) { if (DBG) Log.d(TAG, "IpClient already started"); return; @@ -530,7 +520,7 @@ public class EthernetNetworkFactory { Log.d(TAG, String.format("Starting Ethernet IpClient(%s)", name)); } - mIpClientCallback = new EthernetIpClientCallback(listener); + mIpClientCallback = new EthernetIpClientCallback(cb); mDeps.makeIpClient(mContext, name, mIpClientCallback); mIpClientCallback.awaitIpClientStart(); @@ -540,8 +530,7 @@ public class EthernetNetworkFactory { provisionIpClient(mIpClient, mIpConfig, sTcpBufferSizes); } - void onIpLayerStarted(@NonNull final LinkProperties linkProperties, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + void onIpLayerStarted(@NonNull final LinkProperties linkProperties) { if (mNetworkAgent != null) { Log.e(TAG, "Already have a NetworkAgent - aborting new request"); stop(); @@ -576,7 +565,7 @@ public class EthernetNetworkFactory { realizeNetworkManagementCallback(name, null); } - void onIpLayerStopped(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + void onIpLayerStopped(EthernetCallback cb) { // 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. @@ -586,27 +575,25 @@ public class EthernetNetworkFactory { maybeSendNetworkManagementCallbackForAbort(); return; } - restart(listener); + restart(cb); } private void maybeSendNetworkManagementCallbackForAbort() { - realizeNetworkManagementCallback(null, - new EthernetNetworkManagementException( - "The IP provisioning request has been aborted.")); + realizeNetworkManagementCallback(null, "The IP provisioning request has been aborted."); } // Must be called on the handler thread private void realizeNetworkManagementCallback(@Nullable final String iface, - @Nullable final EthernetNetworkManagementException e) { + @Nullable String msg) { ensureRunningOnEthernetHandlerThread(); if (null == mIpClientCallback) { return; } EthernetNetworkFactory.maybeSendNetworkManagementCallback( - mIpClientCallback.mNetworkManagementListener, iface, e); - // Only send a single callback per listener. - mIpClientCallback.mNetworkManagementListener = null; + mIpClientCallback.mCb, iface, msg); + // Only send a single callback per cb. + mIpClientCallback.mCb = new EthernetCallback(null /* null */); } private void ensureRunningOnEthernetHandlerThread() { @@ -636,12 +623,10 @@ public class EthernetNetworkFactory { } /** Returns true if state has been modified */ - boolean updateLinkState(final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + boolean updateLinkState(final boolean up, final EthernetCallback cb) { if (mLinkUp == up) { - EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, null, - new EthernetNetworkManagementException( - "No changes with requested link state " + up + " for " + name)); + EthernetNetworkFactory.maybeSendNetworkManagementCallback(cb, null, + "No changes with requested link state " + up + " for " + name); return false; } mLinkUp = up; @@ -654,7 +639,7 @@ public class EthernetNetworkFactory { registerNetworkOffer(); } - EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, name, null); + EthernetNetworkFactory.maybeSendNetworkManagementCallback(cb, name, null); return true; } @@ -723,13 +708,14 @@ public class EthernetNetworkFactory { } void restart() { - restart(null); + // TODO: remove EthernetCallback from this class entirely + restart(new EthernetCallback(null)); } - void restart(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + void restart(EthernetCallback cb) { if (DBG) Log.d(TAG, "reconnecting Ethernet"); stop(); - start(listener); + start(cb); } @Override diff --git a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java index dae3d2a9d4..edf04b2570 100644 --- a/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java +++ b/service-t/src/com/android/server/ethernet/EthernetServiceImpl.java @@ -260,7 +260,7 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { @Override public void updateConfiguration(@NonNull final String iface, @NonNull final EthernetNetworkUpdateRequest request, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final INetworkInterfaceOutcomeReceiver cb) { Objects.requireNonNull(iface); Objects.requireNonNull(request); throwIfEthernetNotStarted(); @@ -277,31 +277,31 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { } mTracker.updateConfiguration( - iface, request.getIpConfiguration(), nc, listener); + iface, request.getIpConfiguration(), nc, new EthernetCallback(cb)); } @Override public void enableInterface(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { - Log.i(TAG, "enableInterface called with: iface=" + iface + ", listener=" + listener); + @Nullable final INetworkInterfaceOutcomeReceiver cb) { + Log.i(TAG, "enableInterface called with: iface=" + iface + ", cb=" + cb); Objects.requireNonNull(iface); throwIfEthernetNotStarted(); enforceAdminPermission(iface, false, "enableInterface()"); - mTracker.enableInterface(iface, listener); + mTracker.enableInterface(iface, new EthernetCallback(cb)); } @Override public void disableInterface(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { - Log.i(TAG, "disableInterface called with: iface=" + iface + ", listener=" + listener); + @Nullable final INetworkInterfaceOutcomeReceiver cb) { + Log.i(TAG, "disableInterface called with: iface=" + iface + ", cb=" + cb); Objects.requireNonNull(iface); throwIfEthernetNotStarted(); enforceAdminPermission(iface, false, "disableInterface()"); - mTracker.disableInterface(iface, listener); + mTracker.disableInterface(iface, new EthernetCallback(cb)); } @Override diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index ba367cff64..a24a11945f 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -29,7 +29,6 @@ import android.net.ConnectivityResources; import android.net.EthernetManager; import android.net.IEthernetServiceListener; import android.net.INetd; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.ITetheredInterfaceCallback; import android.net.InterfaceConfigurationParcel; import android.net.IpConfiguration; @@ -335,7 +334,7 @@ public class EthernetTracker { protected void updateConfiguration(@NonNull final String iface, @Nullable final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final EthernetCallback cb) { if (DBG) { Log.i(TAG, "updateConfiguration, iface: " + iface + ", capabilities: " + capabilities + ", ipConfig: " + ipConfig); @@ -353,21 +352,21 @@ public class EthernetTracker { mNetworkCapabilities.put(iface, capabilities); } mHandler.post(() -> { - mFactory.updateInterface(iface, localIpConfig, capabilities, listener); + mFactory.updateInterface(iface, localIpConfig, capabilities, cb); broadcastInterfaceStateChange(iface); }); } @VisibleForTesting(visibility = PACKAGE) protected void enableInterface(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { - mHandler.post(() -> updateInterfaceState(iface, true, listener)); + @Nullable final EthernetCallback cb) { + mHandler.post(() -> updateInterfaceState(iface, true, cb)); } @VisibleForTesting(visibility = PACKAGE) protected void disableInterface(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { - mHandler.post(() -> updateInterfaceState(iface, false, listener)); + @Nullable final EthernetCallback cb) { + mHandler.post(() -> updateInterfaceState(iface, false, cb)); } IpConfiguration getIpConfiguration(String iface) { @@ -614,14 +613,15 @@ public class EthernetTracker { } private void updateInterfaceState(String iface, boolean up) { - updateInterfaceState(iface, up, null /* listener */); + // TODO: pull EthernetCallbacks out of EthernetNetworkFactory. + updateInterfaceState(iface, up, new EthernetCallback(null /* cb */)); } private void updateInterfaceState(@NonNull final String iface, final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final EthernetCallback cb) { final int mode = getInterfaceMode(iface); final boolean factoryLinkStateUpdated = (mode == INTERFACE_MODE_CLIENT) - && mFactory.updateInterfaceLinkState(iface, up, listener); + && mFactory.updateInterfaceLinkState(iface, up, cb); if (factoryLinkStateUpdated) { broadcastInterfaceStateChange(iface); diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 503d920e04..3b5a9b28b0 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -40,7 +40,6 @@ import android.content.res.Resources; import android.net.ConnectivityManager; import android.net.EthernetNetworkManagementException; import android.net.EthernetNetworkSpecifier; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.IpConfiguration; import android.net.LinkAddress; import android.net.LinkProperties; @@ -55,7 +54,6 @@ import android.net.ip.IpClientCallbacks; import android.net.ip.IpClientManager; import android.os.Build; import android.os.Handler; -import android.os.IBinder; import android.os.Looper; import android.os.test.TestLooper; @@ -84,7 +82,7 @@ import java.util.concurrent.TimeUnit; public class EthernetNetworkFactoryTest { private static final int TIMEOUT_MS = 2_000; private static final String TEST_IFACE = "test123"; - private static final INetworkInterfaceOutcomeReceiver NULL_LISTENER = null; + private static final EthernetCallback NULL_CB = new EthernetCallback(null); private static final String IP_ADDR = "192.0.2.2/25"; private static final LinkAddress LINK_ADDR = new LinkAddress(IP_ADDR); private static final String HW_ADDR = "01:02:03:04:05:06"; @@ -241,7 +239,7 @@ public class EthernetNetworkFactoryTest { final IpConfiguration ipConfig = createDefaultIpConfig(); mNetFactory.addInterface(iface, HW_ADDR, ipConfig, createInterfaceCapsBuilder(transportType).build()); - assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_LISTENER)); + assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_CB)); ArgumentCaptor captor = ArgumentCaptor.forClass( NetworkOfferCallback.class); @@ -295,7 +293,7 @@ public class EthernetNetworkFactoryTest { // then calling onNetworkUnwanted. mNetFactory.addInterface(iface, HW_ADDR, createDefaultIpConfig(), createInterfaceCapsBuilder(NetworkCapabilities.TRANSPORT_ETHERNET).build()); - assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_LISTENER)); + assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_CB)); clearInvocations(mIpClient); clearInvocations(mNetworkAgent); @@ -572,17 +570,21 @@ public class EthernetNetworkFactoryTest { } private static final class TestNetworkManagementListener - implements INetworkInterfaceOutcomeReceiver { + extends EthernetCallback { private final CompletableFuture mResult = new CompletableFuture<>(); + TestNetworkManagementListener() { + super(null); + } + @Override public void onResult(@NonNull String iface) { mResult.complete(iface); } @Override - public void onError(@NonNull EthernetNetworkManagementException exception) { - mResult.completeExceptionally(exception); + public void onError(String msg) { + mResult.completeExceptionally(new EthernetNetworkManagementException(msg)); } String expectOnResult() throws Exception { @@ -598,11 +600,6 @@ public class EthernetNetworkFactoryTest { } }); } - - @Override - public IBinder asBinder() { - return null; - } } @Test @@ -632,7 +629,7 @@ public class EthernetNetworkFactoryTest { initEthernetNetworkFactory(); verifyNetworkManagementCallIsAbortedWhenInterrupted( TEST_IFACE, - () -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER)); + () -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_CB)); } @Test @@ -718,7 +715,7 @@ public class EthernetNetworkFactoryTest { final IpConfiguration initialIpConfig = createStaticIpConfig(); mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/, - null /*listener*/); + new EthernetCallback(null /* cb */)); triggerOnProvisioningSuccess(); verifyRestart(initialIpConfig); @@ -730,7 +727,7 @@ public class EthernetNetworkFactoryTest { // verify that sending a null ipConfig does not update the current ipConfig. mNetFactory.updateInterface(TEST_IFACE, null /*ipConfig*/, null /*capabilities*/, - null /*listener*/); + new EthernetCallback(null /* cb */)); triggerOnProvisioningSuccess(); verifyRestart(initialIpConfig); } @@ -739,7 +736,7 @@ public class EthernetNetworkFactoryTest { public void testOnNetworkNeededOnStaleNetworkOffer() throws Exception { initEthernetNetworkFactory(); createAndVerifyProvisionedInterface(TEST_IFACE); - mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, null); + mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, new EthernetCallback(null)); verify(mNetworkProvider).unregisterNetworkOffer(mNetworkOfferCallback); // It is possible that even after a network offer is unregistered, CS still sends it // onNetworkNeeded() callbacks. diff --git a/tests/unit/java/com/android/server/ethernet/EthernetServiceImplTest.java b/tests/unit/java/com/android/server/ethernet/EthernetServiceImplTest.java index a1d93a025e..9bf893a58b 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetServiceImplTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetServiceImplTest.java @@ -21,6 +21,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_TEST; import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; @@ -209,7 +210,8 @@ public class EthernetServiceImplTest { NULL_LISTENER); verify(mEthernetTracker).updateConfiguration(eq(TEST_IFACE), eq(UPDATE_REQUEST_WITHOUT_CAPABILITIES.getIpConfiguration()), - eq(UPDATE_REQUEST_WITHOUT_CAPABILITIES.getNetworkCapabilities()), isNull()); + eq(UPDATE_REQUEST_WITHOUT_CAPABILITIES.getNetworkCapabilities()), + any(EthernetCallback.class)); } private void denyManageEthPermission() { @@ -285,7 +287,8 @@ public class EthernetServiceImplTest { verify(mEthernetTracker).updateConfiguration( eq(TEST_IFACE), eq(UPDATE_REQUEST.getIpConfiguration()), - eq(UPDATE_REQUEST.getNetworkCapabilities()), eq(NULL_LISTENER)); + eq(UPDATE_REQUEST.getNetworkCapabilities()), + any(EthernetCallback.class)); } @Test @@ -303,19 +306,20 @@ public class EthernetServiceImplTest { verify(mEthernetTracker).updateConfiguration( eq(TEST_IFACE), isNull(), - eq(ncWithSpecifier), eq(NULL_LISTENER)); + eq(ncWithSpecifier), any(EthernetCallback.class)); } @Test public void testEnableInterface() { mEthernetServiceImpl.enableInterface(TEST_IFACE, NULL_LISTENER); - verify(mEthernetTracker).enableInterface(eq(TEST_IFACE), eq(NULL_LISTENER)); + verify(mEthernetTracker).enableInterface(eq(TEST_IFACE), + any(EthernetCallback.class)); } @Test public void testDisableInterface() { mEthernetServiceImpl.disableInterface(TEST_IFACE, NULL_LISTENER); - verify(mEthernetTracker).disableInterface(eq(TEST_IFACE), eq(NULL_LISTENER)); + verify(mEthernetTracker).disableInterface(eq(TEST_IFACE), any(EthernetCallback.class)); } @Test @@ -328,7 +332,7 @@ public class EthernetServiceImplTest { mEthernetServiceImpl.updateConfiguration(TEST_IFACE, request, NULL_LISTENER); verify(mEthernetTracker).updateConfiguration(eq(TEST_IFACE), eq(request.getIpConfiguration()), - eq(request.getNetworkCapabilities()), isNull()); + eq(request.getNetworkCapabilities()), any(EthernetCallback.class)); } @Test @@ -337,7 +341,8 @@ public class EthernetServiceImplTest { NULL_LISTENER); verify(mEthernetTracker).updateConfiguration(eq(TEST_IFACE), eq(UPDATE_REQUEST_WITHOUT_IP_CONFIG.getIpConfiguration()), - eq(UPDATE_REQUEST_WITHOUT_IP_CONFIG.getNetworkCapabilities()), isNull()); + eq(UPDATE_REQUEST_WITHOUT_IP_CONFIG.getNetworkCapabilities()), + any(EthernetCallback.class)); } @Test @@ -369,7 +374,7 @@ public class EthernetServiceImplTest { verify(mEthernetTracker).updateConfiguration( eq(TEST_IFACE), eq(request.getIpConfiguration()), - eq(request.getNetworkCapabilities()), eq(NULL_LISTENER)); + eq(request.getNetworkCapabilities()), any(EthernetCallback.class)); } @Test @@ -379,7 +384,8 @@ public class EthernetServiceImplTest { denyManageEthPermission(); mEthernetServiceImpl.enableInterface(TEST_IFACE, NULL_LISTENER); - verify(mEthernetTracker).enableInterface(eq(TEST_IFACE), eq(NULL_LISTENER)); + verify(mEthernetTracker).enableInterface(eq(TEST_IFACE), + any(EthernetCallback.class)); } @Test @@ -389,7 +395,8 @@ public class EthernetServiceImplTest { denyManageEthPermission(); mEthernetServiceImpl.disableInterface(TEST_IFACE, NULL_LISTENER); - verify(mEthernetTracker).disableInterface(eq(TEST_IFACE), eq(NULL_LISTENER)); + verify(mEthernetTracker).disableInterface(eq(TEST_IFACE), + any(EthernetCallback.class)); } private void denyPermissions(String... permissions) { diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index 0376a2a85f..3e1e4a5510 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -39,7 +39,6 @@ import android.content.Context; import android.net.EthernetManager; import android.net.IEthernetServiceListener; import android.net.INetd; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.InetAddresses; import android.net.InterfaceConfigurationParcel; import android.net.IpConfiguration; @@ -76,7 +75,7 @@ public class EthernetTrackerTest { private static final String TEST_IFACE = "test123"; private static final int TIMEOUT_MS = 1_000; private static final String THREAD_NAME = "EthernetServiceThread"; - private static final INetworkInterfaceOutcomeReceiver NULL_LISTENER = null; + private static final EthernetCallback NULL_CB = new EthernetCallback(null); private EthernetTracker tracker; private HandlerThread mHandlerThread; @Mock private Context mContext; @@ -344,7 +343,7 @@ public class EthernetTrackerTest { new StaticIpConfiguration.Builder().setIpAddress(linkAddr).build(); final IpConfiguration ipConfig = new IpConfiguration.Builder().setStaticIpConfiguration(staticIpConfig).build(); - final INetworkInterfaceOutcomeReceiver listener = null; + final EthernetCallback listener = new EthernetCallback(null); tracker.updateConfiguration(TEST_IFACE, ipConfig, capabilities, listener); waitForIdle(); @@ -355,20 +354,20 @@ public class EthernetTrackerTest { @Test public void testEnableInterfaceCorrectlyCallsFactory() { - tracker.enableInterface(TEST_IFACE, NULL_LISTENER); + tracker.enableInterface(TEST_IFACE, NULL_CB); waitForIdle(); verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(true /* up */), - eq(NULL_LISTENER)); + eq(NULL_CB)); } @Test public void testDisableInterfaceCorrectlyCallsFactory() { - tracker.disableInterface(TEST_IFACE, NULL_LISTENER); + tracker.disableInterface(TEST_IFACE, NULL_CB); waitForIdle(); verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(false /* up */), - eq(NULL_LISTENER)); + eq(NULL_CB)); } @Test From de40e137a1daf643129df0cf3e9a9fb347dbe342 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 19 Aug 2022 15:20:27 -0700 Subject: [PATCH 2/4] ethernet: remove callback boilerplate from EthernetNetworkFactory Now that everything uses EthernetCallback, there is no need for the boilerplate code in EthernetNetworkFactory anymore. The next step will be to pull the callbacks out of EthernetNetworkFactory entirely. Bug: 225317892 Test: atest EthernetNetworkFactory Change-Id: I4958df658f7d5852660a5c03a8c9220532e42d3c --- .../ethernet/EthernetNetworkFactory.java | 55 ++++--------------- 1 file changed, 10 insertions(+), 45 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index c4127b6cc0..64cb7353ed 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -196,7 +196,7 @@ public class EthernetNetworkFactory { @Nullable final NetworkCapabilities capabilities, final EthernetCallback cb) { if (!hasInterface(ifaceName)) { - maybeSendNetworkManagementCallbackForUntracked(ifaceName, cb); + cb.onError(ifaceName + " is not tracked by the ethernet service"); return; } @@ -238,7 +238,7 @@ public class EthernetNetworkFactory { protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up, final EthernetCallback cb) { if (!hasInterface(ifaceName)) { - maybeSendNetworkManagementCallbackForUntracked(ifaceName, cb); + cb.onError(ifaceName + " is not tracked by the ethernet service"); return false; } @@ -250,29 +250,11 @@ public class EthernetNetworkFactory { return iface.updateLinkState(up, cb); } - private void maybeSendNetworkManagementCallbackForUntracked( - String ifaceName, EthernetCallback cb) { - maybeSendNetworkManagementCallback(cb, null, - ifaceName + " can't be updated as it is not available."); - } - @VisibleForTesting protected boolean hasInterface(String ifaceName) { return mTrackingInterfaces.containsKey(ifaceName); } - private static void maybeSendNetworkManagementCallback( - final EthernetCallback cb, - @Nullable final String iface, - @Nullable final String msg) { - Objects.requireNonNull(cb, "EthernetCallback may never be null"); - if (iface != null) { - cb.onResult(iface); - } else { - cb.onError(msg); - } - } - @VisibleForTesting static class NetworkInterfaceState { final String name; @@ -562,7 +544,7 @@ public class EthernetNetworkFactory { }); mNetworkAgent.register(); mNetworkAgent.markConnected(); - realizeNetworkManagementCallback(name, null); + mIpClientCallback.mCb.onResult(name); } void onIpLayerStopped(EthernetCallback cb) { @@ -572,30 +554,12 @@ public class EthernetNetworkFactory { 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. - maybeSendNetworkManagementCallbackForAbort(); + mIpClientCallback.mCb.onError("IP provisioning has been aborted."); return; } restart(cb); } - private void maybeSendNetworkManagementCallbackForAbort() { - realizeNetworkManagementCallback(null, "The IP provisioning request has been aborted."); - } - - // Must be called on the handler thread - private void realizeNetworkManagementCallback(@Nullable final String iface, - @Nullable String msg) { - ensureRunningOnEthernetHandlerThread(); - if (null == mIpClientCallback) { - return; - } - - EthernetNetworkFactory.maybeSendNetworkManagementCallback( - mIpClientCallback.mCb, iface, msg); - // Only send a single callback per cb. - mIpClientCallback.mCb = new EthernetCallback(null /* null */); - } - private void ensureRunningOnEthernetHandlerThread() { if (mHandler.getLooper().getThread() != Thread.currentThread()) { throw new IllegalStateException( @@ -625,8 +589,7 @@ public class EthernetNetworkFactory { /** Returns true if state has been modified */ boolean updateLinkState(final boolean up, final EthernetCallback cb) { if (mLinkUp == up) { - EthernetNetworkFactory.maybeSendNetworkManagementCallback(cb, null, - "No changes with requested link state " + up + " for " + name); + cb.onError("No changes with requested link state " + up + " for " + name); return false; } mLinkUp = up; @@ -639,7 +602,7 @@ public class EthernetNetworkFactory { registerNetworkOffer(); } - EthernetNetworkFactory.maybeSendNetworkManagementCallback(cb, name, null); + cb.onResult(name); return true; } @@ -651,8 +614,10 @@ public class EthernetNetworkFactory { mIpClient = null; } // Send an abort callback if an updateInterface request was in progress. - maybeSendNetworkManagementCallbackForAbort(); - mIpClientCallback = null; + if (mIpClientCallback != null) { + mIpClientCallback.mCb.onError("IP provisioning has been aborted."); + mIpClientCallback = null; + } if (mNetworkAgent != null) { mNetworkAgent.unregister(); From ea5c6dd136fd50d5a5897d9991eb5e84ea4e5564 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Fri, 19 Aug 2022 16:12:14 -0700 Subject: [PATCH 3/4] 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 --- .../ethernet/EthernetNetworkFactory.java | 49 +++------- .../server/ethernet/EthernetTracker.java | 14 ++- .../ethernet/EthernetNetworkFactoryTest.java | 90 ++----------------- .../server/ethernet/EthernetTrackerTest.java | 2 +- 4 files changed, 29 insertions(+), 126 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 64cb7353ed..51c750c1c4 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 @@ 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, - 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 @@ 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, - 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 @@ 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.mCb.onError("IP provisioning has been aborted."); - mIpClientCallback = null; - } + + 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 diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index a24a11945f..8b956fa53f 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 @@ 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); - 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 3b5a9b28b0..dcacf234e8 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/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*/, - new EthernetCallback(null /* cb */)); + mNetFactory.updateInterface(TEST_IFACE, initialIpConfig, null /*capabilities*/); + 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*/, - 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 3e1e4a5510..1df56c73dd 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 @@ public class EthernetTrackerTest { waitForIdle(); verify(mFactory).updateInterface( - eq(TEST_IFACE), eq(ipConfig), eq(capabilities), eq(listener)); + eq(TEST_IFACE), eq(ipConfig), eq(capabilities)); } @Test From 750036b286ee40c3aa25361569ccc21fd9a50a14 Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Mon, 22 Aug 2022 09:39:25 -0700 Subject: [PATCH 4/4] ethernet: remove callback from factory updateInterfaceLinkState updateInterfaceLinkState already returns true / false depending on the success, so the EthernetCallback can be removed and sent from EthernetTracker instead. Test: atest EthernetManagerTest Bug: 225317892 Change-Id: Id6ae7929097840ac5c9a4d244fdf669a06b94ed5 --- .../ethernet/EthernetNetworkFactory.java | 10 +-- .../server/ethernet/EthernetTracker.java | 9 ++- .../ethernet/EthernetNetworkFactoryTest.java | 75 +++---------------- .../server/ethernet/EthernetTrackerTest.java | 8 +- 4 files changed, 23 insertions(+), 79 deletions(-) diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java index 51c750c1c4..56c21eba22 100644 --- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java @@ -232,10 +232,8 @@ public class EthernetNetworkFactory { /** Returns true if state has been modified */ @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up, - final EthernetCallback cb) { + protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up) { if (!hasInterface(ifaceName)) { - cb.onError(ifaceName + " is not tracked by the ethernet service"); return false; } @@ -244,7 +242,7 @@ public class EthernetNetworkFactory { } NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName); - return iface.updateLinkState(up, cb); + return iface.updateLinkState(up); } @VisibleForTesting @@ -570,9 +568,8 @@ public class EthernetNetworkFactory { } /** Returns true if state has been modified */ - boolean updateLinkState(final boolean up, final EthernetCallback cb) { + boolean updateLinkState(final boolean up) { if (mLinkUp == up) { - cb.onError("No changes with requested link state " + up + " for " + name); return false; } mLinkUp = up; @@ -585,7 +582,6 @@ public class EthernetNetworkFactory { registerNetworkOffer(); } - cb.onResult(name); return true; } diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java index 8b956fa53f..77addcf6ff 100644 --- a/service-t/src/com/android/server/ethernet/EthernetTracker.java +++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java @@ -629,10 +629,17 @@ public class EthernetTracker { @Nullable final EthernetCallback cb) { final int mode = getInterfaceMode(iface); final boolean factoryLinkStateUpdated = (mode == INTERFACE_MODE_CLIENT) - && mFactory.updateInterfaceLinkState(iface, up, cb); + && mFactory.updateInterfaceLinkState(iface, up); if (factoryLinkStateUpdated) { broadcastInterfaceStateChange(iface); + cb.onResult(iface); + } else { + // The interface may already be in the correct state. While usually this should not be + // an error, since updateInterfaceState is used in setInterfaceEnabled() / + // setInterfaceDisabled() it has to be reported as such. + // It is also possible that the interface disappeared or is in server mode. + cb.onError("Failed to set link state " + (up ? "up" : "down") + " for " + iface); } } diff --git a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index dcacf234e8..aad80d51d0 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -20,7 +20,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; @@ -38,7 +37,6 @@ import android.app.test.MockAnswerUtil.AnswerWithArguments; import android.content.Context; import android.content.res.Resources; import android.net.ConnectivityManager; -import android.net.EthernetNetworkManagementException; import android.net.EthernetNetworkSpecifier; import android.net.IpConfiguration; import android.net.LinkAddress; @@ -72,9 +70,6 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; import java.util.Objects; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.TimeUnit; @SmallTest @RunWith(DevSdkIgnoreRunner.class) @@ -82,7 +77,6 @@ import java.util.concurrent.TimeUnit; public class EthernetNetworkFactoryTest { private static final int TIMEOUT_MS = 2_000; private static final String TEST_IFACE = "test123"; - private static final EthernetCallback NULL_CB = new EthernetCallback(null); private static final String IP_ADDR = "192.0.2.2/25"; private static final LinkAddress LINK_ADDR = new LinkAddress(IP_ADDR); private static final String HW_ADDR = "01:02:03:04:05:06"; @@ -239,7 +233,7 @@ public class EthernetNetworkFactoryTest { final IpConfiguration ipConfig = createDefaultIpConfig(); mNetFactory.addInterface(iface, HW_ADDR, ipConfig, createInterfaceCapsBuilder(transportType).build()); - assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_CB)); + assertTrue(mNetFactory.updateInterfaceLinkState(iface, true)); ArgumentCaptor captor = ArgumentCaptor.forClass( NetworkOfferCallback.class); @@ -293,7 +287,7 @@ public class EthernetNetworkFactoryTest { // then calling onNetworkUnwanted. mNetFactory.addInterface(iface, HW_ADDR, createDefaultIpConfig(), createInterfaceCapsBuilder(NetworkCapabilities.TRANSPORT_ETHERNET).build()); - assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_CB)); + assertTrue(mNetFactory.updateInterfaceLinkState(iface, true)); clearInvocations(mIpClient); clearInvocations(mNetworkAgent); @@ -303,81 +297,63 @@ public class EthernetNetworkFactoryTest { public void testUpdateInterfaceLinkStateForActiveProvisioningInterface() throws Exception { initEthernetNetworkFactory(); createInterfaceUndergoingProvisioning(TEST_IFACE); - final TestNetworkManagementListener listener = new TestNetworkManagementListener(); // verify that the IpClient gets shut down when interface state changes to down. - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */, listener); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */); assertTrue(ret); verify(mIpClient).shutdown(); - assertEquals(TEST_IFACE, listener.expectOnResult()); } @Test public void testUpdateInterfaceLinkStateForProvisionedInterface() throws Exception { initEthernetNetworkFactory(); createAndVerifyProvisionedInterface(TEST_IFACE); - final TestNetworkManagementListener listenerDown = new TestNetworkManagementListener(); - final TestNetworkManagementListener listenerUp = new TestNetworkManagementListener(); - final boolean retDown = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */, listenerDown); + final boolean retDown = mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */); assertTrue(retDown); verifyStop(); - assertEquals(TEST_IFACE, listenerDown.expectOnResult()); - final boolean retUp = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */, listenerUp); + final boolean retUp = mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */); assertTrue(retUp); - assertEquals(TEST_IFACE, listenerUp.expectOnResult()); } @Test public void testUpdateInterfaceLinkStateForUnprovisionedInterface() throws Exception { initEthernetNetworkFactory(); createUnprovisionedInterface(TEST_IFACE); - final TestNetworkManagementListener listener = new TestNetworkManagementListener(); - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */, listener); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, false /* up */); assertTrue(ret); // There should not be an active IPClient or NetworkAgent. verify(mDeps, never()).makeIpClient(any(), any(), any()); verify(mDeps, never()) .makeEthernetNetworkAgent(any(), any(), any(), any(), any(), any(), any()); - assertEquals(TEST_IFACE, listener.expectOnResult()); } @Test public void testUpdateInterfaceLinkStateForNonExistingInterface() throws Exception { initEthernetNetworkFactory(); - final TestNetworkManagementListener listener = new TestNetworkManagementListener(); // if interface was never added, link state cannot be updated. - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */, listener); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */); assertFalse(ret); verifyNoStopOrStart(); - listener.expectOnError(); } @Test public void testUpdateInterfaceLinkStateWithNoChanges() throws Exception { initEthernetNetworkFactory(); createAndVerifyProvisionedInterface(TEST_IFACE); - final TestNetworkManagementListener listener = new TestNetworkManagementListener(); - final boolean ret = - mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */, listener); + final boolean ret = mNetFactory.updateInterfaceLinkState(TEST_IFACE, true /* up */); assertFalse(ret); verifyNoStopOrStart(); - listener.expectOnError(); } @Test @@ -569,39 +545,6 @@ public class EthernetNetworkFactoryTest { verify(mNetworkAgent).markConnected(); } - private static final class TestNetworkManagementListener - extends EthernetCallback { - private final CompletableFuture mResult = new CompletableFuture<>(); - - TestNetworkManagementListener() { - super(null); - } - - @Override - public void onResult(@NonNull String iface) { - mResult.complete(iface); - } - - @Override - public void onError(String msg) { - mResult.completeExceptionally(new EthernetNetworkManagementException(msg)); - } - - String expectOnResult() throws Exception { - return mResult.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); - } - - void expectOnError() throws Exception { - assertThrows(EthernetNetworkManagementException.class, () -> { - try { - mResult.get(); - } catch (ExecutionException e) { - throw e.getCause(); - } - }); - } - } - @Test public void testUpdateInterfaceRestartsAgentCorrectly() throws Exception { initEthernetNetworkFactory(); @@ -656,7 +599,7 @@ public class EthernetNetworkFactoryTest { public void testOnNetworkNeededOnStaleNetworkOffer() throws Exception { initEthernetNetworkFactory(); createAndVerifyProvisionedInterface(TEST_IFACE); - mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, new EthernetCallback(null)); + mNetFactory.updateInterfaceLinkState(TEST_IFACE, false); verify(mNetworkProvider).unregisterNetworkOffer(mNetworkOfferCallback); // It is possible that even after a network offer is unregistered, CS still sends it // onNetworkNeeded() callbacks. diff --git a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java index 1df56c73dd..082a016241 100644 --- a/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/unit/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -87,7 +87,7 @@ public class EthernetTrackerTest { public void setUp() throws RemoteException { MockitoAnnotations.initMocks(this); initMockResources(); - when(mFactory.updateInterfaceLinkState(anyString(), anyBoolean(), any())).thenReturn(false); + when(mFactory.updateInterfaceLinkState(anyString(), anyBoolean())).thenReturn(false); when(mNetd.interfaceGetList()).thenReturn(new String[0]); mHandlerThread = new HandlerThread(THREAD_NAME); mHandlerThread.start(); @@ -357,8 +357,7 @@ public class EthernetTrackerTest { tracker.enableInterface(TEST_IFACE, NULL_CB); waitForIdle(); - verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(true /* up */), - eq(NULL_CB)); + verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(true /* up */)); } @Test @@ -366,8 +365,7 @@ public class EthernetTrackerTest { tracker.disableInterface(TEST_IFACE, NULL_CB); waitForIdle(); - verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(false /* up */), - eq(NULL_CB)); + verify(mFactory).updateInterfaceLinkState(eq(TEST_IFACE), eq(false /* up */)); } @Test