ethernet: clean up test interface tracking
This change adds a isValidEthernetInterface() similar to
isValidTestInterface() in place of updating the interface matching
regex. This makes the code more readable.
It's unfortunate that getInterfaceList() was added as a synchronous API
making its behavior race-y around setting setIncludeTestInterfaces().
Test: atest EthernetManagerTest
Change-Id: Ib0652654020fc3f6e965de9091c5bc960d1d4cdf
diff --git a/service-t/src/com/android/server/ethernet/EthernetTracker.java b/service-t/src/com/android/server/ethernet/EthernetTracker.java
index 31caabc..be9beed 100644
--- a/service-t/src/com/android/server/ethernet/EthernetTracker.java
+++ b/service-t/src/com/android/server/ethernet/EthernetTracker.java
@@ -87,15 +87,17 @@
private static final String TEST_IFACE_REGEXP = TEST_TAP_PREFIX + "\\d+";
/**
- * Interface names we track. This is a product-dependent regular expression, plus,
- * if setIncludeTestInterfaces is true, any test interfaces.
+ * Interface names we track. This is a product-dependent regular expression.
+ * Use isValidEthernetInterface to check if a interface name is a valid ethernet interface (this
+ * includes test interfaces if setIncludeTestInterfaces is set to true).
*/
- private volatile String mIfaceMatch;
+ private final String mIfaceMatch;
/**
* Track test interfaces if true, don't track otherwise.
+ * Volatile is needed as getInterfaceList() does not run on the handler thread.
*/
- private boolean mIncludeTestInterfaces = false;
+ private volatile boolean mIncludeTestInterfaces = false;
/** Mapping between {iface name | mac address} -> {NetworkCapabilities} */
private final ConcurrentHashMap<String, NetworkCapabilities> mNetworkCapabilities =
@@ -162,7 +164,7 @@
mDeps = deps;
// Interface match regex.
- updateIfaceMatchRegexp();
+ mIfaceMatch = mDeps.getInterfaceRegexFromResource(mContext);
// Read default Ethernet interface configuration from resources
final String[] interfaceConfigs = mDeps.getInterfaceConfigFromResource(context);
@@ -321,9 +323,17 @@
Log.e(TAG, "Could not get list of interfaces " + e);
return interfaceList;
}
- final String ifaceMatch = mIfaceMatch;
+
+ // There is a possible race with setIncludeTestInterfaces() which can affect
+ // isValidEthernetInterface (it returns true for test interfaces if setIncludeTestInterfaces
+ // is set to true).
+ // setIncludeTestInterfaces() is only used in tests, and since getInterfaceList() does not
+ // run on the handler thread, the behavior around setIncludeTestInterfaces() is
+ // indeterminate either way. This can easily be circumvented by waiting on a callback from
+ // a test interface after calling setIncludeTestInterfaces() before calling this function.
+ // In production code, this has no effect.
for (String iface : ifaces) {
- if (iface.matches(ifaceMatch)) interfaceList.add(iface);
+ if (isValidEthernetInterface(iface)) interfaceList.add(iface);
}
return interfaceList;
}
@@ -358,7 +368,6 @@
public void setIncludeTestInterfaces(boolean include) {
mHandler.post(() -> {
mIncludeTestInterfaces = include;
- updateIfaceMatchRegexp();
if (!include) {
removeTestData();
}
@@ -570,7 +579,7 @@
}
private void maybeTrackInterface(String iface) {
- if (!iface.matches(mIfaceMatch)) {
+ if (!isValidEthernetInterface(iface)) {
return;
}
@@ -841,12 +850,8 @@
return ret;
}
- private void updateIfaceMatchRegexp() {
- final String match = mDeps.getInterfaceRegexFromResource(mContext);
- mIfaceMatch = mIncludeTestInterfaces
- ? "(" + match + "|" + TEST_IFACE_REGEXP + ")"
- : match;
- Log.d(TAG, "Interface match regexp set to '" + mIfaceMatch + "'");
+ private boolean isValidEthernetInterface(String iface) {
+ return iface.matches(mIfaceMatch) || isValidTestInterface(iface);
}
/**