Correct inotify usage and fix strong count accounting error

Fix an error in polling of inotify fd to avoid 100% CPU usage.
Refactored code to use android Looper.

Fix a string count accounting error that causes unpaired decStrong
crashing sensor hidl service.

Bug: 37719320
Bug: 37714835
Test: no longer have 100% CPU usage.
Test: tested connection of a few mouse/keyboard/usb disk/gnubby
      no crash observed.
Test: cts still working properly after fix.
Change-Id: Ibaa026151e5e4919d8dd134c16f5865d5e30ef8a

3 files changed, 87 insertions, 58 deletions

diff --git a/modules/sensors/dynamic_sensor/ConnectionDetector.cpp b/modules/sensors/dynamic_sensor/ConnectionDetector.cpp
index 67a6f272..c51e9125 100644
--- a/modules/sensors/dynamic_sensor/ConnectionDetector.cpp
+++ b/modules/sensors/dynamic_sensor/ConnectionDetector.cpp
@@ -105,30 +105,35 @@ bool SocketConnectionDetector::threadLoop() {
 // FileConnectionDetector functions
 FileConnectionDetector::FileConnectionDetector (
         BaseDynamicSensorDaemon *d, const std::string &path, const std::string &regex)
-            : ConnectionDetector(d), Thread(false /*callCallJava*/), mPath(path), mRegex(regex) {
+            : ConnectionDetector(d), Thread(false /*callCallJava*/), mPath(path), mRegex(regex),
+              mLooper(new Looper(true /*allowNonCallback*/)), mInotifyFd(-1) {
+    if (mLooper == nullptr) {
+        return;
+    }
+
     mInotifyFd = ::inotify_init1(IN_NONBLOCK);
     if (mInotifyFd < 0) {
         ALOGE("Cannot init inotify");
         return;
     }
 
-    int wd = ::inotify_add_watch(mInotifyFd, path.c_str(), IN_CREATE | IN_DELETE | IN_MOVED_FROM);
-    if (wd < 0) {
+    int wd = ::inotify_add_watch(mInotifyFd, path.c_str(), IN_CREATE | IN_DELETE);
+    if (wd < 0 || !mLooper->addFd(mInotifyFd, POLL_IDENT, Looper::EVENT_INPUT, nullptr, nullptr)) {
         ::close(mInotifyFd);
         mInotifyFd = -1;
         ALOGE("Cannot setup watch on dir %s", path.c_str());
         return;
     }
 
-    mPollFd.fd = wd;
-    mPollFd.events = POLLIN;
-
+    // mLooper != null && mInotifyFd added to looper
     run("ddad_file");
 }
 
 FileConnectionDetector::~FileConnectionDetector() {
-    if (mInotifyFd) {
-        requestExitAndWait();
+    if (mInotifyFd > 0) {
+        requestExit();
+        mLooper->wake();
+        join();
         ::close(mInotifyFd);
     }
 }
@@ -153,63 +158,81 @@ void FileConnectionDetector::processExistingFiles() const {
     ::closedir(dirp);
 }
 
-bool FileConnectionDetector::threadLoop() {
+void FileConnectionDetector::handleInotifyData(ssize_t len, const char *data) {
+    const char *dataEnd = data + len;
+    const struct inotify_event *ev;
+
+    // inotify adds paddings to guarantee the next read is aligned
+    for (; data < dataEnd; data += sizeof(struct inotify_event) + ev->len) {
+        ev = reinterpret_cast<const struct inotify_event*>(data);
+        if (ev->mask & IN_ISDIR) {
+            continue;
+        }
+
+        const std::string name(ev->name);
+        if (matches(name)) {
+            if (ev->mask & IN_CREATE) {
+                mDaemon->onConnectionChange(getFullName(name), true /*connected*/);
+            }
+            if (ev->mask & IN_DELETE) {
+                mDaemon->onConnectionChange(getFullName(name), false /*connected*/);
+            }
+        }
+    }
+}
+
+bool FileConnectionDetector::readInotifyData() {
     struct {
-        struct inotify_event e;
-        uint8_t padding[NAME_MAX + 1];
-    } ev;
+        struct inotify_event ev;
+        char padding[NAME_MAX + 1];
+    } buffer;
+
+    bool ret = true;
+    while (true) {
+        ssize_t len = ::read(mInotifyFd, &buffer, sizeof(buffer));
+        if (len == -1 && errno == EAGAIN) {
+            // no more data
+            break;
+        } else if (len > static_cast<ssize_t>(sizeof(struct inotify_event))) {
+            handleInotifyData(len, reinterpret_cast<char*>(&buffer));
+        } else if (len < 0) {
+            ALOGE("read error: %s", ::strerror(errno));
+            ret = false;
+            break;
+        } else {
+            // 0 <= len <= sizeof(struct inotify_event)
+            ALOGE("read return %zd, shorter than inotify_event size %zu",
+                  len, sizeof(struct inotify_event));
+            ret = false;
+            break;
+        }
+    }
+    return ret;
+}
 
+bool FileConnectionDetector::threadLoop() {
+    Looper::setForThread(mLooper);
     processExistingFiles();
+    while(!Thread::exitPending()) {
+        int ret = mLooper->pollOnce(-1);
 
-    while (!Thread::exitPending()) {
-        int pollNum = ::poll(&mPollFd, 1, -1);
-        if (pollNum == -1) {
-           if (errno == EINTR)
-               continue;
-           ALOGE("inotify poll error: %s", ::strerror(errno));
+        if (ret != Looper::POLL_WAKE && ret != POLL_IDENT) {
+            ALOGE("Unexpected value %d from pollOnce, quit", ret);
+            requestExit();
+            break;
         }
 
-        if (pollNum > 0) {
-            if (! (mPollFd.revents & POLLIN)) {
-                continue;
-            }
-
-            /* Inotify events are available */
-            while (true) {
-                /* Read some events. */
-                ssize_t len = ::read(mInotifyFd, &ev, sizeof ev);
-                if (len == -1 && errno != EAGAIN) {
-                    ALOGE("read error: %s", ::strerror(errno));
-                    requestExit();
-                    break;
-                }
-
-                /* If the nonblocking read() found no events to read, then
-                   it returns -1 with errno set to EAGAIN. In that case,
-                   we exit the loop. */
-                if (len <= 0) {
-                    break;
-                }
-
-                if (ev.e.len && !(ev.e.mask & IN_ISDIR)) {
-                    const std::string name(ev.e.name);
-                    ALOGV("device %s state changed", name.c_str());
-                    if (matches(name)) {
-                        if (ev.e.mask & IN_CREATE) {
-                            mDaemon->onConnectionChange(getFullName(name), true /* connected*/);
-                        }
-
-                        if (ev.e.mask & IN_DELETE || ev.e.mask & IN_MOVED_FROM) {
-                            mDaemon->onConnectionChange(getFullName(name), false /* connected*/);
-                        }
-                    }
-                }
+        if (ret == POLL_IDENT) {
+            if (!readInotifyData()) {
+                requestExit();
             }
         }
     }
 
+    mLooper->removeFd(mInotifyFd);
     ALOGD("FileConnectionDetection thread exited");
     return false;
 }
+
 } // namespace SensorHalExt
 } // namespace android
diff --git a/modules/sensors/dynamic_sensor/ConnectionDetector.h b/modules/sensors/dynamic_sensor/ConnectionDetector.h
index 712d832f..0ee1df29 100644
--- a/modules/sensors/dynamic_sensor/ConnectionDetector.h
+++ b/modules/sensors/dynamic_sensor/ConnectionDetector.h
@@ -19,6 +19,7 @@
 
 #include "BaseDynamicSensorDaemon.h"
 #include <utils/Thread.h>
+#include <utils/Looper.h>
 
 #include <regex>
 
@@ -62,17 +63,20 @@ public:
             BaseDynamicSensorDaemon *d, const std::string &path, const std::string &regex);
     virtual ~FileConnectionDetector();
 private:
+    static constexpr int POLL_IDENT = 1;
     // implement virtual of Thread
     virtual bool threadLoop();
 
     bool matches(const std::string &name) const;
     void processExistingFiles() const;
+    void handleInotifyData(ssize_t len, const char *data);
+    bool readInotifyData();
     std::string getFullName(const std::string name) const;
 
     std::string mPath;
     std::regex mRegex;
+    sp<Looper> mLooper;
     int mInotifyFd;
-    struct pollfd mPollFd;
 };
 
 } // namespace SensorHalExt
diff --git a/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp b/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp
index 16e9060a..8aa4cf92 100644
--- a/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp
+++ b/modules/sensors/dynamic_sensor/HidRawSensorDevice.cpp
@@ -37,7 +37,7 @@ const std::unordered_set<unsigned int> HidRawSensorDevice::sInterested{
 
 sp<HidRawSensorDevice> HidRawSensorDevice::create(const std::string &devName) {
     sp<HidRawSensorDevice> device(new HidRawSensorDevice(devName));
-    // remove +1 strong count added by constructor
+    // offset +1 strong count added by constructor
     device->decStrong(device.get());
 
     if (device->mValid) {
@@ -50,13 +50,15 @@ sp<HidRawSensorDevice> HidRawSensorDevice::create(const std::string &devName) {
 HidRawSensorDevice::HidRawSensorDevice(const std::string &devName)
         : RefBase(), HidRawDevice(devName, sInterested),
           Thread(false /*canCallJava*/), mValid(false) {
-    if (!HidRawDevice::isValid()) {
-        return;
-    }
     // create HidRawSensor objects from digest
     // HidRawSensor object will take sp<HidRawSensorDevice> as parameter, so increment strong count
     // to prevent "this" being destructed.
     this->incStrong(this);
+
+    if (!HidRawDevice::isValid()) {
+        return;
+    }
+
     for (const auto &digest : mDigestVector) { // for each usage - vec<ReportPacket> pair
         uint32_t usage = static_cast<uint32_t>(digest.fullUsage);
         sp<HidRawSensor> s(new HidRawSensor(this, usage, digest.packets));