WifiLogger: fix bug in HAL callback registration

When we call WifiLogger.startLogging(), WifiLogger
may or may not register callbacks for ring-buffer
data delivery, and WiFi alerts.

Whether or not WifiLogger registers the callbacks
depends on whether or not the callbacks have already
been reigstered. If WifiLogger has already registered
callbacks, then a new call to startLogging() will
skip callback registration.

Now, it's not entirely clear if this already-registered
check is necessary. It is certainly conceivable that the
HAL implementations allow us to replace existing callbacks,
without explicitly removing the existing callbacks first.

But, the necessity of the check aside, the immediate
problem is that the already-registered check fails to
handle the case where an early registration failed.

This CL revises the code, to handle this case.

While there:
- Fix some whitespace issues.
- Remove unnecessary comment about the effects of
  resetLogHandler(). (Even if the comment _were_ necessary,
  it should be part of the Javadoc for resetLogHandler().)

BUG=28274991
TEST=(new) unit tests, manual

Manual test:
- boot device
$ adb shell dumpsys | grep -A1 'ring-buffer = driver_prints_rb'
ring-buffer = driver_prints_rb
<base64 encoded data...>

Change-Id: I04e9bc1863aaf5bda00ca9cf8dc35696ae1e5f24

2 files changed, 95 insertions, 7 deletions

diff --git a/service/java/com/android/server/wifi/WifiLogger.java b/service/java/com/android/server/wifi/WifiLogger.java
index 6d7b608a8..3ffe5837b 100644
--- a/service/java/com/android/server/wifi/WifiLogger.java
+++ b/service/java/com/android/server/wifi/WifiLogger.java
@@ -96,6 +96,7 @@ class WifiLogger extends BaseWifiLogger {
             "Driver state dump";
 
     private int mLogLevel = VERBOSE_NO_LOG;
+    private boolean mIsLoggingEventHandlerRegistered;
     private WifiNative.RingBufferStatus[] mRingBuffers;
     private WifiNative.RingBufferStatus mPerPacketRingBuffer;
     private WifiStateMachine mWifiStateMachine;
@@ -106,6 +107,7 @@ class WifiLogger extends BaseWifiLogger {
             WifiStateMachine wifiStateMachine, WifiNative wifiNative) {
         mWifiStateMachine = wifiStateMachine;
         mWifiNative = wifiNative;
+        mIsLoggingEventHandlerRegistered = false;
     }
 
     @Override
@@ -114,8 +116,9 @@ class WifiLogger extends BaseWifiLogger {
         mDriverVersion = mWifiNative.getDriverVersion();
         mSupportedFeatureSet = mWifiNative.getSupportedLoggerFeatureSet();
 
-        if (mLogLevel == VERBOSE_NO_LOG)
-            mWifiNative.setLoggingEventHandler(mHandler);
+        if (!mIsLoggingEventHandlerRegistered) {
+            mIsLoggingEventHandlerRegistered = mWifiNative.setLoggingEventHandler(mHandler);
+        }
 
         if (verboseEnabled) {
             mLogLevel = VERBOSE_LOG_WITH_WAKEUP;
@@ -162,14 +165,17 @@ class WifiLogger extends BaseWifiLogger {
 
     @Override
     public synchronized void stopLogging() {
-        if (mLogLevel != VERBOSE_NO_LOG) {
-            //resetLogHandler only can be used when you terminate all logging since all handler will
-            //be removed. This also stop alert logging
-            if(!mWifiNative.resetLogHandler()) {
+        if (mIsLoggingEventHandlerRegistered) {
+            if (!mWifiNative.resetLogHandler()) {
                 Log.e(TAG, "Fail to reset log handler");
             } else {
-                if (DBG) Log.d(TAG,"Reset log handler");
+                if (DBG) Log.d(TAG, "Reset log handler");
             }
+            // Clear mIsLoggingEventHandlerRegistered even if resetLogHandler() failed, because
+            // the log handler is in an indeterminate state.
+            mIsLoggingEventHandlerRegistered = false;
+        }
+        if (mLogLevel != VERBOSE_NO_LOG) {
             stopLoggingAllBuffers();
             mRingBuffers = null;
             mLogLevel = VERBOSE_NO_LOG;
diff --git a/tests/wifitests/src/com/android/server/wifi/WifiLoggerTest.java b/tests/wifitests/src/com/android/server/wifi/WifiLoggerTest.java
index 6f68187a9..10b2a8276 100644
--- a/tests/wifitests/src/com/android/server/wifi/WifiLoggerTest.java
+++ b/tests/wifitests/src/com/android/server/wifi/WifiLoggerTest.java
@@ -26,6 +26,7 @@ import static org.mockito.Mockito.anyInt;
 import static org.mockito.Mockito.anyObject;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -82,6 +83,48 @@ public class WifiLoggerTest {
         mWifiLogger = new WifiLogger(mWsm, mWifiNative);
     }
 
+    /** Verifies that startLogging() registers a logging event handler. */
+    @Test
+    public void startLoggingRegistersLogEventHandler() throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+        mWifiLogger.startLogging(verbosityToggle);
+        verify(mWifiNative).setLoggingEventHandler(anyObject());
+    }
+
+    /**
+     * Verifies that a failure to set the logging event handler does not prevent a future
+     * startLogging() from setting the logging event handler.
+     */
+    @Test
+    public void startLoggingRegistersLogEventHandlerIfPriorAttemptFailed()
+            throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+
+        when(mWifiNative.setLoggingEventHandler(anyObject())).thenReturn(false);
+        mWifiLogger.startLogging(verbosityToggle);
+        verify(mWifiNative).setLoggingEventHandler(anyObject());
+        reset(mWifiNative);
+
+        when(mWifiNative.setLoggingEventHandler(anyObject())).thenReturn(true);
+        mWifiLogger.startLogging(verbosityToggle);
+        verify(mWifiNative).setLoggingEventHandler(anyObject());
+    }
+
+    /** Verifies that startLogging() does not make redundant calls to setLoggingEventHandler(). */
+    @Test
+    public void startLoggingDoesNotRegisterLogEventHandlerIfPriorAttemptSucceeded()
+            throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+
+        when(mWifiNative.setLoggingEventHandler(anyObject())).thenReturn(true);
+        mWifiLogger.startLogging(verbosityToggle);
+        verify(mWifiNative).setLoggingEventHandler(anyObject());
+        reset(mWifiNative);
+
+        mWifiLogger.startLogging(verbosityToggle);
+        verify(mWifiNative, never()).setLoggingEventHandler(anyObject());
+    }
+
     /**
      * Verifies that startLogging() restarts HAL ringbuffers.
      *
@@ -101,6 +144,45 @@ public class WifiLoggerTest {
                 eq(FAKE_RING_BUFFER_NAME));
     }
 
+    /** Verifies that, if a log handler was registered, then stopLogging() resets it. */
+    @Test
+    public void stopLoggingResetsLogHandlerIfHandlerWasRegistered() throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+
+        when(mWifiNative.setLoggingEventHandler(anyObject())).thenReturn(true);
+        mWifiLogger.startLogging(verbosityToggle);
+        reset(mWifiNative);
+
+        mWifiLogger.stopLogging();
+        verify(mWifiNative).resetLogHandler();
+    }
+
+    /** Verifies that, if a log handler is not registered, stopLogging() skips resetLogHandler(). */
+    @Test
+    public void stopLoggingOnlyResetsLogHandlerIfHandlerWasRegistered() throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+        mWifiLogger.stopLogging();
+        verify(mWifiNative, never()).resetLogHandler();
+    }
+
+    /** Verifies that stopLogging() remembers that we've reset the log handler. */
+    @Test
+    public void multipleStopLoggingCallsOnlyResetLogHandlerOnce() throws Exception {
+        final boolean verbosityToggle = false;  // even default mode wants log events from HAL
+
+        when(mWifiNative.setLoggingEventHandler(anyObject())).thenReturn(true);
+        mWifiLogger.startLogging(verbosityToggle);
+        reset(mWifiNative);
+
+        when(mWifiNative.resetLogHandler()).thenReturn(true);
+        mWifiLogger.stopLogging();
+        verify(mWifiNative).resetLogHandler();
+        reset(mWifiNative);
+
+        mWifiLogger.stopLogging();
+        verify(mWifiNative, never()).resetLogHandler();
+    }
+
     /**
      * Verifies that we capture ring-buffer data.
      */