Address comments on NetworkStack AIDL v6
Address issues found during AIDL review:
- Rename clientAddr to singleClientAddr
- Do not use a ParcelableBundle for notifyNetworkTested or
notifyDataStallSuspected; instead use AIDL parcelables for stronger
backwards compatibility guarantees.
Test: atest NetworkMonitorTest ConnectivityServiceTest
ConnectivityServiceIntegrationTest, manual
Bug: 153500847
Change-Id: Id9b71784e5f6294d203230e57737979e063ff0f8
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index 336a090..91a2a87 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -18,6 +18,14 @@
import static android.Manifest.permission.RECEIVE_DATA_ACTIVITY_CHANGE;
import static android.content.pm.PackageManager.PERMISSION_GRANTED;
+import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_PROBES_ATTEMPTED_BITMASK;
+import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_PROBES_SUCCEEDED_BITMASK;
+import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_VALIDATION_RESULT;
+import static android.net.ConnectivityDiagnosticsManager.DataStallReport.DETECTION_METHOD_DNS_EVENTS;
+import static android.net.ConnectivityDiagnosticsManager.DataStallReport.DETECTION_METHOD_TCP_METRICS;
+import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_DNS_CONSECUTIVE_TIMEOUTS;
+import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_TCP_METRICS_COLLECTION_PERIOD_MILLIS;
+import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_TCP_PACKET_FAIL_RATE;
import static android.net.ConnectivityManager.CONNECTIVITY_ACTION;
import static android.net.ConnectivityManager.NETID_UNSET;
import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC;
@@ -72,6 +80,7 @@
import android.net.ConnectivityDiagnosticsManager.ConnectivityReport;
import android.net.ConnectivityDiagnosticsManager.DataStallReport;
import android.net.ConnectivityManager;
+import android.net.DataStallReportParcelable;
import android.net.ICaptivePortal;
import android.net.IConnectivityDiagnosticsCallback;
import android.net.IConnectivityManager;
@@ -108,6 +117,7 @@
import android.net.NetworkStack;
import android.net.NetworkStackClient;
import android.net.NetworkState;
+import android.net.NetworkTestResultParcelable;
import android.net.NetworkUtils;
import android.net.NetworkWatchlistManager;
import android.net.PrivateDnsConfigParcel;
@@ -2811,14 +2821,6 @@
handleNetworkTested(nai, results.mTestResult,
(results.mRedirectUrl == null) ? "" : results.mRedirectUrl);
-
- // Invoke ConnectivityReport generation for this Network test event.
- final Message m =
- mConnectivityDiagnosticsHandler.obtainMessage(
- ConnectivityDiagnosticsHandler.EVENT_NETWORK_TESTED,
- new ConnectivityReportEvent(results.mTimestampMillis, nai));
- m.setData(msg.getData());
- mConnectivityDiagnosticsHandler.sendMessage(m);
break;
}
case EVENT_PROVISIONING_NOTIFICATION: {
@@ -2997,23 +2999,33 @@
@Override
public void notifyNetworkTested(int testResult, @Nullable String redirectUrl) {
- notifyNetworkTestedWithExtras(testResult, redirectUrl, SystemClock.elapsedRealtime(),
- PersistableBundle.EMPTY);
+ // Legacy version of notifyNetworkTestedWithExtras.
+ // Would only be called if the system has a NetworkStack module older than the
+ // framework, which does not happen in practice.
}
@Override
- public void notifyNetworkTestedWithExtras(
- int testResult,
- @Nullable String redirectUrl,
- long timestampMillis,
- @NonNull PersistableBundle extras) {
- final Message msg =
- mTrackerHandler.obtainMessage(
- EVENT_NETWORK_TESTED,
- new NetworkTestedResults(
- mNetId, testResult, timestampMillis, redirectUrl));
- msg.setData(new Bundle(extras));
+ public void notifyNetworkTestedWithExtras(NetworkTestResultParcelable p) {
+ final Message msg = mTrackerHandler.obtainMessage(
+ EVENT_NETWORK_TESTED,
+ new NetworkTestedResults(
+ mNetId, p.result, p.timestampMillis, p.redirectUrl));
mTrackerHandler.sendMessage(msg);
+
+ // Invoke ConnectivityReport generation for this Network test event.
+ final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(mNetId);
+ if (nai == null) return;
+ final Message m = mConnectivityDiagnosticsHandler.obtainMessage(
+ ConnectivityDiagnosticsHandler.EVENT_NETWORK_TESTED,
+ new ConnectivityReportEvent(p.timestampMillis, nai));
+
+ final PersistableBundle extras = new PersistableBundle();
+ extras.putInt(KEY_NETWORK_VALIDATION_RESULT, p.result);
+ extras.putInt(KEY_NETWORK_PROBES_SUCCEEDED_BITMASK, p.probesSucceeded);
+ extras.putInt(KEY_NETWORK_PROBES_ATTEMPTED_BITMASK, p.probesAttempted);
+
+ m.setData(new Bundle(extras));
+ mConnectivityDiagnosticsHandler.sendMessage(m);
}
@Override
@@ -3062,12 +3074,25 @@
}
@Override
- public void notifyDataStallSuspected(
- long timestampMillis, int detectionMethod, PersistableBundle extras) {
- final Message msg =
- mConnectivityDiagnosticsHandler.obtainMessage(
- ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED,
- detectionMethod, mNetId, timestampMillis);
+ public void notifyDataStallSuspected(DataStallReportParcelable p) {
+ final Message msg = mConnectivityDiagnosticsHandler.obtainMessage(
+ ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED,
+ p.detectionMethod, mNetId, p.timestampMillis);
+
+ final PersistableBundle extras = new PersistableBundle();
+ switch (p.detectionMethod) {
+ case DETECTION_METHOD_DNS_EVENTS:
+ extras.putInt(KEY_DNS_CONSECUTIVE_TIMEOUTS, p.dnsConsecutiveTimeouts);
+ break;
+ case DETECTION_METHOD_TCP_METRICS:
+ extras.putInt(KEY_TCP_PACKET_FAIL_RATE, p.tcpPacketFailRate);
+ extras.putInt(KEY_TCP_METRICS_COLLECTION_PERIOD_MILLIS,
+ p.tcpMetricsCollectionPeriodMillis);
+ break;
+ default:
+ log("Unknown data stall detection method, ignoring: " + p.detectionMethod);
+ return;
+ }
msg.setData(new Bundle(extras));
// NetworkStateTrackerHandler currently doesn't take any actions based on data
diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java
index a478e68..a992778 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -144,6 +144,7 @@
import android.net.ConnectivityManager.PacketKeepaliveCallback;
import android.net.ConnectivityManager.TooManyRequestsException;
import android.net.ConnectivityThread;
+import android.net.DataStallReportParcelable;
import android.net.IConnectivityDiagnosticsCallback;
import android.net.IDnsResolver;
import android.net.IIpConnectivityMetrics;
@@ -170,6 +171,7 @@
import android.net.NetworkStack;
import android.net.NetworkStackClient;
import android.net.NetworkState;
+import android.net.NetworkTestResultParcelable;
import android.net.NetworkUtils;
import android.net.ProxyInfo;
import android.net.ResolverParamsParcel;
@@ -196,7 +198,6 @@
import android.os.Parcel;
import android.os.ParcelFileDescriptor;
import android.os.Parcelable;
-import android.os.PersistableBundle;
import android.os.Process;
import android.os.RemoteException;
import android.os.SystemClock;
@@ -580,14 +581,6 @@
}
private class TestNetworkAgentWrapper extends NetworkAgentWrapper {
- private static final int VALIDATION_RESULT_BASE = NETWORK_VALIDATION_PROBE_DNS
- | NETWORK_VALIDATION_PROBE_HTTP
- | NETWORK_VALIDATION_PROBE_HTTPS;
- private static final int VALIDATION_RESULT_VALID = VALIDATION_RESULT_BASE
- | NETWORK_VALIDATION_RESULT_VALID;
- private static final int VALIDATION_RESULT_PARTIAL = VALIDATION_RESULT_BASE
- | NETWORK_VALIDATION_PROBE_FALLBACK
- | NETWORK_VALIDATION_RESULT_PARTIAL;
private static final int VALIDATION_RESULT_INVALID = 0;
private static final long DATA_STALL_TIMESTAMP = 10L;
@@ -595,12 +588,10 @@
private INetworkMonitor mNetworkMonitor;
private INetworkMonitorCallbacks mNmCallbacks;
- private int mNmValidationResult = VALIDATION_RESULT_BASE;
+ private int mNmValidationResult = VALIDATION_RESULT_INVALID;
private int mProbesCompleted;
private int mProbesSucceeded;
private String mNmValidationRedirectUrl = null;
- private PersistableBundle mValidationExtras = PersistableBundle.EMPTY;
- private PersistableBundle mDataStallExtras = PersistableBundle.EMPTY;
private boolean mNmProvNotificationRequested = false;
private final ConditionVariable mNetworkStatusReceived = new ConditionVariable();
@@ -668,8 +659,13 @@
}
mNmCallbacks.notifyProbeStatusChanged(mProbesCompleted, mProbesSucceeded);
- mNmCallbacks.notifyNetworkTestedWithExtras(
- mNmValidationResult, mNmValidationRedirectUrl, TIMESTAMP, mValidationExtras);
+ final NetworkTestResultParcelable p = new NetworkTestResultParcelable();
+ p.result = mNmValidationResult;
+ p.probesAttempted = mProbesCompleted;
+ p.probesSucceeded = mProbesSucceeded;
+ p.redirectUrl = mNmValidationRedirectUrl;
+ p.timestampMillis = TIMESTAMP;
+ mNmCallbacks.notifyNetworkTestedWithExtras(p);
if (mNmValidationRedirectUrl != null) {
mNmCallbacks.showProvisioningNotification(
@@ -751,9 +747,9 @@
}
void setNetworkValid(boolean isStrictMode) {
- mNmValidationResult = VALIDATION_RESULT_VALID;
+ mNmValidationResult = NETWORK_VALIDATION_RESULT_VALID;
mNmValidationRedirectUrl = null;
- int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTP;
+ int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS;
if (isStrictMode) {
probesSucceeded |= NETWORK_VALIDATION_PROBE_PRIVDNS;
}
@@ -765,8 +761,9 @@
void setNetworkInvalid(boolean isStrictMode) {
mNmValidationResult = VALIDATION_RESULT_INVALID;
mNmValidationRedirectUrl = null;
- int probesCompleted = VALIDATION_RESULT_BASE;
- int probesSucceeded = VALIDATION_RESULT_INVALID;
+ int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS
+ | NETWORK_VALIDATION_PROBE_HTTP;
+ int probesSucceeded = 0;
// If the isStrictMode is true, it means the network is invalid when NetworkMonitor
// tried to validate the private DNS but failed.
if (isStrictMode) {
@@ -782,7 +779,7 @@
mNmValidationRedirectUrl = redirectUrl;
// Suppose the portal is found when NetworkMonitor probes NETWORK_VALIDATION_PROBE_HTTP
// in the beginning, so the NETWORK_VALIDATION_PROBE_HTTPS hasn't probed yet.
- int probesCompleted = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS;
+ int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTP;
int probesSucceeded = VALIDATION_RESULT_INVALID;
if (isStrictMode) {
probesCompleted |= NETWORK_VALIDATION_PROBE_PRIVDNS;
@@ -791,18 +788,20 @@
}
void setNetworkPartial() {
- mNmValidationResult = VALIDATION_RESULT_PARTIAL;
+ mNmValidationResult = NETWORK_VALIDATION_RESULT_PARTIAL;
mNmValidationRedirectUrl = null;
- int probesCompleted = VALIDATION_RESULT_BASE;
- int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS;
+ int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS
+ | NETWORK_VALIDATION_PROBE_FALLBACK;
+ int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_FALLBACK;
setProbesStatus(probesCompleted, probesSucceeded);
}
void setNetworkPartialValid(boolean isStrictMode) {
setNetworkPartial();
- mNmValidationResult |= VALIDATION_RESULT_VALID;
- int probesCompleted = VALIDATION_RESULT_BASE;
- int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS;
+ mNmValidationResult |= NETWORK_VALIDATION_RESULT_VALID;
+ int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS
+ | NETWORK_VALIDATION_PROBE_HTTP;
+ int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTP;
// Suppose the partial network cannot pass the private DNS validation as well, so only
// add NETWORK_VALIDATION_PROBE_DNS in probesCompleted but not probesSucceeded.
if (isStrictMode) {
@@ -838,8 +837,10 @@
}
void notifyDataStallSuspected() throws Exception {
- mNmCallbacks.notifyDataStallSuspected(
- DATA_STALL_TIMESTAMP, DATA_STALL_DETECTION_METHOD, mDataStallExtras);
+ final DataStallReportParcelable p = new DataStallReportParcelable();
+ p.detectionMethod = DATA_STALL_DETECTION_METHOD;
+ p.timestampMillis = DATA_STALL_TIMESTAMP;
+ mNmCallbacks.notifyDataStallSuspected(p);
}
}