Merge changes I86cb5af0,I706e40db into rvc-dev

* changes:
  Fix dumpVersion for IpMemoryStore on Q
  Log interface hash in NetworkStack dumpsys version

2 files changed, 171 insertions, 26 deletions

diff --git a/src/com/android/server/NetworkStackService.java b/src/com/android/server/NetworkStackService.java
index e478a80..5de5837 100644
--- a/src/com/android/server/NetworkStackService.java
+++ b/src/com/android/server/NetworkStackService.java
@@ -48,6 +48,7 @@ import android.os.HandlerThread;
 import android.os.IBinder;
 import android.os.Looper;
 import android.os.RemoteException;
+import android.text.TextUtils;
 import android.util.ArraySet;
 
 import androidx.annotation.NonNull;
@@ -67,8 +68,15 @@ import java.io.PrintWriter;
 import java.lang.ref.WeakReference;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.List;
+import java.util.Objects;
+import java.util.SortedSet;
+import java.util.TreeSet;
 
 /**
  * Android service used to start the network stack when bound to via an intent.
@@ -190,10 +198,66 @@ public class NetworkStackService extends Service {
 
         private static final String DUMPSYS_ARG_VERSION = "version";
 
-        /** Version of the framework AIDL interfaces observed. Should hold only one value. */
+        private static final String AIDL_KEY_NETWORKSTACK = "networkstack";
+        private static final String AIDL_KEY_IPMEMORYSTORE = "ipmemorystore";
+        private static final String AIDL_KEY_NETD = "netd";
+
+        private static final int VERSION_UNKNOWN = -1;
+        private static final String HASH_UNKNOWN = "unknown";
+
+        /**
+         * Versions of the AIDL interfaces observed by the network stack, in other words versions
+         * that the framework and other modules communicating with the network stack are using.
+         * The map may hold multiple values as the interface is used by modules with different
+         * versions.
+         */
         @GuardedBy("mFrameworkAidlVersions")
-        private final ArraySet<Integer> mFrameworkAidlVersions = new ArraySet<>(1);
-        private final int mNetdAidlVersion;
+        private final ArraySet<AidlVersion> mAidlVersions = new ArraySet<>();
+
+        private static final class AidlVersion implements Comparable<AidlVersion> {
+            @NonNull
+            final String mKey;
+            final int mVersion;
+            @NonNull
+            final String mHash;
+
+            private static final Comparator<AidlVersion> COMPARATOR =
+                    Comparator.comparing((AidlVersion v) -> v.mKey)
+                            .thenComparingInt(v -> v.mVersion)
+                            .thenComparing(v -> v.mHash, String::compareTo);
+
+            AidlVersion(@NonNull String key, int version, @NonNull String hash) {
+                mKey = key;
+                mVersion = version;
+                mHash = hash;
+            }
+
+            @Override
+            public int hashCode() {
+                return Objects.hash(mVersion, mHash);
+            }
+
+            @Override
+            public boolean equals(@Nullable Object obj) {
+                if (!(obj instanceof AidlVersion)) return false;
+                final AidlVersion other = (AidlVersion) obj;
+                return Objects.equals(mKey, other.mKey)
+                        && Objects.equals(mVersion, other.mVersion)
+                        && Objects.equals(mHash, other.mHash);
+            }
+
+            @NonNull
+            @Override
+            public String toString() {
+                // Use a format that can be easily parsed by tests for the version
+                return String.format("%s:%s:%s", mKey, mVersion, mHash);
+            }
+
+            @Override
+            public int compareTo(AidlVersion o) {
+                return COMPARATOR.compare(this, o);
+            }
+        }
 
         private SharedLog addValidationLogs(Network network, String name) {
             final SharedLog log = new SharedLog(NUM_VALIDATION_LOG_LINES, network + " - " + name);
@@ -232,13 +296,16 @@ public class NetworkStackService extends Service {
             }
 
             int netdVersion;
+            String netdHash;
             try {
                 netdVersion = mNetd.getInterfaceVersion();
+                netdHash = mNetd.getInterfaceHash();
             } catch (RemoteException e) {
                 mLog.e("Error obtaining INetd version", e);
-                netdVersion = -1;
+                netdVersion = VERSION_UNKNOWN;
+                netdHash = HASH_UNKNOWN;
             }
-            mNetdAidlVersion = netdVersion;
+            updateNetdAidlVersion(netdVersion, netdHash);
 
             try {
                 mObserverRegistry.register(mNetd);
@@ -247,9 +314,21 @@ public class NetworkStackService extends Service {
             }
         }
 
-        private void updateSystemAidlVersion(final int version) {
-            synchronized (mFrameworkAidlVersions) {
-                mFrameworkAidlVersions.add(version);
+        private void updateNetdAidlVersion(final int version, final String hash) {
+            synchronized (mAidlVersions) {
+                mAidlVersions.add(new AidlVersion(AIDL_KEY_NETD, version, hash));
+            }
+        }
+
+        private void updateNetworkStackAidlVersion(final int version, final String hash) {
+            synchronized (mAidlVersions) {
+                mAidlVersions.add(new AidlVersion(AIDL_KEY_NETWORKSTACK, version, hash));
+            }
+        }
+
+        private void updateIpMemoryStoreAidlVersion(final int version, final String hash) {
+            synchronized (mAidlVersions) {
+                mAidlVersions.add(new AidlVersion(AIDL_KEY_IPMEMORYSTORE, version, hash));
             }
         }
 
@@ -260,7 +339,7 @@ public class NetworkStackService extends Service {
         public void makeDhcpServer(@NonNull String ifName, @NonNull DhcpServingParamsParcel params,
                 @NonNull IDhcpServerCallbacks cb) throws RemoteException {
             mPermChecker.enforceNetworkStackCallingPermission();
-            updateSystemAidlVersion(cb.getInterfaceVersion());
+            updateNetworkStackAidlVersion(cb.getInterfaceVersion(), cb.getInterfaceHash());
             final DhcpServer server;
             try {
                 server = mDeps.makeDhcpServer(
@@ -284,7 +363,7 @@ public class NetworkStackService extends Service {
         public void makeNetworkMonitor(Network network, String name, INetworkMonitorCallbacks cb)
                 throws RemoteException {
             mPermChecker.enforceNetworkStackCallingPermission();
-            updateSystemAidlVersion(cb.getInterfaceVersion());
+            updateNetworkStackAidlVersion(cb.getInterfaceVersion(), cb.getInterfaceHash());
             final SharedLog log = addValidationLogs(network, name);
             final NetworkMonitor nm = mDeps.makeNetworkMonitor(mContext, cb, network, log, this);
             cb.onNetworkMonitorCreated(new NetworkMonitorConnector(nm, mPermChecker));
@@ -293,7 +372,7 @@ public class NetworkStackService extends Service {
         @Override
         public void makeIpClient(String ifName, IIpClientCallbacks cb) throws RemoteException {
             mPermChecker.enforceNetworkStackCallingPermission();
-            updateSystemAidlVersion(cb.getInterfaceVersion());
+            updateNetworkStackAidlVersion(cb.getInterfaceVersion(), cb.getInterfaceHash());
             final IpClient ipClient = mDeps.makeIpClient(
                     mContext, ifName, cb, mObserverRegistry, this);
 
@@ -325,7 +404,7 @@ public class NetworkStackService extends Service {
         public void fetchIpMemoryStore(@NonNull final IIpMemoryStoreCallbacks cb)
                 throws RemoteException {
             mPermChecker.enforceNetworkStackCallingPermission();
-            updateSystemAidlVersion(cb.getInterfaceVersion());
+            updateIpMemoryStoreAidlVersion(cb.getInterfaceVersion(), cb.getInterfaceHash());
             cb.onIpMemoryStoreFetched(mIpMemoryStoreService);
         }
 
@@ -389,11 +468,59 @@ public class NetworkStackService extends Service {
          * Dump version information of the module and detected system version.
          */
         private void dumpVersion(@NonNull PrintWriter fout) {
+            if (!ShimUtils.isReleaseOrDevelopmentApiAbove(Build.VERSION_CODES.Q)) {
+                dumpVersionNumberOnly(fout);
+                return;
+            }
+
+            fout.println("LocalInterface:" + this.VERSION + ":" + this.HASH);
+            synchronized (mAidlVersions) {
+                // Sort versions for deterministic order in output
+                for (AidlVersion version : sortVersions(mAidlVersions)) {
+                    fout.println(version);
+                }
+            }
+        }
+
+        private List<AidlVersion> sortVersions(Collection<AidlVersion> versions) {
+            final List<AidlVersion> sorted = new ArrayList<>(versions);
+            Collections.sort(sorted);
+            return sorted;
+        }
+
+        /**
+         * Legacy version of dumpVersion, only used for Q, as only the interface version number
+         * was used in Q.
+         *
+         * <p>Q behavior needs to be preserved as conformance tests for Q still expect this format.
+         * Once all conformance test suites are updated to expect the new format even on Q devices,
+         * this can be removed.
+         */
+        private void dumpVersionNumberOnly(@NonNull PrintWriter fout) {
             fout.println("NetworkStackConnector: " + this.VERSION);
-            synchronized (mFrameworkAidlVersions) {
-                fout.println("SystemServer: " + mFrameworkAidlVersions);
+            final SortedSet<Integer> systemServerVersions = new TreeSet<>();
+            int netdVersion = VERSION_UNKNOWN;
+            synchronized (mAidlVersions) {
+                for (AidlVersion version : mAidlVersions) {
+                    switch (version.mKey) {
+                        case AIDL_KEY_IPMEMORYSTORE:
+                        case AIDL_KEY_NETWORKSTACK:
+                            systemServerVersions.add(version.mVersion);
+                            break;
+                        case AIDL_KEY_NETD:
+                            netdVersion = version.mVersion;
+                            break;
+                        default:
+                            break;
+                    }
+                }
             }
-            fout.println("Netd: " + mNetdAidlVersion);
+            // TreeSet.toString is formatted as [a, b], but Q used ArraySet.toString formatted as
+            // {a, b}. ArraySet does not have guaranteed ordering, which was not a problem in Q
+            // when only one interface number was expected (and there was no unit test relying on
+            // the ordering).
+            fout.println("SystemServer: {" + TextUtils.join(", ", systemServerVersions) + "}");
+            fout.println("Netd: " + netdVersion);
         }
 
         /**
diff --git a/tests/unit/src/com/android/server/NetworkStackServiceTest.kt b/tests/unit/src/com/android/server/NetworkStackServiceTest.kt
index 52556bc..9de828f 100644
--- a/tests/unit/src/com/android/server/NetworkStackServiceTest.kt
+++ b/tests/unit/src/com/android/server/NetworkStackServiceTest.kt
@@ -40,6 +40,7 @@ import com.android.server.NetworkStackService.PermissionChecker
 import com.android.server.connectivity.NetworkMonitor
 import com.android.server.connectivity.ipmemorystore.IpMemoryStoreService
 import com.android.testutils.DevSdkIgnoreRule
+import com.android.testutils.DevSdkIgnoreRule.IgnoreAfter
 import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo
 import org.junit.Rule
 import org.junit.Test
@@ -62,9 +63,6 @@ import kotlin.test.assertEquals
 private val TEST_NETD_VERSION = 9991001
 private val TEST_NETD_HASH = "test_netd_hash"
 
-private val TEST_IPMEMORYSTORE_VERSION = 9991002
-private val TEST_IPMEMORYSTORE_HASH = "test_ipmemorystore_hash"
-
 private val TEST_IFACE = "test_iface"
 
 @RunWith(AndroidJUnit4::class)
@@ -98,7 +96,7 @@ class NetworkStackServiceTest {
 
     private val connector = NetworkStackConnector(context, permChecker, deps)
 
-    @Test
+    @Test @IgnoreAfter(Build.VERSION_CODES.Q)
     fun testDumpVersion_Q() {
         prepareDumpVersionTest()
 
@@ -115,16 +113,36 @@ class NetworkStackServiceTest {
 
     @Test @IgnoreUpTo(Build.VERSION_CODES.Q)
     fun testDumpVersion() {
-        // TODO: log interface hash on R+ and test it
+        prepareDumpVersionTest()
+
+        val connectorVersion = INetworkStackConnector.VERSION
+        val connectorHash = INetworkStackConnector.HASH
+
+        val dumpsysOut = StringWriter()
+        connector.dump(FileDescriptor(), PrintWriter(dumpsysOut, true /* autoFlush */),
+                arrayOf("version") /* args */)
+
+        assertEquals("NetworkStack version:\n" +
+                "LocalInterface:$connectorVersion:$connectorHash\n" +
+                "ipmemorystore:9990001:ipmemorystore_hash\n" +
+                "netd:$TEST_NETD_VERSION:$TEST_NETD_HASH\n" +
+                "networkstack:9990002:dhcp_server_hash\n" +
+                "networkstack:9990003:networkmonitor_hash\n" +
+                "networkstack:9990004:ipclient_hash\n" +
+                "networkstack:9990005:multiple_use_hash\n\n",
+                dumpsysOut.toString())
     }
 
     fun prepareDumpVersionTest() {
         // Call each method on INetworkStackConnector and verify that it notes down the version of
-        // the remote.
+        // the remote. This is usually a component in the system server that implements one of the
+        // NetworkStack AIDL callback interfaces (e.g., IIpClientCallbacks). On a device there may
+        // be different versions of the generated AIDL classes for different components, even within
+        // the same process (e.g., system_server).
         // Call fetchIpMemoryStore
         val mockIpMemoryStoreCb = mock(IIpMemoryStoreCallbacks::class.java)
         doReturn(9990001).`when`(mockIpMemoryStoreCb).interfaceVersion
-        doReturn("fetch_ipmemorystore_hash").`when`(mockIpMemoryStoreCb).interfaceHash
+        doReturn("ipmemorystore_hash").`when`(mockIpMemoryStoreCb).interfaceHash
 
         connector.fetchIpMemoryStore(mockIpMemoryStoreCb)
         // IpMemoryStore was created at initialization time
@@ -174,10 +192,10 @@ class NetworkStackServiceTest {
 
         // Call some methods one more time with a shared version number and hash to verify no
         // duplicates are reported
-        doReturn(9990005).`when`(mockIpMemoryStoreCb).interfaceVersion
-        doReturn("multiple_use_hash").`when`(mockIpMemoryStoreCb).interfaceHash
-        connector.fetchIpMemoryStore(mockIpMemoryStoreCb)
-        verify(mockIpMemoryStoreCb, times(2)).onIpMemoryStoreFetched(any())
+        doReturn(9990005).`when`(mockIpClientCb).interfaceVersion
+        doReturn("multiple_use_hash").`when`(mockIpClientCb).interfaceHash
+        connector.makeIpClient(TEST_IFACE, mockIpClientCb)
+        verify(mockIpClientCb, times(2)).onIpClientCreated(any())
 
         doReturn(9990005).`when`(mockDhcpCb).interfaceVersion
         doReturn("multiple_use_hash").`when`(mockDhcpCb).interfaceHash