commit e7cf1424b902e14548642d4dcbc9ce7bd957fa7a
author Steven Moreland <smoreland@google.com> Fri Feb 01 20:17:25 2019 -0800
committer Steven Moreland <smoreland@google.com> Tue Feb 05 13:51:24 2019 -0800
tree 980fcdbf530b972e67c6ebd0f2e0bac52729208a
parent 2fa0f5509bde0d6922ad605b35566358a78a1151

Only access lazy HAL feature when clients do

This was causing some excess kernel logs.

Fixes: 123769263
Test: hwservicemanager_test
    HidlServiceLazyTest ClientWithoutLazy (now passes)
Test: internal 'lazy_hidl_test' fuzzing health storage HAL, for sanity

Change-Id: Ie47c9e085c6f251a9fe1806d7f7f70865e6f5f12

HidlService.cpp

diff --git a/HidlService.cpp b/HidlService.cpp
index 6d020a4..e56579f 100644
--- a/HidlService.cpp
+++ b/HidlService.cpp
@@ -105,9 +105,13 @@
 }
 
 void HidlService::addClientCallback(const sp<IClientCallback>& callback) {
-    handleClientCallbacks(false /* isCalledOnInterval */);
+    if (mHasClients) {
+        // we have this kernel feature, so make sure we're in an updated state
+        forceHandleClientCallbacks(false /*onInterval*/);
+    }
 
     if (mHasClients) {
+        // make sure this callback is in the same state as all of the rest
         sendClientCallbackNotification(callback, true /*hasClients*/);
     }
 
@@ -130,6 +134,14 @@
 }
 
 ssize_t HidlService::handleClientCallbacks(bool isCalledOnInterval) {
+    if (!mClientCallbacks.empty()) {
+        return forceHandleClientCallbacks(isCalledOnInterval);
+    }
+
+    return -1;
+}
+
+ssize_t HidlService::forceHandleClientCallbacks(bool isCalledOnInterval) {
     ssize_t count = getNodeStrongRefCount();
 
     // binder driver doesn't support this feature

HidlService.h

diff --git a/HidlService.h b/HidlService.h
index 426df8e..4214d00 100644
--- a/HidlService.h
+++ b/HidlService.h
@@ -71,10 +71,14 @@
     void addClientCallback(const sp<IClientCallback>& callback);
     bool removeClientCallback(const sp<IClientCallback>& callback);
 
-    // return is number of clients (-1 means this is not implemented)
+    // return is number of clients (-1 means this is not implemented or we didn't check)
     // count includes one held by hwservicemanager
     ssize_t handleClientCallbacks(bool isCalledOnInterval);
 
+    // Updates client callbacks (even if mClientCallbacks is emtpy)
+    // see handleClientCallbacks
+    ssize_t forceHandleClientCallbacks(bool isCalledOnInterval);
+
     // when giving out a handle to a client, but the kernel might not know this yet
     void guaranteeClient();
 

ServiceManager.cpp

diff --git a/ServiceManager.cpp b/ServiceManager.cpp
index ac142f5..9f45801 100644
--- a/ServiceManager.cpp
+++ b/ServiceManager.cpp
@@ -654,7 +654,7 @@
         return false;
     }
 
-    int clients = registered->handleClientCallbacks(false /* isCalledOnInterval */);
+    int clients = registered->forceHandleClientCallbacks(false /* isCalledOnInterval */);
 
     // clients < 0: feature not implemented or other error. Assume clients.
     // Otherwise:

test_lazy.cpp

diff --git a/test_lazy.cpp b/test_lazy.cpp
index df42329..21108ba 100644
--- a/test_lazy.cpp
+++ b/test_lazy.cpp
@@ -51,21 +51,32 @@
     // Note that this should include one count for hwservicemanager. A count of
     // 1 indicates that hwservicemanager is the only process holding the service.
     void setReportedClientCount(ssize_t count) {
-        mInjectedReportCount = count;
+        mState.mInjectedReportCount = count;
+    }
+
+    // Essentially, the number of times the kernel API would be called
+    size_t getNumTimesReported() {
+        return mState.mInjectedTimes;
     }
 
     std::unique_ptr<HidlService> makeService() {
         auto service = std::make_unique<NiceMock<MockHidlService>>();
-        ON_CALL(*service, getNodeStrongRefCount()).WillByDefault(Invoke([&]() { return mInjectedReportCount; }));
+        ON_CALL(*service, getNodeStrongRefCount()).WillByDefault(Invoke([&]() {
+            mState.mInjectedTimes++;
+            return mState.mInjectedReportCount;
+        }));
         return service;
     }
 
 protected:
     void SetUp() override {
-        mInjectedReportCount = -1;
+        mState = TestState();
     }
 
-    ssize_t mInjectedReportCount = -1;
+    struct TestState {
+        ssize_t mInjectedReportCount = -1;
+        size_t mInjectedTimes = 0;
+    } mState;
 };
 
 TEST_F(HidlServiceLazyTest, NoChange) {
@@ -197,3 +208,13 @@
     ASSERT_THAT(cb->stream, ElementsAre(true, false)); // reported only after two intervals
     ASSERT_THAT(laterCb->stream, ElementsAre(true, false)); // reported only after two intervals
 }
+
+TEST_F(HidlServiceLazyTest, ClientWithoutLazy) {
+    std::unique_ptr<HidlService> service = makeService();
+
+    setReportedClientCount(2);
+    service->handleClientCallbacks(false /*onInterval*/);
+
+    // kernel API should not be called
+    EXPECT_EQ(0u, getNumTimesReported());
+}