commit 50d83ff1b383bb0f8f51fb834b29594370272070
author Alex Buynytskyy <alexbuy@google.com> Tue Mar 23 22:37:02 2021 -0700
committer Alex Buynytskyy <alexbuy@google.com> Tue Mar 23 22:50:15 2021 -0700
tree 77142904eefe7fba83a4a5f3a62971fb81426003
parent bcb2fe0cbb6b93e16f34d113c1da1bfd14c6180c

Reduce ifs lock scope.

Bug: 182214420
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: Ic36002205c93b316b28ba10ea8f5fbc50dcff70a

services/incremental/IncrementalService.cpp

diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 60d9ea2..217b621 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -323,6 +323,14 @@
     }
 }
 
+void IncrementalService::IncFsMount::setReadLogsRequested(bool value) {
+    if (value) {
+        flags |= StorageFlags::ReadLogsRequested;
+    } else {
+        flags &= ~StorageFlags::ReadLogsRequested;
+    }
+}
+
 IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_view rootDir)
       : mVold(sm.getVoldService()),
         mDataLoaderManager(sm.getDataLoaderManager()),
@@ -804,32 +812,38 @@
         return -EINVAL;
     }
 
-    std::unique_lock l(ifs->lock);
-    if (!enableReadLogs) {
-        return disableReadLogsLocked(*ifs);
-    }
+    std::string packageName;
 
-    if (!ifs->readLogsAllowed()) {
-        LOG(ERROR) << "enableReadLogs failed, readlogs disallowed for storageId: " << storageId;
-        return -EPERM;
-    }
+    {
+        std::unique_lock l(ifs->lock);
+        if (!enableReadLogs) {
+            return disableReadLogsLocked(*ifs);
+        }
 
-    if (!ifs->dataLoaderStub) {
-        // This should never happen - only DL can call enableReadLogs.
-        LOG(ERROR) << "enableReadLogs failed: invalid state";
-        return -EPERM;
-    }
+        if (!ifs->readLogsAllowed()) {
+            LOG(ERROR) << "enableReadLogs failed, readlogs disallowed for storageId: " << storageId;
+            return -EPERM;
+        }
 
-    // Check installation time.
-    const auto now = mClock->now();
-    const auto startLoadingTs = ifs->startLoadingTs;
-    if (startLoadingTs <= now && now - startLoadingTs > getReadLogsMaxInterval()) {
-        LOG(ERROR) << "enableReadLogs failed, readlogs can't be enabled at this time, storageId: "
-                   << storageId;
-        return -EPERM;
-    }
+        if (!ifs->dataLoaderStub) {
+            // This should never happen - only DL can call enableReadLogs.
+            LOG(ERROR) << "enableReadLogs failed: invalid state";
+            return -EPERM;
+        }
 
-    const auto& packageName = ifs->dataLoaderStub->params().packageName;
+        // Check installation time.
+        const auto now = mClock->now();
+        const auto startLoadingTs = ifs->startLoadingTs;
+        if (startLoadingTs <= now && now - startLoadingTs > getReadLogsMaxInterval()) {
+            LOG(ERROR)
+                    << "enableReadLogs failed, readlogs can't be enabled at this time, storageId: "
+                    << storageId;
+            return -EPERM;
+        }
+
+        packageName = ifs->dataLoaderStub->params().packageName;
+        ifs->setReadLogsRequested(true);
+    }
 
     // Check loader usage stats permission and apop.
     if (auto status =
@@ -849,8 +863,14 @@
         return fromBinderStatus(status);
     }
 
-    if (auto status = applyStorageParamsLocked(*ifs, /*enableReadLogs=*/true); status != 0) {
-        return status;
+    {
+        std::unique_lock l(ifs->lock);
+        if (!ifs->readLogsRequested()) {
+            return 0;
+        }
+        if (auto status = applyStorageParamsLocked(*ifs, /*enableReadLogs=*/true); status != 0) {
+            return status;
+        }
     }
 
     registerAppOpsCallback(packageName);
@@ -859,6 +879,7 @@
 }
 
 int IncrementalService::disableReadLogsLocked(IncFsMount& ifs) {
+    ifs.setReadLogsRequested(false);
     return applyStorageParamsLocked(ifs, /*enableReadLogs=*/false);
 }
 
@@ -2214,7 +2235,6 @@
         affected.reserve(mMounts.size());
         for (auto&& [id, ifs] : mMounts) {
             std::unique_lock ll(ifs->lock);
-
             if (ifs->mountId == id && ifs->dataLoaderStub &&
                 ifs->dataLoaderStub->params().packageName == packageName) {
                 affected.push_back(ifs);
@@ -2222,7 +2242,8 @@
         }
     }
     for (auto&& ifs : affected) {
-        applyStorageParamsLocked(*ifs, /*enableReadLogs=*/false);
+        std::unique_lock ll(ifs->lock);
+        disableReadLogsLocked(*ifs);
     }
 }