Follow up I1f0c56651eaa59f0ce90cdb08c71e89a96c48dd4

Beside addressing the comments on this change, this adds a check that
the token in end( is correct and prints a message if not. This is useful
when creating new dumping methods.

Test: adb shell dumpsys print
Change-Id: Ic2e6152cbd82f98d9d305a15edffc69c55fd1fd3

diff --git a/core/java/com/android/internal/print/DualDumpOutputStream.java b/core/java/com/android/internal/print/DualDumpOutputStream.java
index f7826cc..4b10ef2 100644 (file)
--- a/core/java/com/android/internal/print/DualDumpOutputStream.java
+++ b/core/java/com/android/internal/print/DualDumpOutputStream.java
@@ -18,11 +18,10 @@ package com.android.internal.print;
 
 import android.annotation.NonNull;
 import android.annotation.Nullable;
-import android.util.ArrayMap;
+import android.util.Log;
 import android.util.proto.ProtoOutputStream;
 
 import com.android.internal.util.IndentingPrintWriter;
-import com.android.internal.util.Preconditions;
 
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -36,6 +35,8 @@ import java.util.LinkedList;
  * <p>This mirrors the interface of {@link ProtoOutputStream}.
  */
 public class DualDumpOutputStream {
+    private static final String LOG_TAG = DualDumpOutputStream.class.getSimpleName();
+
     // When writing to a proto, the proto
     private final @Nullable ProtoOutputStream mProtoStream;
 
@@ -44,18 +45,18 @@ public class DualDumpOutputStream {
     // Temporary storage of data when printing to mIpw
     private final LinkedList<DumpObject> mDumpObjects = new LinkedList<>();
 
-    private static abstract class DumpAble {
+    private static abstract class Dumpable {
         final String name;
 
-        private DumpAble(String name) {
+        private Dumpable(String name) {
             this.name = name;
         }
 
         abstract void print(IndentingPrintWriter ipw, boolean printName);
     }
 
-    private static class DumpObject extends DumpAble {
-        private final LinkedHashMap<String, ArrayList<DumpAble>> mSubObjects = new LinkedHashMap<>();
+    private static class DumpObject extends Dumpable {
+        private final LinkedHashMap<String, ArrayList<Dumpable>> mSubObjects = new LinkedHashMap<>();
 
         private DumpObject(String name) {
             super(name);
@@ -70,7 +71,7 @@ public class DualDumpOutputStream {
             }
             ipw.increaseIndent();
 
-            for (ArrayList<DumpAble> subObject: mSubObjects.values()) {
+            for (ArrayList<Dumpable> subObject: mSubObjects.values()) {
                 int numDumpables = subObject.size();
 
                 if (numDumpables == 1) {
@@ -100,8 +101,8 @@ public class DualDumpOutputStream {
          * @param fieldName name of the field added
          * @param d The dumpable to add
          */
-        public void add(String fieldName, DumpAble d) {
-            ArrayList<DumpAble> l = mSubObjects.get(fieldName);
+        public void add(String fieldName, Dumpable d) {
+            ArrayList<Dumpable> l = mSubObjects.get(fieldName);
 
             if (l == null) {
                 l = new ArrayList<>(1);
@@ -112,7 +113,7 @@ public class DualDumpOutputStream {
         }
     }
 
-    private static class DumpField extends DumpAble {
+    private static class DumpField extends Dumpable {
         private final String mValue;
 
         private DumpField(String name, String value) {
@@ -139,7 +140,10 @@ public class DualDumpOutputStream {
      */
     public DualDumpOutputStream(@Nullable ProtoOutputStream proto,
             @Nullable IndentingPrintWriter ipw) {
-        Preconditions.checkArgument((proto == null) != (ipw == null));
+        if ((proto == null) == (ipw == null)) {
+            Log.e(LOG_TAG, "Cannot dump to clear text and proto at once. Ignoring proto");
+            proto = null;
+        }
 
         mProtoStream = proto;
         mIpw = ipw;
@@ -213,7 +217,7 @@ public class DualDumpOutputStream {
             DumpObject d = new DumpObject(fieldName);
             mDumpObjects.getLast().add(fieldName, d);
             mDumpObjects.addLast(d);
-            return 0;
+            return System.identityHashCode(d);
         }
     }
 
@@ -221,6 +225,10 @@ public class DualDumpOutputStream {
         if (mProtoStream != null) {
             mProtoStream.end(token);
         } else {
+            if (System.identityHashCode(mDumpObjects.getLast()) != token) {
+                Log.w(LOG_TAG, "Unexpected token for ending " + mDumpObjects.getLast().name
+                                + " at " + Arrays.toString(Thread.currentThread().getStackTrace()));
+            }
             mDumpObjects.removeLast();
         }
     }
@@ -250,7 +258,10 @@ public class DualDumpOutputStream {
      * @param nestedState The state of the dump
      */
     public void writeNested(@NonNull String fieldName, byte[] nestedState) {
-        Preconditions.checkNotNull(mIpw);
+        if (mIpw == null) {
+            Log.w(LOG_TAG, "writeNested does not work for proto logging");
+            return;
+        }
 
         mDumpObjects.getLast().add(fieldName,
                 new DumpField(fieldName, (new String(nestedState, StandardCharsets.UTF_8)).trim()));

diff --git a/packages/PrintSpooler/src/com/android/printspooler/model/PrintSpoolerService.java b/packages/PrintSpooler/src/com/android/printspooler/model/PrintSpoolerService.java
index 074f9ef..6c74418 100644 (file)
--- a/packages/PrintSpooler/src/com/android/printspooler/model/PrintSpoolerService.java
+++ b/packages/PrintSpooler/src/com/android/printspooler/model/PrintSpoolerService.java
@@ -161,11 +161,11 @@ public final class PrintSpoolerService extends Service {
         return new PrintSpooler();
     }
 
-    private void dumpLocked(@NonNull DualDumpOutputStream proto) {
+    private void dumpLocked(@NonNull DualDumpOutputStream dumpStream) {
         int numPrintJobs = mPrintJobs.size();
         for (int i = 0; i < numPrintJobs; i++) {
-            writePrintJobInfo(this, proto, "print_jobs", PrintSpoolerInternalStateProto.PRINT_JOBS,
-                    mPrintJobs.get(i));
+            writePrintJobInfo(this, dumpStream, "print_jobs",
+                    PrintSpoolerInternalStateProto.PRINT_JOBS, mPrintJobs.get(i));
         }
 
         File[] files = getFilesDir().listFiles();
@@ -173,8 +173,8 @@ public final class PrintSpoolerService extends Service {
             for (int i = 0; i < files.length; i++) {
                 File file = files[i];
                 if (file.isFile() && file.getName().startsWith(PRINT_JOB_FILE_PREFIX)) {
-                    proto.write("print_job_files", PrintSpoolerInternalStateProto.PRINT_JOB_FILES,
-                            file.getName());
+                    dumpStream.write("print_job_files",
+                            PrintSpoolerInternalStateProto.PRINT_JOB_FILES, file.getName());
                 }
             }
         }
@@ -184,13 +184,13 @@ public final class PrintSpoolerService extends Service {
             for (String approvedService : approvedPrintServices) {
                 ComponentName componentName = ComponentName.unflattenFromString(approvedService);
                 if (componentName != null) {
-                    writeComponentName(proto, "approved_services",
+                    writeComponentName(dumpStream, "approved_services",
                             PrintSpoolerInternalStateProto.APPROVED_SERVICES, componentName);
                 }
             }
         }
 
-        proto.flush();
+        dumpStream.flush();
     }
 
     @Override

diff --git a/services/print/java/com/android/server/print/PrintManagerService.java b/services/print/java/com/android/server/print/PrintManagerService.java
index 1dc9d26..cd4e8f9 100644 (file)
--- a/services/print/java/com/android/server/print/PrintManagerService.java
+++ b/services/print/java/com/android/server/print/PrintManagerService.java
@@ -685,16 +685,16 @@ public final class PrintManagerService extends SystemService {
             }
         }
 
-        private void dump(@NonNull DualDumpOutputStream proto,
+        private void dump(@NonNull DualDumpOutputStream dumpStream,
                 @NonNull ArrayList<UserState> userStatesToDump) {
             final int userStateCount = userStatesToDump.size();
             for (int i = 0; i < userStateCount; i++) {
-                long token = proto.start("user_states", PrintServiceDumpProto.USER_STATES);
-                userStatesToDump.get(i).dump(proto);
-                proto.end(token);
+                long token = dumpStream.start("user_states", PrintServiceDumpProto.USER_STATES);
+                userStatesToDump.get(i).dump(dumpStream);
+                dumpStream.end(token);
             }
 
-            proto.flush();
+            dumpStream.flush();
         }
 
         private void registerContentObservers() {

diff --git a/services/print/java/com/android/server/print/RemotePrintSpooler.java b/services/print/java/com/android/server/print/RemotePrintSpooler.java
index 5520255..a69baa1 100644 (file)
--- a/services/print/java/com/android/server/print/RemotePrintSpooler.java
+++ b/services/print/java/com/android/server/print/RemotePrintSpooler.java
@@ -556,18 +556,18 @@ final class RemotePrintSpooler {
         }
     }
 
-    public void dump(@NonNull DualDumpOutputStream proto) {
+    public void dump(@NonNull DualDumpOutputStream dumpStream) {
         synchronized (mLock) {
-            proto.write("is_destroyed", PrintSpoolerStateProto.IS_DESTROYED, mDestroyed);
-            proto.write("is_bound", PrintSpoolerStateProto.IS_BOUND, mRemoteInstance != null);
+            dumpStream.write("is_destroyed", PrintSpoolerStateProto.IS_DESTROYED, mDestroyed);
+            dumpStream.write("is_bound", PrintSpoolerStateProto.IS_BOUND, mRemoteInstance != null);
         }
 
         try {
-            if (proto.isProto()) {
-                proto.write(null, PrintSpoolerStateProto.INTERNAL_STATE,
+            if (dumpStream.isProto()) {
+                dumpStream.write(null, PrintSpoolerStateProto.INTERNAL_STATE,
                         TransferPipe.dumpAsync(getRemoteInstanceLazy().asBinder(), "--proto"));
             } else {
-                proto.writeNested("internal_state", TransferPipe.dumpAsync(
+                dumpStream.writeNested("internal_state", TransferPipe.dumpAsync(
                         getRemoteInstanceLazy().asBinder()));
             }
         } catch (IOException | TimeoutException | RemoteException | InterruptedException e) {

diff --git a/services/print/java/com/android/server/print/UserState.java b/services/print/java/com/android/server/print/UserState.java
index 318e481..e2808e8 100644 (file)
--- a/services/print/java/com/android/server/print/UserState.java
+++ b/services/print/java/com/android/server/print/UserState.java
@@ -815,61 +815,63 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks,
         mDestroyed = true;
     }
 
-    public void dump(@NonNull DualDumpOutputStream proto) {
+    public void dump(@NonNull DualDumpOutputStream dumpStream) {
         synchronized (mLock) {
-            proto.write("user_id", PrintUserStateProto.USER_ID, mUserId);
+            dumpStream.write("user_id", PrintUserStateProto.USER_ID, mUserId);
 
             final int installedServiceCount = mInstalledServices.size();
             for (int i = 0; i < installedServiceCount; i++) {
-                long token = proto.start("installed_services",
+                long token = dumpStream.start("installed_services",
                         PrintUserStateProto.INSTALLED_SERVICES);
                 PrintServiceInfo installedService = mInstalledServices.get(i);
 
                 ResolveInfo resolveInfo = installedService.getResolveInfo();
-                writeComponentName(proto, "component_name",
+                writeComponentName(dumpStream, "component_name",
                         InstalledPrintServiceProto.COMPONENT_NAME,
                         new ComponentName(resolveInfo.serviceInfo.packageName,
                                 resolveInfo.serviceInfo.name));
 
-                writeStringIfNotNull(proto, "settings_activity",
+                writeStringIfNotNull(dumpStream, "settings_activity",
                         InstalledPrintServiceProto.SETTINGS_ACTIVITY,
                         installedService.getSettingsActivityName());
-                writeStringIfNotNull(proto, "add_printers_activity",
+                writeStringIfNotNull(dumpStream, "add_printers_activity",
                         InstalledPrintServiceProto.ADD_PRINTERS_ACTIVITY,
                         installedService.getAddPrintersActivityName());
-                writeStringIfNotNull(proto, "advanced_options_activity",
+                writeStringIfNotNull(dumpStream, "advanced_options_activity",
                         InstalledPrintServiceProto.ADVANCED_OPTIONS_ACTIVITY,
                         installedService.getAdvancedOptionsActivityName());
 
-                proto.end(token);
+                dumpStream.end(token);
             }
 
             for (ComponentName disabledService : mDisabledServices) {
-                writeComponentName(proto, "disabled_services",
+                writeComponentName(dumpStream, "disabled_services",
                         PrintUserStateProto.DISABLED_SERVICES, disabledService);
             }
 
             final int activeServiceCount = mActiveServices.size();
             for (int i = 0; i < activeServiceCount; i++) {
-                long token = proto.start("actives_services", PrintUserStateProto.ACTIVE_SERVICES);
-                mActiveServices.valueAt(i).dump(proto);
-                proto.end(token);
+                long token = dumpStream.start("actives_services",
+                        PrintUserStateProto.ACTIVE_SERVICES);
+                mActiveServices.valueAt(i).dump(dumpStream);
+                dumpStream.end(token);
             }
 
-            mPrintJobForAppCache.dumpLocked(proto);
+            mPrintJobForAppCache.dumpLocked(dumpStream);
 
             if (mPrinterDiscoverySession != null) {
-                long token = proto.start("discovery_service",
+                long token = dumpStream.start("discovery_service",
                         PrintUserStateProto.DISCOVERY_SESSIONS);
-                mPrinterDiscoverySession.dumpLocked(proto);
-                proto.end(token);
+                mPrinterDiscoverySession.dumpLocked(dumpStream);
+                dumpStream.end(token);
             }
 
         }
 
-        long token = proto.start("print_spooler_state", PrintUserStateProto.PRINT_SPOOLER_STATE);
-        mSpooler.dump(proto);
-        proto.end(token);
+        long token = dumpStream.start("print_spooler_state",
+                PrintUserStateProto.PRINT_SPOOLER_STATE);
+        mSpooler.dump(dumpStream);
+        dumpStream.end(token);
     }
 
     private void readConfigurationLocked() {
@@ -1597,16 +1599,16 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks,
             }
         }
 
-        public void dumpLocked(@NonNull DualDumpOutputStream proto) {
-            proto.write("is_destroyed", PrinterDiscoverySessionProto.IS_DESTROYED, mDestroyed);
-            proto.write("is_printer_discovery_in_progress",
+        public void dumpLocked(@NonNull DualDumpOutputStream dumpStream) {
+            dumpStream.write("is_destroyed", PrinterDiscoverySessionProto.IS_DESTROYED, mDestroyed);
+            dumpStream.write("is_printer_discovery_in_progress",
                     PrinterDiscoverySessionProto.IS_PRINTER_DISCOVERY_IN_PROGRESS,
                     !mStartedPrinterDiscoveryTokens.isEmpty());
 
             final int observerCount = mDiscoveryObservers.beginBroadcast();
             for (int i = 0; i < observerCount; i++) {
                 IPrinterDiscoveryObserver observer = mDiscoveryObservers.getBroadcastItem(i);
-                proto.write("printer_discovery_observers",
+                dumpStream.write("printer_discovery_observers",
                         PrinterDiscoverySessionProto.PRINTER_DISCOVERY_OBSERVERS,
                         observer.toString());
             }
@@ -1615,21 +1617,21 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks,
             final int tokenCount = this.mStartedPrinterDiscoveryTokens.size();
             for (int i = 0; i < tokenCount; i++) {
                 IBinder token = mStartedPrinterDiscoveryTokens.get(i);
-                proto.write("discovery_requests",
+                dumpStream.write("discovery_requests",
                         PrinterDiscoverySessionProto.DISCOVERY_REQUESTS, token.toString());
             }
 
             final int trackedPrinters = mStateTrackedPrinters.size();
             for (int i = 0; i < trackedPrinters; i++) {
                 PrinterId printer = mStateTrackedPrinters.get(i);
-                writePrinterId(proto, "tracked_printer_requests",
+                writePrinterId(dumpStream, "tracked_printer_requests",
                         PrinterDiscoverySessionProto.TRACKED_PRINTER_REQUESTS, printer);
             }
 
             final int printerCount = mPrinters.size();
             for (int i = 0; i < printerCount; i++) {
                 PrinterInfo printer = mPrinters.valueAt(i);
-                writePrinterInfo(mContext, proto, "printer",
+                writePrinterInfo(mContext, dumpStream, "printer",
                         PrinterDiscoverySessionProto.PRINTER, printer);
             }
         }
@@ -1843,22 +1845,22 @@ final class UserState implements PrintSpoolerCallbacks, PrintServiceCallbacks,
             }
         }
 
-        public void dumpLocked(@NonNull DualDumpOutputStream proto) {
+        public void dumpLocked(@NonNull DualDumpOutputStream dumpStream) {
             final int bucketCount = mPrintJobsForRunningApp.size();
             for (int i = 0; i < bucketCount; i++) {
                 final int appId = mPrintJobsForRunningApp.keyAt(i);
                 List<PrintJobInfo> bucket = mPrintJobsForRunningApp.valueAt(i);
                 final int printJobCount = bucket.size();
                 for (int j = 0; j < printJobCount; j++) {
-                    long token = proto.start("cached_print_jobs",
+                    long token = dumpStream.start("cached_print_jobs",
                             PrintUserStateProto.CACHED_PRINT_JOBS);
 
-                    proto.write("app_id", CachedPrintJobProto.APP_ID, appId);
+                    dumpStream.write("app_id", CachedPrintJobProto.APP_ID, appId);
 
-                    writePrintJobInfo(mContext, proto, "print_job", CachedPrintJobProto.PRINT_JOB,
-                            bucket.get(j));
+                    writePrintJobInfo(mContext, dumpStream, "print_job",
+                            CachedPrintJobProto.PRINT_JOB, bucket.get(j));
 
-                    proto.end(token);
+                    dumpStream.end(token);
                 }
             }
         }