commit 4f22786cc9c9b22d353487678058e2075a968a9d
author Tom Cherry <tomcherry@google.com> Mon Oct 08 17:33:50 2018 -0700
committer Tom Cherry <tomcherry@google.com> Fri Oct 12 18:28:59 2018 -0700
tree a310d4bcf5d6fc314df454fc8aa1c431ed8703b1
parent b3bc8427505c55a4134eac438f118a9b1acb4e2f

logd: rework logic for LogTimeEntry

LogTimeEntry's lifecycle is spread out in various locations.  It
further seems incomplete as there is logic that assumes that its
associated thread can exit while the underlying LogTimeEntry remains
valid, however it doesn't appear that that is actually a supported
situation.

This change simplifies this logic to have only one valid state for a
LogTimeEntry: it must have its thread running and be present in
LastLogTimes.  A LogTimeEntry will never be placed into LastLogTimes
unless its thread is running and its thread will remove its associated
LogTimeEntry from LastLogTimes before it has exited.

This admittedly breaks situations where a blocking socket gets issued
multiple commands with different pid filters, tail lines, etc,
however, I'm reasonably sure that these situations were already
broken.  A check is added to close the socket in this case.

Test: multiple logcat instances work, logd.reader.per's are cleaned up
Change-Id: Ibe8651e7d530c5e9a8d6ce3150cd247982887cbe

logd/LogReader.cpp

diff --git a/logd/LogReader.cpp b/logd/LogReader.cpp
index 2b6556d..13c7af3 100644
--- a/logd/LogReader.cpp
+++ b/logd/LogReader.cpp
@@ -41,6 +41,7 @@
     runOnEachSocket(&command);
 }
 
+// Note returning false will release the SocketClient instance.
 bool LogReader::onDataAvailable(SocketClient* cli) {
     static bool name_set;
     if (!name_set) {
@@ -57,6 +58,18 @@
     }
     buffer[len] = '\0';
 
+    // Clients are only allowed to send one command, disconnect them if they
+    // send another.
+    LogTimeEntry::wrlock();
+    for (const auto& entry : mLogbuf.mTimes) {
+        if (entry->mClient == cli) {
+            entry->release_Locked();
+            LogTimeEntry::unlock();
+            return false;
+        }
+    }
+    LogTimeEntry::unlock();
+
     unsigned long tail = 0;
     static const char _tail[] = " tail=";
     char* cp = strstr(buffer, _tail);
@@ -199,14 +212,25 @@
         cli->getUid(), cli->getGid(), cli->getPid(), nonBlock ? 'n' : 'b', tail,
         logMask, (int)pid, sequence.nsec(), timeout);
 
-    FlushCommand command(*this, nonBlock, tail, logMask, pid, sequence, timeout);
+    LogTimeEntry::wrlock();
+    auto entry = std::make_unique<LogTimeEntry>(
+        *this, cli, nonBlock, tail, logMask, pid, sequence, timeout);
+    if (!entry->startReader_Locked()) {
+        LogTimeEntry::unlock();
+        return false;
+    }
+
+    // release client and entry reference counts once done
+    cli->incRef();
+    mLogbuf.mTimes.emplace_front(std::move(entry));
 
     // Set acceptable upper limit to wait for slow reader processing b/27242723
     struct timeval t = { LOGD_SNDTIMEO, 0 };
     setsockopt(cli->getSocket(), SOL_SOCKET, SO_SNDTIMEO, (const char*)&t,
                sizeof(t));
 
-    command.runSocketCommand(cli);
+    LogTimeEntry::unlock();
+
     return true;
 }
 
@@ -215,9 +239,8 @@
     LogTimeEntry::wrlock();
     LastLogTimes::iterator it = times.begin();
     while (it != times.end()) {
-        LogTimeEntry* entry = (*it);
+        LogTimeEntry* entry = it->get();
         if (entry->mClient == cli) {
-            times.erase(it);
             entry->release_Locked();
             break;
         }