Merge "WifiLogger: fix bug in HAL callback registration" into nyc-dev

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 047794f98..f46440c10 100644
--- a/service/java/com/android/server/wifi/WifiLogger.java
+++ b/service/java/com/android/server/wifi/WifiLogger.java
@@ -97,6 +97,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;
@@ -107,6 +108,7 @@ class WifiLogger extends BaseWifiLogger {
             WifiStateMachine wifiStateMachine, WifiNative wifiNative) {
         mWifiStateMachine = wifiStateMachine;
         mWifiNative = wifiNative;
+        mIsLoggingEventHandlerRegistered = false;
     }
 
     @Override
@@ -115,8 +117,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;
@@ -163,14 +166,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 64bb880fe..357818e9a 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;
 
@@ -83,6 +84,48 @@ public class WifiLoggerTest {
         mWifiNative.enableVerboseLogging(0);
     }
 
+    /** 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.
      *
@@ -102,6 +145,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.
      */