joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

Move EthernetNetworkFactory to using the NetworkProvider API

Browse source 
This CL makes EthernetNetworkFactory inherit from NetworkProvider rather
than NetworkFactory. The name of the class is purposefully unchanged to
make the code review easier (it will be changed to
EthernetNetworkProvider in a follow up).

As part of the conversion, NetworkInterfaceState now registers a
NetworkOffer when the link comes up and unregisters it when the link
goes down. It updates the existing offer when capabilities change (by
calling registerNetworkOffer with an already registered
NetworkOfferCallback).

This change should fix existing refCount issues. When a NetworkOffer is
first registered, it receives callbacks for all existing requests. This
is the main problem with the NetworkFactory implementation where only
one NetworkOffer is registered when the factory is first created; so
when interfaces come up, they do not receive callbacks for existing
requests.

Test: atest EthernetNetworkTest
Bug: 197548738
Change-Id: I5e8e4673d2ed04bc1a0c8d232a8772edfff65b5d
This commit is contained in:
Patrick Rohr 2022-04-08 12:04:34 +02:00
commit daca9ca74f
2 changed files with 62 additions and 180 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -31,10 +31,9 @@ import android.net.IpConfiguration.ProxySettings;
import android.net.LinkProperties;
import android.net.NetworkAgentConfig;
import android.net.NetworkCapabilities;
import android.net.NetworkFactory;
import android.net.NetworkProvider;
import android.net.NetworkRequest;
import android.net.NetworkSpecifier;
import android.net.NetworkScore;
import android.net.ip.IIpClient;
import android.net.ip.IpClientCallbacks;
import android.net.ip.IpClientManager;
@ -61,22 +60,19 @@ import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
/**
* {@link NetworkFactory} that represents Ethernet networks.
*
* This class reports a static network score of 70 when it is tracking an interface and that
* interface's link is up, and a score of 0 otherwise.
* {@link NetworkProvider} that manages NetworkOffers for Ethernet networks.
*/
public class EthernetNetworkFactory extends NetworkFactory {
public class EthernetNetworkFactory {
private final static String TAG = EthernetNetworkFactory.class.getSimpleName();
final static boolean DBG = true;
private final static int NETWORK_SCORE = 70;
private static final String NETWORK_TYPE = "Ethernet";
private final ConcurrentHashMap<String, NetworkInterfaceState> mTrackingInterfaces =
new ConcurrentHashMap<>();
private final Handler mHandler;
private final Context mContext;
private final NetworkProvider mProvider;
final Dependencies mDeps;
public static class Dependencies {
@ -111,54 +107,24 @@ public class EthernetNetworkFactory extends NetworkFactory {
}
public EthernetNetworkFactory(Handler handler, Context context) {
this(handler, context, new Dependencies());
this(handler, context, new NetworkProvider(context, handler.getLooper(), TAG),
new Dependencies());
}
@VisibleForTesting
EthernetNetworkFactory(Handler handler, Context context, Dependencies deps) {
super(handler.getLooper(), context, NETWORK_TYPE, createDefaultNetworkCapabilities());
EthernetNetworkFactory(Handler handler, Context context, NetworkProvider provider,
Dependencies deps) {
mHandler = handler;
mContext = context;
mProvider = provider;
mDeps = deps;
setScoreFilter(NETWORK_SCORE);
}
@Override
public boolean acceptRequest(NetworkRequest request) {
if (DBG) {
Log.d(TAG, "acceptRequest, request: " + request);
}
return networkForRequest(request) != null;
}
@Override
protected void needNetworkFor(NetworkRequest networkRequest) {
NetworkInterfaceState network = networkForRequest(networkRequest);
if (network == null) {
Log.e(TAG, "needNetworkFor, failed to get a network for " + networkRequest);
return;
}
if (++network.refCount == 1) {
network.start();
}
}
@Override
protected void releaseNetworkFor(NetworkRequest networkRequest) {
NetworkInterfaceState network = networkForRequest(networkRequest);
if (network == null) {
Log.e(TAG, "releaseNetworkFor, failed to get a network for " + networkRequest);
return;
}
if (--network.refCount == 0) {
network.stop();
}
/**
* Registers the network provider with the system.
*/
public void register() {
mContext.getSystemService(ConnectivityManager.class).registerNetworkProvider(mProvider);
}
/**
@ -196,9 +162,8 @@ public class EthernetNetworkFactory extends NetworkFactory {
}
final NetworkInterfaceState iface = new NetworkInterfaceState(
ifaceName, hwAddress, mHandler, mContext, ipConfig, nc, getProvider(), mDeps);
ifaceName, hwAddress, mHandler, mContext, ipConfig, nc, mProvider, mDeps);
mTrackingInterfaces.put(ifaceName, iface);
updateCapabilityFilter();
}
@VisibleForTesting
@ -239,7 +204,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
final NetworkInterfaceState iface = mTrackingInterfaces.get(ifaceName);
iface.updateInterface(ipConfig, capabilities, listener);
mTrackingInterfaces.put(ifaceName, iface);
updateCapabilityFilter();
}
private static NetworkCapabilities mixInCapabilities(NetworkCapabilities nc,
@ -250,16 +214,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
return builder.build();
}
private void updateCapabilityFilter() {
NetworkCapabilities capabilitiesFilter = createDefaultNetworkCapabilities();
for (NetworkInterfaceState iface: mTrackingInterfaces.values()) {
capabilitiesFilter = mixInCapabilities(capabilitiesFilter, iface.mCapabilities);
}
if (DBG) Log.d(TAG, "updateCapabilityFilter: " + capabilitiesFilter);
setCapabilityFilter(capabilitiesFilter);
}
private static NetworkCapabilities createDefaultNetworkCapabilities() {
return NetworkCapabilities.Builder
.withoutDefaultCapabilities()
@ -270,11 +224,8 @@ public class EthernetNetworkFactory extends NetworkFactory {
protected void removeInterface(String interfaceName) {
NetworkInterfaceState iface = mTrackingInterfaces.remove(interfaceName);
if (iface != null) {
iface.maybeSendNetworkManagementCallbackForAbort();
iface.stop();
iface.destroy();
}
updateCapabilityFilter();
}
/** Returns true if state has been modified */
@ -306,37 +257,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
return mTrackingInterfaces.containsKey(ifaceName);
}
private NetworkInterfaceState networkForRequest(NetworkRequest request) {
String requestedIface = null;
NetworkSpecifier specifier = request.getNetworkSpecifier();
if (specifier instanceof EthernetNetworkSpecifier) {
requestedIface = ((EthernetNetworkSpecifier) specifier)
.getInterfaceName();
}
NetworkInterfaceState network = null;
if (!TextUtils.isEmpty(requestedIface)) {
NetworkInterfaceState n = mTrackingInterfaces.get(requestedIface);
if (n != null && request.canBeSatisfiedBy(n.mCapabilities)) {
network = n;
}
} else {
for (NetworkInterfaceState n : mTrackingInterfaces.values()) {
if (request.canBeSatisfiedBy(n.mCapabilities) && n.mLinkUp) {
network = n;
break;
}
}
}
if (DBG) {
Log.i(TAG, "networkForRequest, request: " + request + ", network: " + network);
}
return network;
}
private static void maybeSendNetworkManagementCallback(
@Nullable final INetworkInterfaceOutcomeReceiver listener,
@Nullable final String iface,
@ -401,8 +321,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
ConnectivityManager.TYPE_NONE);
}
long refCount = 0;
private class EthernetIpClientCallback extends IpClientCallbacks {
private final ConditionVariable mIpClientStartCv = new ConditionVariable(false);
private final ConditionVariable mIpClientShutdownCv = new ConditionVariable(false);
@ -476,6 +394,9 @@ public class EthernetNetworkFactory extends NetworkFactory {
private class EthernetNetworkOfferCallback implements NetworkProvider.NetworkOfferCallback {
@Override
public void onNetworkNeeded(@NonNull NetworkRequest request) {
if (DBG) {
Log.d(TAG, String.format("%s: onNetworkNeeded for request: %s", name, request));
}
// When the network offer is first registered, onNetworkNeeded is called with all
// existing requests.
// ConnectivityService filters requests for us based on the NetworkCapabilities
@ -487,6 +408,10 @@ public class EthernetNetworkFactory extends NetworkFactory {
@Override
public void onNetworkUnneeded(@NonNull NetworkRequest request) {
if (DBG) {
Log.d(TAG,
String.format("%s: onNetworkUnneeded for request: %s", name, request));
}
mRequests.remove(request);
if (mRequests.isEmpty()) {
// not currently serving any requests, stop the network.
@ -529,9 +454,21 @@ public class EthernetNetworkFactory extends NetworkFactory {
+ "transport type.");
}
private static NetworkScore getBestNetworkScore() {
return new NetworkScore.Builder().build();
}
private void setCapabilities(@NonNull final NetworkCapabilities capabilities) {
mCapabilities = new NetworkCapabilities(capabilities);
mLegacyType = getLegacyType(mCapabilities);
if (mLinkUp) {
// registering a new network offer will update the existing one, not install a
// new one.
mNetworkProvider.registerNetworkOffer(getBestNetworkScore(),
new NetworkCapabilities(capabilities), cmd -> mHandler.post(cmd),
mNetworkOfferCallback);
}
}
void updateInterface(@Nullable final IpConfiguration ipConfig,
@ -693,20 +630,21 @@ public class EthernetNetworkFactory extends NetworkFactory {
mLinkUp = up;
if (!up) { // was up, goes down
// Send an abort on a provisioning request callback if necessary before stopping.
maybeSendNetworkManagementCallbackForAbort();
stop();
// retract network offer and stop IpClient.
destroy();
// If only setting the interface down, send a callback to signal completion.
EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, name, null);
} else { // was down, goes up
stop();
start(listener);
// register network offer
mNetworkProvider.registerNetworkOffer(getBestNetworkScore(),
new NetworkCapabilities(mCapabilities), (cmd) -> mHandler.post(cmd),
mNetworkOfferCallback);
}
return true;
}
void stop() {
private void stop() {
// Invalidate all previous start requests
if (mIpClient != null) {
mIpClient.shutdown();
@ -722,6 +660,13 @@ public class EthernetNetworkFactory extends NetworkFactory {
mLinkProperties.clear();
}
public void destroy() {
mNetworkProvider.unregisterNetworkOffer(mNetworkOfferCallback);
maybeSendNetworkManagementCallbackForAbort();
stop();
mRequests.clear();
}
private static void provisionIpClient(@NonNull final IpClientManager ipClient,
@NonNull final IpConfiguration config, @NonNull final String tcpBufferSizes) {
if (config.getProxySettings() == ProxySettings.STATIC ||
@ -761,7 +706,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
@Override
public String toString() {
return getClass().getSimpleName() + "{ "
+ "refCount: " + refCount + ", "
+ "iface: " + name + ", "
+ "up: " + mLinkUp + ", "
+ "hwAddress: " + mHwAddress + ", "
@ -774,7 +718,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
}
void dump(FileDescriptor fd, IndentingPrintWriter pw, String[] args) {
super.dump(fd, pw, args);
pw.println(getClass().getSimpleName());
pw.println("Tracking interfaces:");
pw.increaseIndent();

87
tests/unit/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
View file

@ -32,7 +32,6 @@ import static org.mockito.ArgumentMatchers.same;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@ -51,6 +50,7 @@ import android.net.Network;
import android.net.NetworkAgentConfig;
import android.net.NetworkCapabilities;
import android.net.NetworkProvider;
import android.net.NetworkProvider.NetworkOfferCallback;
import android.net.NetworkRequest;
import android.net.StaticIpConfiguration;
import android.net.ip.IpClientCallbacks;
@ -99,6 +99,7 @@ public class EthernetNetworkFactoryTest {
@Mock private EthernetNetworkAgent mNetworkAgent;
@Mock private InterfaceParams mInterfaceParams;
@Mock private Network mMockNetwork;
@Mock private NetworkProvider mNetworkProvider;
@Before
public void setUp() throws Exception {
@ -112,7 +113,7 @@ public class EthernetNetworkFactoryTest {
private void initEthernetNetworkFactory() {
mLooper = new TestLooper();
mHandler = new Handler(mLooper.getLooper());
mNetFactory = new EthernetNetworkFactory(mHandler, mContext, mDeps);
mNetFactory = new EthernetNetworkFactory(mHandler, mContext, mNetworkProvider, mDeps);
}
private void setupNetworkAgentMock() {
@ -239,9 +240,16 @@ public class EthernetNetworkFactoryTest {
mNetFactory.addInterface(iface, HW_ADDR, ipConfig,
createInterfaceCapsBuilder(transportType).build());
assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_LISTENER));
ArgumentCaptor<NetworkOfferCallback> captor = ArgumentCaptor.forClass(
NetworkOfferCallback.class);
verify(mNetworkProvider).registerNetworkOffer(any(), any(), any(), captor.capture());
captor.getValue().onNetworkNeeded(createDefaultRequest());
verifyStart(ipConfig);
clearInvocations(mDeps);
clearInvocations(mIpClient);
clearInvocations(mNetworkProvider);
}
// creates a provisioned interface
@ -281,28 +289,14 @@ public class EthernetNetworkFactoryTest {
// To create an unprovisioned interface, provision and then "stop" it, i.e. stop its
// NetworkAgent and IpClient. One way this can be done is by provisioning an interface and
// then calling onNetworkUnwanted.
createAndVerifyProvisionedInterface(iface);
mNetworkAgent.getCallbacks().onNetworkUnwanted();
mLooper.dispatchAll();
verifyStop();
mNetFactory.addInterface(iface, HW_ADDR, createDefaultIpConfig(),
createInterfaceCapsBuilder(NetworkCapabilities.TRANSPORT_ETHERNET).build());
assertTrue(mNetFactory.updateInterfaceLinkState(iface, true, NULL_LISTENER));
clearInvocations(mIpClient);
clearInvocations(mNetworkAgent);
}
@Test
public void testAcceptRequest() throws Exception {
initEthernetNetworkFactory();
createInterfaceUndergoingProvisioning(TEST_IFACE);
assertTrue(mNetFactory.acceptRequest(createDefaultRequest()));
NetworkRequest wifiRequest = createDefaultRequestBuilder()
.removeTransportType(NetworkCapabilities.TRANSPORT_ETHERNET)
.addTransportType(NetworkCapabilities.TRANSPORT_WIFI).build();
assertFalse(mNetFactory.acceptRequest(wifiRequest));
}
@Test
public void testUpdateInterfaceLinkStateForActiveProvisioningInterface() throws Exception {
initEthernetNetworkFactory();
@ -377,36 +371,6 @@ public class EthernetNetworkFactoryTest {
listener.expectOnError();
}
@Test
public void testNeedNetworkForOnProvisionedInterface() throws Exception {
initEthernetNetworkFactory();
createAndVerifyProvisionedInterface(TEST_IFACE);
mNetFactory.needNetworkFor(createDefaultRequest());
verify(mIpClient, never()).startProvisioning(any());
}
@Test
public void testNeedNetworkForOnUnprovisionedInterface() throws Exception {
initEthernetNetworkFactory();
createUnprovisionedInterface(TEST_IFACE);
mNetFactory.needNetworkFor(createDefaultRequest());
verify(mIpClient).startProvisioning(any());
triggerOnProvisioningSuccess();
verifyNetworkAgentRegistersAndConnects();
}
@Test
public void testNeedNetworkForOnInterfaceUndergoingProvisioning() throws Exception {
initEthernetNetworkFactory();
createInterfaceUndergoingProvisioning(TEST_IFACE);
mNetFactory.needNetworkFor(createDefaultRequest());
verify(mIpClient, never()).startProvisioning(any());
triggerOnProvisioningSuccess();
verifyNetworkAgentRegistersAndConnects();
}
@Test
public void testProvisioningLoss() throws Exception {
initEthernetNetworkFactory();
@ -440,31 +404,6 @@ public class EthernetNetworkFactoryTest {
verify(mIpClient, never()).startProvisioning(any());
}
@Test
public void testIpClientIsNotStartedWhenLinkIsDown() throws Exception {
initEthernetNetworkFactory();
createUnprovisionedInterface(TEST_IFACE);
mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER);
mNetFactory.needNetworkFor(createDefaultRequest());
verify(mDeps, never()).makeIpClient(any(), any(), any());
// BUG(b/191854824): requesting a network with a specifier (Android Auto use case) should
// not start an IpClient when the link is down, but fixing this may make matters worse by
// tiggering b/197548738.
NetworkRequest specificNetRequest = new NetworkRequest.Builder()
.addTransportType(NetworkCapabilities.TRANSPORT_ETHERNET)
.setNetworkSpecifier(new EthernetNetworkSpecifier(TEST_IFACE))
.build();
mNetFactory.needNetworkFor(specificNetRequest);
mNetFactory.releaseNetworkFor(specificNetRequest);
mNetFactory.updateInterfaceLinkState(TEST_IFACE, true, NULL_LISTENER);
// TODO: change to once when b/191854824 is fixed.
verify(mDeps, times(2)).makeIpClient(any(), eq(TEST_IFACE), any());
}
@Test
public void testLinkPropertiesChanged() throws Exception {
initEthernetNetworkFactory();