From eae7a2294481ab5c45b9c3b43858a71e570d554f Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Tue, 25 Jul 2017 11:40:56 +0900 Subject: [PATCH] Logging improvements when NetworkCapabilities change This patch improves the wtf() logging in updateCapabilities to better distinguish between the cases of a changed specifiers, changed transports, or changed capabilities. The case of NOT_METERED being added or removed is ignored. Bug: 63326103 Test: runtest frameworks-net, runtest frameworks-wifi Merged-In: I05c6e78891e1eac658f1cf883223af520a9a4f8f Merged-In: I4f6cbc0adb461cef6610460daeba72ca38b8f10c Merged-In: I165a8bbe8362100f1e2bb909459fb45b1c68d5ae Merged-In: Iec6d92e9a3a12bab87c5adfaf17f776465077060 Merged-In: I633d6347a7f852c27c03fc96b36ca2a60f70c73c Merged-In: I38739184fc0db105bfd3b4c02cce01e803739e5d Merged-In: Ia58b877056e2442136cc8b145ac8f4e6560cfc2c (cherry pick from commit 683ea489d302b494ab40c0d5dc97d352a59d8aa9) Change-Id: Id32ca66068c8ff549627e8e8c0e50897ef928c58 --- core/java/android/net/NetworkCapabilities.java | 126 ++++++++++++++------- .../com/android/server/ConnectivityService.java | 10 +- 2 files changed, 93 insertions(+), 43 deletions(-) diff --git a/core/java/android/net/NetworkCapabilities.java b/core/java/android/net/NetworkCapabilities.java index 305cf7695e5e..d2af0235809d 100644 --- a/core/java/android/net/NetworkCapabilities.java +++ b/core/java/android/net/NetworkCapabilities.java @@ -24,6 +24,7 @@ import com.android.internal.util.BitUtils; import com.android.internal.util.Preconditions; import java.util.Objects; +import java.util.StringJoiner; /** * This class represents the capabilities of a network. This is used both to specify @@ -347,11 +348,6 @@ public final class NetworkCapabilities implements Parcelable { return (nc.mNetworkCapabilities == this.mNetworkCapabilities); } - private boolean equalsNetCapabilitiesImmutable(NetworkCapabilities that) { - return ((this.mNetworkCapabilities & ~MUTABLE_CAPABILITIES) == - (that.mNetworkCapabilities & ~MUTABLE_CAPABILITIES)); - } - private boolean equalsNetCapabilitiesRequestable(NetworkCapabilities that) { return ((this.mNetworkCapabilities & ~NON_REQUESTABLE_CAPABILITIES) == (that.mNetworkCapabilities & ~NON_REQUESTABLE_CAPABILITIES)); @@ -502,10 +498,12 @@ public final class NetworkCapabilities implements Parcelable { private void combineTransportTypes(NetworkCapabilities nc) { this.mTransportTypes |= nc.mTransportTypes; } + private boolean satisfiedByTransportTypes(NetworkCapabilities nc) { return ((this.mTransportTypes == 0) || ((this.mTransportTypes & nc.mTransportTypes) != 0)); } + /** @hide */ public boolean equalsTransportTypes(NetworkCapabilities nc) { return (nc.mTransportTypes == this.mTransportTypes); @@ -760,15 +758,43 @@ public final class NetworkCapabilities implements Parcelable { /** * Checks that our immutable capabilities are the same as those of the given - * {@code NetworkCapabilities}. + * {@code NetworkCapabilities} and return a String describing any difference. + * The returned String is empty if there is no difference. * * @hide */ - public boolean equalImmutableCapabilities(NetworkCapabilities nc) { - if (nc == null) return false; - return (equalsNetCapabilitiesImmutable(nc) && - equalsTransportTypes(nc) && - equalsSpecifier(nc)); + public String describeImmutableDifferences(NetworkCapabilities that) { + if (that == null) { + return "other NetworkCapabilities was null"; + } + + StringJoiner joiner = new StringJoiner(", "); + + // TODO: consider only enforcing that capabilities are not removed, allowing addition. + // Ignore NOT_METERED being added or removed as it is effectively dynamic. http://b/63326103 + // TODO: properly support NOT_METERED as a mutable and requestable capability. + final long mask = ~MUTABLE_CAPABILITIES & ~NET_CAPABILITY_NOT_METERED; + long oldImmutableCapabilities = this.mNetworkCapabilities & mask; + long newImmutableCapabilities = that.mNetworkCapabilities & mask; + if (oldImmutableCapabilities != newImmutableCapabilities) { + String before = capabilityNamesOf(BitUtils.unpackBits(oldImmutableCapabilities)); + String after = capabilityNamesOf(BitUtils.unpackBits(newImmutableCapabilities)); + joiner.add(String.format("immutable capabilities changed: %s -> %s", before, after)); + } + + if (!equalsSpecifier(that)) { + NetworkSpecifier before = this.getNetworkSpecifier(); + NetworkSpecifier after = that.getNetworkSpecifier(); + joiner.add(String.format("specifier changed: %s -> %s", before, after)); + } + + if (!equalsTransportTypes(that)) { + String before = transportNamesOf(this.getTransportTypes()); + String after = transportNamesOf(that.getTransportTypes()); + joiner.add(String.format("transports changed: %s -> %s", before, after)); + } + + return joiner.toString(); } /** @@ -843,33 +869,15 @@ public final class NetworkCapabilities implements Parcelable { @Override public String toString() { + // TODO: enumerate bits for transports and capabilities instead of creating arrays. + // TODO: use a StringBuilder instead of string concatenation. int[] types = getTransportTypes(); String transports = (types.length > 0) ? " Transports: " + transportNamesOf(types) : ""; types = getCapabilities(); String capabilities = (types.length > 0 ? " Capabilities: " : ""); for (int i = 0; i < types.length; ) { - switch (types[i]) { - case NET_CAPABILITY_MMS: capabilities += "MMS"; break; - case NET_CAPABILITY_SUPL: capabilities += "SUPL"; break; - case NET_CAPABILITY_DUN: capabilities += "DUN"; break; - case NET_CAPABILITY_FOTA: capabilities += "FOTA"; break; - case NET_CAPABILITY_IMS: capabilities += "IMS"; break; - case NET_CAPABILITY_CBS: capabilities += "CBS"; break; - case NET_CAPABILITY_WIFI_P2P: capabilities += "WIFI_P2P"; break; - case NET_CAPABILITY_IA: capabilities += "IA"; break; - case NET_CAPABILITY_RCS: capabilities += "RCS"; break; - case NET_CAPABILITY_XCAP: capabilities += "XCAP"; break; - case NET_CAPABILITY_EIMS: capabilities += "EIMS"; break; - case NET_CAPABILITY_NOT_METERED: capabilities += "NOT_METERED"; break; - case NET_CAPABILITY_INTERNET: capabilities += "INTERNET"; break; - case NET_CAPABILITY_NOT_RESTRICTED: capabilities += "NOT_RESTRICTED"; break; - case NET_CAPABILITY_TRUSTED: capabilities += "TRUSTED"; break; - case NET_CAPABILITY_NOT_VPN: capabilities += "NOT_VPN"; break; - case NET_CAPABILITY_VALIDATED: capabilities += "VALIDATED"; break; - case NET_CAPABILITY_CAPTIVE_PORTAL: capabilities += "CAPTIVE_PORTAL"; break; - case NET_CAPABILITY_FOREGROUND: capabilities += "FOREGROUND"; break; - } + capabilities += capabilityNameOf(types[i]); if (++i < types.length) capabilities += "&"; } @@ -889,15 +897,55 @@ public final class NetworkCapabilities implements Parcelable { /** * @hide */ - public static String transportNamesOf(int[] types) { - if (types == null || types.length == 0) { - return ""; + public static String capabilityNamesOf(int[] capabilities) { + StringJoiner joiner = new StringJoiner("|"); + if (capabilities != null) { + for (int c : capabilities) { + joiner.add(capabilityNameOf(c)); + } + } + return joiner.toString(); + } + + /** + * @hide + */ + public static String capabilityNameOf(int capability) { + switch (capability) { + case NET_CAPABILITY_MMS: return "MMS"; + case NET_CAPABILITY_SUPL: return "SUPL"; + case NET_CAPABILITY_DUN: return "DUN"; + case NET_CAPABILITY_FOTA: return "FOTA"; + case NET_CAPABILITY_IMS: return "IMS"; + case NET_CAPABILITY_CBS: return "CBS"; + case NET_CAPABILITY_WIFI_P2P: return "WIFI_P2P"; + case NET_CAPABILITY_IA: return "IA"; + case NET_CAPABILITY_RCS: return "RCS"; + case NET_CAPABILITY_XCAP: return "XCAP"; + case NET_CAPABILITY_EIMS: return "EIMS"; + case NET_CAPABILITY_NOT_METERED: return "NOT_METERED"; + case NET_CAPABILITY_INTERNET: return "INTERNET"; + case NET_CAPABILITY_NOT_RESTRICTED: return "NOT_RESTRICTED"; + case NET_CAPABILITY_TRUSTED: return "TRUSTED"; + case NET_CAPABILITY_NOT_VPN: return "NOT_VPN"; + case NET_CAPABILITY_VALIDATED: return "VALIDATED"; + case NET_CAPABILITY_CAPTIVE_PORTAL: return "CAPTIVE_PORTAL"; + case NET_CAPABILITY_FOREGROUND: return "FOREGROUND"; + default: return Integer.toString(capability); } - StringBuilder transports = new StringBuilder(); - for (int t : types) { - transports.append("|").append(transportNameOf(t)); + } + + /** + * @hide + */ + public static String transportNamesOf(int[] types) { + StringJoiner joiner = new StringJoiner("|"); + if (types != null) { + for (int t : types) { + joiner.add(transportNameOf(t)); + } } - return transports.substring(1); + return joiner.toString(); } /** diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index d3d043382d57..897280262b1e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -4702,10 +4702,12 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private void updateCapabilities( int oldScore, NetworkAgentInfo nai, NetworkCapabilities networkCapabilities) { - if (nai.everConnected && !nai.networkCapabilities.equalImmutableCapabilities( - networkCapabilities)) { - Slog.wtf(TAG, "BUG: " + nai + " changed immutable capabilities: " - + nai.networkCapabilities + " -> " + networkCapabilities); + // Sanity check: a NetworkAgent should not change its static capabilities or parameters. + if (nai.everConnected) { + String diff = nai.networkCapabilities.describeImmutableDifferences(networkCapabilities); + if (!TextUtils.isEmpty(diff)) { + Slog.wtf(TAG, "BUG: " + nai + " changed immutable capabilities:" + diff); + } } // Don't modify caller's NetworkCapabilities. -- 2.11.0